[dpdk-dev,v5,2/7] net: Add common PTP structures and functions
Commit Message
This patch add common functions and structures used for PTP processing.
Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
---
lib/librte_net/Makefile | 2 +-
lib/librte_net/rte_ptp.h | 105 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 106 insertions(+), 1 deletion(-)
create mode 100644 lib/librte_net/rte_ptp.h
Comments
2015-11-05 15:06, Daniel Mrzyglod:
> This patch add common functions and structures used for PTP processing.
>
> Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
> ---
> lib/librte_net/Makefile | 2 +-
> lib/librte_net/rte_ptp.h | 105 +++++++++++++++++++++++++++++++++++++++++++++++
The library librte_net gather some structures and functions for network headers/layers
parsing.
These PTP functions looks really generic. Why not add them in EAL?
Maybe they could benefit from specific implementations in the arch/ directory.
> +/*
> + * Structure for cyclecounter IEEE1588 functionality.
> + */
> +struct cyclecounter {
> + uint64_t (*read)(struct rte_eth_dev *dev);
This field is not used.
Please avoid using a reference to ethdev here.
> + uint64_t mask;
> + uint32_t shift;
> +};
> +
> +/*
> + * Structure to hold and calculate Unix epoch time.
> + */
> +struct timecounter {
> + struct cyclecounter *cc;
> + uint64_t cycle_last;
> + uint64_t nsec;
> + uint64_t mask;
> + uint64_t frac;
> +};
This structure is not used.
It is not clear what these structures are for, and what means each field.
When adding a new field in an API, it must be commented in doxygen.
> +static inline uint64_t
> +timespec_to_ns(const struct timespec *ts)
[...]
> +static inline struct timespec
> +ns_to_timespec(uint64_t nsec)
[...]
> +static inline uint64_t
> +cyclecounter_cycles_to_ns(const struct cyclecounter *cc,
> + uint64_t cycles, uint64_t mask, uint64_t *frac)
[...]
> +static inline uint64_t
> +cyclecounter_cycles_to_ns_backwards(const struct cyclecounter *cc,
> + uint64_t cycles, uint64_t frac)
They must be prefixed with rte_ with full doxygen comments.
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Tuesday, November 10, 2015 11:26 AM
> To: Mrzyglod, DanielX T
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 2/7] net: Add common PTP structures and
> functions
>
> 2015-11-05 15:06, Daniel Mrzyglod:
> > This patch add common functions and structures used for PTP processing.
> >
> > Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
> > ---
> > lib/librte_net/Makefile | 2 +-
> > lib/librte_net/rte_ptp.h | 105
> > +++++++++++++++++++++++++++++++++++++++++++++++
>
> The library librte_net gather some structures and functions for network
> headers/layers parsing.
> These PTP functions looks really generic. Why not add them in EAL?
> Maybe they could benefit from specific implementations in the arch/
> directory.
Hi Thomas,
How about the following location and new header file?
lib/librte_eal/common/include/rte_time.h
> > +/*
> > + * Structure for cyclecounter IEEE1588 functionality.
> > + */
> > +struct cyclecounter {
> > + uint64_t (*read)(struct rte_eth_dev *dev);
>
> This field is not used.
> Please avoid using a reference to ethdev here.
Ok. We'll try rework this to be more generic.
> > + uint64_t mask;
> > + uint32_t shift;
> > +};
> > +
> > +/*
> > + * Structure to hold and calculate Unix epoch time.
> > + */
> > +struct timecounter {
> > + struct cyclecounter *cc;
> > + uint64_t cycle_last;
> > + uint64_t nsec;
> > + uint64_t mask;
> > + uint64_t frac;
> > +};
>
> This structure is not used.
>
> It is not clear what these structures are for, and what means each field.
> When adding a new field in an API, it must be commented in Doxygen.
The structure is used. We will add Doxygen comments.
> > +static inline uint64_t
> > +timespec_to_ns(const struct timespec *ts)
> [...]
> > +static inline struct timespec
> > +ns_to_timespec(uint64_t nsec)
> [...]
> > +static inline uint64_t
> > +cyclecounter_cycles_to_ns(const struct cyclecounter *cc,
> > + uint64_t cycles, uint64_t mask, uint64_t *frac)
> [...]
> > +static inline uint64_t
> > +cyclecounter_cycles_to_ns_backwards(const struct cyclecounter *cc,
> > + uint64_t cycles, uint64_t frac)
>
> They must be prefixed with rte_ with full doxygen comments.
We can do that. But is the intention that these function are public? We really only need them internally. It is possible that they could be used somewhere else but probably not likely. How about if we document them but mark them as @internal in Doxygen. Then if they are require by some other libs they can be made external at a later stage.
John
2015-11-11 10:45, Mcnamara, John:
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > The library librte_net gather some structures and functions for network
> > headers/layers parsing.
> > These PTP functions looks really generic. Why not add them in EAL?
> > Maybe they could benefit from specific implementations in the arch/
> > directory.
>
> Hi Thomas,
>
> How about the following location and new header file?
>
> lib/librte_eal/common/include/rte_time.h
Yes
> > > +static inline uint64_t
> > > +timespec_to_ns(const struct timespec *ts)
> > [...]
> > > +static inline struct timespec
> > > +ns_to_timespec(uint64_t nsec)
> > [...]
> > > +static inline uint64_t
> > > +cyclecounter_cycles_to_ns(const struct cyclecounter *cc,
> > > + uint64_t cycles, uint64_t mask, uint64_t *frac)
> > [...]
> > > +static inline uint64_t
> > > +cyclecounter_cycles_to_ns_backwards(const struct cyclecounter *cc,
> > > + uint64_t cycles, uint64_t frac)
> >
> > They must be prefixed with rte_ with full doxygen comments.
>
> We can do that. But is the intention that these function are public? We really only need them internally. It is possible that they could be used somewhere else but probably not likely. How about if we document them but mark them as @internal in Doxygen. Then if they are require by some other libs they can be made external at a later stage.
Yes they can be marked @internal with rte_ prefix.
@@ -34,7 +34,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) -O3
# install includes
-SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include := rte_ip.h rte_tcp.h rte_udp.h rte_sctp.h rte_icmp.h rte_arp.h
+SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include := rte_ip.h rte_tcp.h rte_udp.h rte_sctp.h rte_icmp.h rte_arp.h rte_ptp.h
include $(RTE_SDK)/mk/rte.install.mk
new file mode 100644
@@ -0,0 +1,105 @@
+/*-
+ * BSD LICENSE
+ *
+ * Copyright(c) 2015 Intel Corporation. All rights reserved.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in
+ * the documentation and/or other materials provided with the
+ * distribution.
+ * * Neither the name of Intel Corporation nor the names of its
+ * contributors may be used to endorse or promote products derived
+ * from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#define NSEC_PER_SEC 1000000000L
+
+/*
+ * Structure for cyclecounter IEEE1588 functionality.
+ */
+struct cyclecounter {
+ uint64_t (*read)(struct rte_eth_dev *dev);
+ uint64_t mask;
+ uint32_t shift;
+};
+
+/*
+ * Structure to hold and calculate Unix epoch time.
+ */
+struct timecounter {
+ struct cyclecounter *cc;
+ uint64_t cycle_last;
+ uint64_t nsec;
+ uint64_t mask;
+ uint64_t frac;
+};
+
+
+/* Utility functions for PTP/IEEE1588 support. */
+
+static inline uint64_t
+timespec_to_ns(const struct timespec *ts)
+{
+ return ((uint64_t) ts->tv_sec * NSEC_PER_SEC) + ts->tv_nsec;
+}
+
+static inline struct timespec
+ns_to_timespec(uint64_t nsec)
+{
+ struct timespec ts = {0, 0};
+
+ if (nsec == 0)
+ return ts;
+
+ ts.tv_sec = nsec / NSEC_PER_SEC;
+ ts.tv_nsec = nsec % NSEC_PER_SEC;
+
+ return ts;
+}
+
+/*
+ * Converts cycle counter cycles to nanoseconds.
+ */
+static inline uint64_t
+cyclecounter_cycles_to_ns(const struct cyclecounter *cc,
+ uint64_t cycles, uint64_t mask, uint64_t *frac)
+{
+ uint64_t ns;
+
+ /* Add fractional nanoseconds */
+ ns = cycles + *frac;
+ *frac = ns & mask;
+
+ /* Shift to get only nanoseconds. */
+ return ns >> cc->shift;
+}
+
+/*
+ * Like cyclecounter_cycles_to_ns(), but this is used when
+ * computing a time previous to the stored in the cycle counter.
+ */
+static inline uint64_t
+cyclecounter_cycles_to_ns_backwards(const struct cyclecounter *cc,
+ uint64_t cycles, uint64_t frac)
+{
+ return ((cycles - frac) >> cc->shift);
+}