| Message ID | 1445001927-13916-2-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 16 October 2015 at 15:25, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > --- > include/odp/api/packet_io_stats.h | 166 > +++++++++++++++++++++++++ > platform/linux-generic/include/odp/packet_io.h | 1 + > 2 files changed, 167 insertions(+) > create mode 100644 include/odp/api/packet_io_stats.h > > diff --git a/include/odp/api/packet_io_stats.h > b/include/odp/api/packet_io_stats.h > new file mode 100644 > index 0000000..94af18c > --- /dev/null > +++ b/include/odp/api/packet_io_stats.h > @@ -0,0 +1,166 @@ > +/* Copyright (c) 2015, Linaro Limited > + * All rights reserved. > + * > + * SPDX-License-Identifier: BSD-3-Clause > + */ > + > +/** > + * @file > + * > + * ODP Packet IO > + */ > + > +#ifndef ODP_API_PACKET_IO_STATS_H_ > +#define ODP_API_PACKET_IO_STATS_H_ > + > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +/** @defgroup odp_packet_io ODP PACKET IO > + * @{ > + */ > + > +/** > + * Packet IO statistics > + * > + * Packet IO statictics counters follow RFCs for Management Information > Base > + * (MIB)for use with network management protocols in the Internet > community: > + * https://tools.ietf.org/html/rfc3635 > + * https://tools.ietf.org/html/rfc2863 > + * https://tools.ietf.org/html/rfc2819 > + */ > +typedef struct odp_pktio_stats_t { > + /** > + * The number of octets in valid MAC frames received on this > interface, > + * including the MAC header and FCS. See ifHCInOctets counter > + * description in RFC 3635 for details. > + */ > + uint64_t in_octets; > + /** > + * The number of packets, delivered by this sub-layer to a higher > + * (sub-)layer, which were not addressed to a multicast or > broadcast > + * address at this sub-layer. See InUcastPkts in RFC 2863. > + */ > + uint64_t in_ucast_pkts; > + /** > + * The number of inbound packets which were chosen to be discarded > + * even though no errors had been detected to preven their being > + * deliverable to a higher-layer protocol. One possible reason for > + * discarding such a packet could be to free up buffer space. > + * See InDiscards in RFC 2863. > + */ > + uint64_t in_discards; > + /** > + * The sum for this interface of AlignmentErrors, FCSErrors, > FrameTooLongs, > + * InternalMacReceiveErrors. See InErrors in RFC 3635. > + */ > + uint64_t in_errors; > + /** > + * For packet-oriented interfaces, the number of packets received > via > + * the interface which were discarded because of an unknown or > + * unsupported protocol. For character-oriented or fixed-length > + * interfaces that support protocol multiplexing the number of > + * transmission units received via the interface which were > discarded > + * because of an unknown or unsupported protocol. For any > interface > + * that does not support protocol multiplexing, this counter will > always > + * be 0. See InUnknownProtos in RFC 2863. > + */ > + uint64_t in_unknown_protos; > + /** > + * The number of octets transmitted in valid MAC frames on this > + * interface, including the MAC header and FCS. This does include > + * the number of octets in valid MAC Control frames transmitted on > + * this interface. See OutOctets in RFC 3635. > + */ > + uint64_t out_octets; > + /** > + * The total number of packets that higher-level protocols > requested > + * be transmitted, and which were not addressed to a multicast or > + * broadcast address at this sub-layer, including those that were > + * discarded or not sent. does not include MAC Control frames. > + * See OutUcastPkts in RFC 2863, 3635. > + */ > + uint64_t out_ucast_pkts; > + /** > + * The number of outbound packets which were chosen to be discarded > + * even though no errors had been detected to prevent their being > + * transmitted. One possible reason for discarding such a packet > could > + * be to free up buffer space. See OutDiscards in RFC 2863. > + */ > + uint64_t out_discards; > + /** > + * The sum for this interface of SQETestErrors, LateCollisions, > + * ExcessiveCollisions, InternalMacTransmitErrors and > + * CarrierSenseErrors. See OutErrors in RFC 3635. > + */ > + uint64_t out_errors; > +} odp_pktio_stats_t; > + > +/** Bit in odp_pktio_stats_mask_t defines if in_octets counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_IN_OCTETS (1 << 0) > +/** Bit in odp_pktio_stats_mask_t defines if in_ucast_pkts counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_IN_UCAST_PKTS (1 << 1) > +/** Bit in odp_pktio_stats_mask_t defines if in_discards counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_IN_DISCARDS (1 << 2) > +/** Bit in odp_pktio_stats_mask_t defines if in_errors counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_IN_ERRORS (1 << 3) > +/** Bit in odp_pktio_stats_mask_t defines if in_unknown_protos counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_IN_UPROTOS (1 << 4) > +/** Bit in odp_pktio_stats_mask_t defines if out_octets counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_OUT_OCTETS (1 << 5) > +/** Bit in odp_pktio_stats_mask_t defines if out_ucast_pkts counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_OUT_UCAST_PKTS (1 << 6) > +/** Bit in odp_pktio_stats_mask_t defines if out_discards counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_OUT_DISCARDS (1 << 7) > +/** Bit in odp_pktio_stats_mask_t defines if out_errors counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_OUT_ERRORS (1 << 8) > These definitions should be explicitly defined to mirror the order in which the corresponding fields occur in the stats struct. They do mirror the order now but it is not defined that it must be so. The reason is that we might add fields to the stats struct and want to preserve some binary compatibility (think e.g. separately maintained drivers that provide these counters to the caller). + > +/** > + * Mask type of supported counters by platform, defined as: > + * ODP_PKTIO_STATS_xxxx > + */ > +typedef uint32_t odp_pktio_stats_mask_t; > + > +/** > + * Get statistics for pktio handle > + * > + * @param pktio Packet IO handle > + * @param[out] *stats Output buffer for counters > + * @param[out] *mask Output buffer for counters supported mask > + * @retval 0 on success > + * @retval <0 on failure > + */ > +int odp_pktio_stats(odp_pktio_t pktio, > + odp_pktio_stats_t *stats, > Mask should be a field in the stats structure, not a separate parameter. > + odp_pktio_stats_mask_t *mask); > + > +/** > + * Reset statistics for pktio handle > + * > + * @param pktio Packet IO handle > + * @param mask Counters mask to reset > + * @retval 0 on success > + * @retval <0 on failure > + * > + */ > +int odp_pktio_stats_reset(odp_pktio_t pktio, odp_pktio_stats_mask_t mask); > + > +/** > + * @} > + */ > + > +#ifdef __cplusplus > +} > +#endif > + > +#endif > diff --git a/platform/linux-generic/include/odp/packet_io.h > b/platform/linux-generic/include/odp/packet_io.h > index 1d690f5..18f8e78 100644 > --- a/platform/linux-generic/include/odp/packet_io.h > +++ b/platform/linux-generic/include/odp/packet_io.h > @@ -33,6 +33,7 @@ extern "C" { > */ > > #include <odp/api/packet_io.h> > +#include <odp/api/packet_io_stats.h> > > #ifdef __cplusplus > } > -- > 1.9.1 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On 10/19/2015 18:49, Ola Liljedahl wrote: > On 16 October 2015 at 15:25, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> > --- > include/odp/api/packet_io_stats.h | 166 > +++++++++++++++++++++++++ > platform/linux-generic/include/odp/packet_io.h | 1 + > 2 files changed, 167 insertions(+) > create mode 100644 include/odp/api/packet_io_stats.h > > diff --git a/include/odp/api/packet_io_stats.h > b/include/odp/api/packet_io_stats.h > new file mode 100644 > index 0000000..94af18c > --- /dev/null > +++ b/include/odp/api/packet_io_stats.h > @@ -0,0 +1,166 @@ > +/* Copyright (c) 2015, Linaro Limited > + * All rights reserved. > + * > + * SPDX-License-Identifier: BSD-3-Clause > + */ > + > +/** > + * @file > + * > + * ODP Packet IO > + */ > + > +#ifndef ODP_API_PACKET_IO_STATS_H_ > +#define ODP_API_PACKET_IO_STATS_H_ > + > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +/** @defgroup odp_packet_io ODP PACKET IO > + * @{ > + */ > + > +/** > + * Packet IO statistics > + * > + * Packet IO statictics counters follow RFCs for Management > Information Base > + * (MIB)for use with network management protocols in the Internet > community: > + * https://tools.ietf.org/html/rfc3635 > + * https://tools.ietf.org/html/rfc2863 > + * https://tools.ietf.org/html/rfc2819 > + */ > +typedef struct odp_pktio_stats_t { > + /** > + * The number of octets in valid MAC frames received on > this interface, > + * including the MAC header and FCS. See ifHCInOctets counter > + * description in RFC 3635 for details. > + */ > + uint64_t in_octets; > + /** > + * The number of packets, delivered by this sub-layer to a > higher > + * (sub-)layer, which were not addressed to a multicast or > broadcast > + * address at this sub-layer. See InUcastPkts in RFC 2863. > + */ > + uint64_t in_ucast_pkts; > + /** > + * The number of inbound packets which were chosen to be > discarded > + * even though no errors had been detected to preven their > being > + * deliverable to a higher-layer protocol. One possible > reason for > + * discarding such a packet could be to free up buffer space. > + * See InDiscards in RFC 2863. > + */ > + uint64_t in_discards; > + /** > + * The sum for this interface of AlignmentErrors, > FCSErrors, FrameTooLongs, > + * InternalMacReceiveErrors. See InErrors in RFC 3635. > + */ > + uint64_t in_errors; > + /** > + * For packet-oriented interfaces, the number of packets > received via > + * the interface which were discarded because of an unknown or > + * unsupported protocol. For character-oriented or > fixed-length > + * interfaces that support protocol multiplexing the number of > + * transmission units received via the interface which > were discarded > + * because of an unknown or unsupported protocol. For any > interface > + * that does not support protocol multiplexing, this > counter will always > + * be 0. See InUnknownProtos in RFC 2863. > + */ > + uint64_t in_unknown_protos; > + /** > + * The number of octets transmitted in valid MAC frames on > this > + * interface, including the MAC header and FCS. This does > include > + * the number of octets in valid MAC Control frames > transmitted on > + * this interface. See OutOctets in RFC 3635. > + */ > + uint64_t out_octets; > + /** > + * The total number of packets that higher-level protocols > requested > + * be transmitted, and which were not addressed to a > multicast or > + * broadcast address at this sub-layer, including those > that were > + * discarded or not sent. does not include MAC Control frames. > + * See OutUcastPkts in RFC 2863, 3635. > + */ > + uint64_t out_ucast_pkts; > + /** > + * The number of outbound packets which were chosen to be > discarded > + * even though no errors had been detected to prevent > their being > + * transmitted. One possible reason for discarding such a > packet could > + * be to free up buffer space. See OutDiscards in RFC 2863. > + */ > + uint64_t out_discards; > + /** > + * The sum for this interface of SQETestErrors, > LateCollisions, > + * ExcessiveCollisions, InternalMacTransmitErrors and > + * CarrierSenseErrors. See OutErrors in RFC 3635. > + */ > + uint64_t out_errors; > +} odp_pktio_stats_t; > + > +/** Bit in odp_pktio_stats_mask_t defines if in_octets counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_IN_OCTETS (1 << 0) > +/** Bit in odp_pktio_stats_mask_t defines if in_ucast_pkts counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_IN_UCAST_PKTS (1 << 1) > +/** Bit in odp_pktio_stats_mask_t defines if in_discards counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_IN_DISCARDS (1 << 2) > +/** Bit in odp_pktio_stats_mask_t defines if in_errors counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_IN_ERRORS (1 << 3) > +/** Bit in odp_pktio_stats_mask_t defines if in_unknown_protos > counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_IN_UPROTOS (1 << 4) > +/** Bit in odp_pktio_stats_mask_t defines if out_octets counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_OUT_OCTETS (1 << 5) > +/** Bit in odp_pktio_stats_mask_t defines if out_ucast_pkts counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_OUT_UCAST_PKTS (1 << 6) > +/** Bit in odp_pktio_stats_mask_t defines if out_discards counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_OUT_DISCARDS (1 << 7) > +/** Bit in odp_pktio_stats_mask_t defines if out_errors counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_OUT_ERRORS (1 << 8) > > These definitions should be explicitly defined to mirror the order in > which the corresponding fields occur in the stats struct. They do > mirror the order now but it is not defined that it must be so. The > reason is that we might add fields to the stats struct and want to > preserve some binary compatibility (think e.g. separately maintained > drivers that provide these counters to the caller). > Ola, how to solve that? I added comment about 2 lines bellow the line I'm writing. Do you think we need something else? > + > +/** > + * Mask type of supported counters by platform, defined as: > + * ODP_PKTIO_STATS_xxxx > + */ > +typedef uint32_t odp_pktio_stats_mask_t; > + > +/** > + * Get statistics for pktio handle > + * > + * @param pktio Packet IO handle > + * @param[out] *stats Output buffer for counters > + * @param[out] *mask Output buffer for counters supported mask > + * @retval 0 on success > + * @retval <0 on failure > + */ > +int odp_pktio_stats(odp_pktio_t pktio, > + odp_pktio_stats_t *stats, > > Mask should be a field in the stats structure, not a separate parameter. > ok, will do. Maxim. > > + odp_pktio_stats_mask_t *mask); > + > +/** > + * Reset statistics for pktio handle > + * > + * @param pktio Packet IO handle > + * @param mask Counters mask to reset > + * @retval 0 on success > + * @retval <0 on failure > + * > + */ > +int odp_pktio_stats_reset(odp_pktio_t pktio, > odp_pktio_stats_mask_t mask); > + > +/** > + * @} > + */ > + > +#ifdef __cplusplus > +} > +#endif > + > +#endif > diff --git a/platform/linux-generic/include/odp/packet_io.h > b/platform/linux-generic/include/odp/packet_io.h > index 1d690f5..18f8e78 100644 > --- a/platform/linux-generic/include/odp/packet_io.h > +++ b/platform/linux-generic/include/odp/packet_io.h > @@ -33,6 +33,7 @@ extern "C" { > */ > > #include <odp/api/packet_io.h> > +#include <odp/api/packet_io_stats.h> > > #ifdef __cplusplus > } > -- > 1.9.1 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > https://lists.linaro.org/mailman/listinfo/lng-odp > >
On 19 October 2015 at 17:58, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 10/19/2015 18:49, Ola Liljedahl wrote: > >> On 16 October 2015 at 15:25, Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> wrote: >> >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> >> --- >> include/odp/api/packet_io_stats.h | 166 >> +++++++++++++++++++++++++ >> platform/linux-generic/include/odp/packet_io.h | 1 + >> 2 files changed, 167 insertions(+) >> create mode 100644 include/odp/api/packet_io_stats.h >> >> diff --git a/include/odp/api/packet_io_stats.h >> b/include/odp/api/packet_io_stats.h >> new file mode 100644 >> index 0000000..94af18c >> --- /dev/null >> +++ b/include/odp/api/packet_io_stats.h >> @@ -0,0 +1,166 @@ >> +/* Copyright (c) 2015, Linaro Limited >> + * All rights reserved. >> + * >> + * SPDX-License-Identifier: BSD-3-Clause >> + */ >> + >> +/** >> + * @file >> + * >> + * ODP Packet IO >> + */ >> + >> +#ifndef ODP_API_PACKET_IO_STATS_H_ >> +#define ODP_API_PACKET_IO_STATS_H_ >> + >> +#ifdef __cplusplus >> +extern "C" { >> +#endif >> + >> +/** @defgroup odp_packet_io ODP PACKET IO >> + * @{ >> + */ >> + >> +/** >> + * Packet IO statistics >> + * >> + * Packet IO statictics counters follow RFCs for Management >> Information Base >> + * (MIB)for use with network management protocols in the Internet >> community: >> + * https://tools.ietf.org/html/rfc3635 >> + * https://tools.ietf.org/html/rfc2863 >> + * https://tools.ietf.org/html/rfc2819 >> + */ >> +typedef struct odp_pktio_stats_t { >> + /** >> + * The number of octets in valid MAC frames received on >> this interface, >> + * including the MAC header and FCS. See ifHCInOctets counter >> + * description in RFC 3635 for details. >> + */ >> + uint64_t in_octets; >> + /** >> + * The number of packets, delivered by this sub-layer to a >> higher >> + * (sub-)layer, which were not addressed to a multicast or >> broadcast >> + * address at this sub-layer. See InUcastPkts in RFC 2863. >> + */ >> + uint64_t in_ucast_pkts; >> + /** >> + * The number of inbound packets which were chosen to be >> discarded >> + * even though no errors had been detected to preven their >> being >> + * deliverable to a higher-layer protocol. One possible >> reason for >> + * discarding such a packet could be to free up buffer space. >> + * See InDiscards in RFC 2863. >> + */ >> + uint64_t in_discards; >> + /** >> + * The sum for this interface of AlignmentErrors, >> FCSErrors, FrameTooLongs, >> + * InternalMacReceiveErrors. See InErrors in RFC 3635. >> + */ >> + uint64_t in_errors; >> + /** >> + * For packet-oriented interfaces, the number of packets >> received via >> + * the interface which were discarded because of an unknown or >> + * unsupported protocol. For character-oriented or >> fixed-length >> + * interfaces that support protocol multiplexing the number of >> + * transmission units received via the interface which >> were discarded >> + * because of an unknown or unsupported protocol. For any >> interface >> + * that does not support protocol multiplexing, this >> counter will always >> + * be 0. See InUnknownProtos in RFC 2863. >> + */ >> + uint64_t in_unknown_protos; >> + /** >> + * The number of octets transmitted in valid MAC frames on >> this >> + * interface, including the MAC header and FCS. This does >> include >> + * the number of octets in valid MAC Control frames >> transmitted on >> + * this interface. See OutOctets in RFC 3635. >> + */ >> + uint64_t out_octets; >> + /** >> + * The total number of packets that higher-level protocols >> requested >> + * be transmitted, and which were not addressed to a >> multicast or >> + * broadcast address at this sub-layer, including those >> that were >> + * discarded or not sent. does not include MAC Control frames. >> + * See OutUcastPkts in RFC 2863, 3635. >> + */ >> + uint64_t out_ucast_pkts; >> + /** >> + * The number of outbound packets which were chosen to be >> discarded >> + * even though no errors had been detected to prevent >> their being >> + * transmitted. One possible reason for discarding such a >> packet could >> + * be to free up buffer space. See OutDiscards in RFC 2863. >> + */ >> + uint64_t out_discards; >> + /** >> + * The sum for this interface of SQETestErrors, >> LateCollisions, >> + * ExcessiveCollisions, InternalMacTransmitErrors and >> + * CarrierSenseErrors. See OutErrors in RFC 3635. >> + */ >> + uint64_t out_errors; >> +} odp_pktio_stats_t; >> + >> +/** Bit in odp_pktio_stats_mask_t defines if in_octets counter >> + * of odp_pktio_stats_t struct is supported */ >> +#define ODP_PKTIO_STATS_IN_OCTETS (1 << 0) >> +/** Bit in odp_pktio_stats_mask_t defines if in_ucast_pkts counter >> + * of odp_pktio_stats_t struct is supported */ >> +#define ODP_PKTIO_STATS_IN_UCAST_PKTS (1 << 1) >> +/** Bit in odp_pktio_stats_mask_t defines if in_discards counter >> + * of odp_pktio_stats_t struct is supported */ >> +#define ODP_PKTIO_STATS_IN_DISCARDS (1 << 2) >> +/** Bit in odp_pktio_stats_mask_t defines if in_errors counter >> + * of odp_pktio_stats_t struct is supported */ >> +#define ODP_PKTIO_STATS_IN_ERRORS (1 << 3) >> +/** Bit in odp_pktio_stats_mask_t defines if in_unknown_protos >> counter >> + * of odp_pktio_stats_t struct is supported */ >> +#define ODP_PKTIO_STATS_IN_UPROTOS (1 << 4) >> +/** Bit in odp_pktio_stats_mask_t defines if out_octets counter >> + * of odp_pktio_stats_t struct is supported */ >> +#define ODP_PKTIO_STATS_OUT_OCTETS (1 << 5) >> +/** Bit in odp_pktio_stats_mask_t defines if out_ucast_pkts counter >> + * of odp_pktio_stats_t struct is supported */ >> +#define ODP_PKTIO_STATS_OUT_UCAST_PKTS (1 << 6) >> +/** Bit in odp_pktio_stats_mask_t defines if out_discards counter >> + * of odp_pktio_stats_t struct is supported */ >> +#define ODP_PKTIO_STATS_OUT_DISCARDS (1 << 7) >> +/** Bit in odp_pktio_stats_mask_t defines if out_errors counter >> + * of odp_pktio_stats_t struct is supported */ >> +#define ODP_PKTIO_STATS_OUT_ERRORS (1 << 8) >> >> These definitions should be explicitly defined to mirror the order in >> which the corresponding fields occur in the stats struct. They do mirror >> the order now but it is not defined that it must be so. The reason is that >> we might add fields to the stats struct and want to preserve some binary >> compatibility (think e.g. separately maintained drivers that provide these >> counters to the caller). >> >> > Ola, how to solve that? I added comment about 2 lines bellow the line I'm > writing. Do you think we need something else? > Just add to the comment that the numbering of the mask bits must mirror the ordering of the fields in the stats structure and that new fields should be added to the end of the structure (I have seen people add new fields in the middle of structs, structs that were part of user/kernel space interfaces and thus must be backwards and forwards compatible... the horror). > > + >> +/** >> + * Mask type of supported counters by platform, defined as: >> + * ODP_PKTIO_STATS_xxxx >> + */ >> +typedef uint32_t odp_pktio_stats_mask_t; >> + >> +/** >> + * Get statistics for pktio handle >> + * >> + * @param pktio Packet IO handle >> + * @param[out] *stats Output buffer for counters >> + * @param[out] *mask Output buffer for counters supported mask >> + * @retval 0 on success >> + * @retval <0 on failure >> + */ >> +int odp_pktio_stats(odp_pktio_t pktio, >> + odp_pktio_stats_t *stats, >> >> Mask should be a field in the stats structure, not a separate parameter. >> >> ok, will do. > > Maxim. > >> >> + odp_pktio_stats_mask_t *mask); >> + >> +/** >> + * Reset statistics for pktio handle >> + * >> + * @param pktio Packet IO handle >> + * @param mask Counters mask to reset >> + * @retval 0 on success >> + * @retval <0 on failure >> + * >> + */ >> +int odp_pktio_stats_reset(odp_pktio_t pktio, >> odp_pktio_stats_mask_t mask); >> + >> +/** >> + * @} >> + */ >> + >> +#ifdef __cplusplus >> +} >> +#endif >> + >> +#endif >> diff --git a/platform/linux-generic/include/odp/packet_io.h >> b/platform/linux-generic/include/odp/packet_io.h >> index 1d690f5..18f8e78 100644 >> --- a/platform/linux-generic/include/odp/packet_io.h >> +++ b/platform/linux-generic/include/odp/packet_io.h >> @@ -33,6 +33,7 @@ extern "C" { >> */ >> >> #include <odp/api/packet_io.h> >> +#include <odp/api/packet_io_stats.h> >> >> #ifdef __cplusplus >> } >> -- >> 1.9.1 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >
+
+/** Bit in odp_pktio_stats_mask_t defines if in_octets counter
+ * of odp_pktio_stats_t struct is supported */
+#define ODP_PKTIO_STATS_IN_OCTETS (1 << 0)
+/** Bit in odp_pktio_stats_mask_t defines if in_ucast_pkts counter
+ * of odp_pktio_stats_t struct is supported */
+#define ODP_PKTIO_STATS_IN_UCAST_PKTS (1 << 1)
+/** Bit in odp_pktio_stats_mask_t defines if in_discards counter
+ * of odp_pktio_stats_t struct is supported */
+#define ODP_PKTIO_STATS_IN_DISCARDS (1 << 2)
+/** Bit in odp_pktio_stats_mask_t defines if in_errors counter
+ * of odp_pktio_stats_t struct is supported */
+#define ODP_PKTIO_STATS_IN_ERRORS (1 << 3)
+/** Bit in odp_pktio_stats_mask_t defines if in_unknown_protos
counter
+ * of odp_pktio_stats_t struct is supported */
+#define ODP_PKTIO_STATS_IN_UPROTOS (1 << 4)
+/** Bit in odp_pktio_stats_mask_t defines if out_octets counter
+ * of odp_pktio_stats_t struct is supported */
+#define ODP_PKTIO_STATS_OUT_OCTETS (1 << 5)
+/** Bit in odp_pktio_stats_mask_t defines if out_ucast_pkts counter
+ * of odp_pktio_stats_t struct is supported */
+#define ODP_PKTIO_STATS_OUT_UCAST_PKTS (1 << 6)
+/** Bit in odp_pktio_stats_mask_t defines if out_discards counter
+ * of odp_pktio_stats_t struct is supported */
+#define ODP_PKTIO_STATS_OUT_DISCARDS (1 << 7)
+/** Bit in odp_pktio_stats_mask_t defines if out_errors counter
+ * of odp_pktio_stats_t struct is supported */
+#define ODP_PKTIO_STATS_OUT_ERRORS (1 << 8)
These definitions should be explicitly defined to mirror the order in which the corresponding fields occur in the stats struct. They do mirror the order now but it is not defined that it must be so. The reason is that we might add fields to the stats struct and want to preserve some binary compatibility (think e.g. separately maintained drivers that provide these counters to the caller).
Ola, how to solve that? I added comment about 2 lines bellow the line I'm writing. Do you think we need something else?
Just add to the comment that the numbering of the mask bits must mirror the ordering of the fields in the stats structure and that new fields should be added to the end of the structure (I have seen people add new fields in the middle of structs, structs that were part of user/kernel space interfaces and thus must be backwards and forwards compatible... the horror).
It’s better to define another type (bit-field struct) for this and leave out the #defines.
typedef struct {
union {
uint32_t all;
uint32_t in_octets:1;
uint32_t in_ucast_pkts:1;
…
uint32_t reserved:15;
}
} odp_pktio_stat_capability_t;
+
+/**
+ * Mask type of supported counters by platform, defined as:
+ * ODP_PKTIO_STATS_xxxx
+ */
+typedef uint32_t odp_pktio_stats_mask_t;
+
+/**
+ * Get statistics for pktio handle
+ *
+ * @param pktio Packet IO handle
+ * @param[out] *stats Output buffer for counters
+ * @param[out] *mask Output buffer for counters supported mask
+ * @retval 0 on success
+ * @retval <0 on failure
+ */
+int odp_pktio_stats(odp_pktio_t pktio,
+ odp_pktio_stats_t *stats,
Mask should be a field in the stats structure, not a separate parameter.
ok, will do.
Capability query should be another API call, since capability will not change between statistics calls.
// Get stats
int odp_pktio_stats(odp_pktio_t pktio, odp_pktio_stats_t *stats);
// Get stats capability
int odp_pktio_stats_capability(odp_pktio_t pktio, odp_pktio_stats_capability_t *capability);
// Check that everything is supported
// 1: all is supported, 0: some stats missing
int odp_pktio_stats_capability_all(odp_pktio_stats_capability_t *capability);
-Petri
Maxim.
+ odp_pktio_stats_mask_t *mask);
+
+/**
+ * Reset statistics for pktio handle
+ *
+ * @param pktio Packet IO handle
+ * @param mask Counters mask to reset
+ * @retval 0 on success
+ * @retval <0 on failure
+ *
+ */
+int odp_pktio_stats_reset(odp_pktio_t pktio,
odp_pktio_stats_mask_t mask);
+
+/**
+ * @}
+ */
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif
diff --git a/platform/linux-generic/include/odp/packet_io.h
b/platform/linux-generic/include/odp/packet_io.h
index 1d690f5..18f8e78 100644
--- a/platform/linux-generic/include/odp/packet_io.h
+++ b/platform/linux-generic/include/odp/packet_io.h
@@ -33,6 +33,7 @@ extern "C" {
*/
#include <odp/api/packet_io.h>
+#include <odp/api/packet_io_stats.h>
#ifdef __cplusplus
}
--
1.9.1
_______________________________________________
lng-odp mailing list
lng-odp@lists.linaro.org<mailto:lng-odp@lists.linaro.org> <mailto:lng-odp@lists.linaro.org<mailto:lng-odp@lists.linaro.org>>
https://lists.linaro.org/mailman/listinfo/lng-odp
On 20 October 2015 at 10:09, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolainen@nokia.com> wrote: > + > +/** Bit in odp_pktio_stats_mask_t defines if in_octets counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_IN_OCTETS (1 << 0) > +/** Bit in odp_pktio_stats_mask_t defines if in_ucast_pkts counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_IN_UCAST_PKTS (1 << 1) > +/** Bit in odp_pktio_stats_mask_t defines if in_discards counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_IN_DISCARDS (1 << 2) > +/** Bit in odp_pktio_stats_mask_t defines if in_errors counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_IN_ERRORS (1 << 3) > +/** Bit in odp_pktio_stats_mask_t defines if in_unknown_protos > counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_IN_UPROTOS (1 << 4) > +/** Bit in odp_pktio_stats_mask_t defines if out_octets counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_OUT_OCTETS (1 << 5) > +/** Bit in odp_pktio_stats_mask_t defines if out_ucast_pkts counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_OUT_UCAST_PKTS (1 << 6) > +/** Bit in odp_pktio_stats_mask_t defines if out_discards counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_OUT_DISCARDS (1 << 7) > +/** Bit in odp_pktio_stats_mask_t defines if out_errors counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_OUT_ERRORS (1 << 8) > > These definitions should be explicitly defined to mirror the order in > which the corresponding fields occur in the stats struct. They do mirror > the order now but it is not defined that it must be so. The reason is that > we might add fields to the stats struct and want to preserve some binary > compatibility (think e.g. separately maintained drivers that provide these > counters to the caller). > > > Ola, how to solve that? I added comment about 2 lines bellow the line I'm > writing. Do you think we need something else? > > Just add to the comment that the numbering of the mask bits must mirror > the ordering of the fields in the stats structure and that new fields > should be added to the end of the structure (I have seen people add new > fields in the middle of structs, structs that were part of user/kernel > space interfaces and thus must be backwards and forwards compatible... the > horror). > > > > > > It’s better to define another type (bit-field struct) for this and leave > out the #defines. > > > > typedef struct { > > > > union { > > uint32_t all; > > > > uint32_t in_octets:1; > > uint32_t in_ucast_pkts:1; > > … > > uint32_t reserved:15; > > } > > } odp_pktio_stat_capability_t; > > > > > > > > + > +/** > + * Mask type of supported counters by platform, defined as: > + * ODP_PKTIO_STATS_xxxx > + */ > +typedef uint32_t odp_pktio_stats_mask_t; > + > +/** > + * Get statistics for pktio handle > + * > + * @param pktio Packet IO handle > + * @param[out] *stats Output buffer for counters > + * @param[out] *mask Output buffer for counters supported mask > + * @retval 0 on success > + * @retval <0 on failure > + */ > +int odp_pktio_stats(odp_pktio_t pktio, > + odp_pktio_stats_t *stats, > > Mask should be a field in the stats structure, not a separate parameter. > > ok, will do. > > > > Capability query should be another API call, since capability will not > change between statistics calls. > > Yes but now you separate the stats counters from the mask or capabilities which tells you which stats counters are actually valid This is exactly what I wanted to avoid. And the API is more complicated, three calls instead of one. What did we gain? Nothing I think. But we lost the simplicity. > > // Get stats > > int odp_pktio_stats(odp_pktio_t pktio, odp_pktio_stats_t *stats); > > > > // Get stats capability > > int odp_pktio_stats_capability(odp_pktio_t pktio, > odp_pktio_stats_capability_t *capability); > > > > // Check that everything is supported > > // 1: all is supported, 0: some stats missing > > int odp_pktio_stats_capability_all(odp_pktio_stats_capability_t > *capability); > > And what is the actual use case of this function? > > > -Petri > > > > Maxim. > > > + odp_pktio_stats_mask_t *mask); > + > +/** > + * Reset statistics for pktio handle > + * > + * @param pktio Packet IO handle > + * @param mask Counters mask to reset > + * @retval 0 on success > + * @retval <0 on failure > + * > + */ > +int odp_pktio_stats_reset(odp_pktio_t pktio, > odp_pktio_stats_mask_t mask); > + > +/** > + * @} > + */ > + > +#ifdef __cplusplus > +} > +#endif > + > +#endif > diff --git a/platform/linux-generic/include/odp/packet_io.h > b/platform/linux-generic/include/odp/packet_io.h > index 1d690f5..18f8e78 100644 > --- a/platform/linux-generic/include/odp/packet_io.h > +++ b/platform/linux-generic/include/odp/packet_io.h > @@ -33,6 +33,7 @@ extern "C" { > */ > > #include <odp/api/packet_io.h> > +#include <odp/api/packet_io_stats.h> > > #ifdef __cplusplus > } > -- > 1.9.1 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > https://lists.linaro.org/mailman/listinfo/lng-odp > > > > >
From: EXT Ola Liljedahl [mailto:ola.liljedahl@linaro.org]
Sent: Tuesday, October 20, 2015 11:23 AM
To: Savolainen, Petri (Nokia - FI/Espoo)
Cc: Maxim Uvarov; LNG ODP Mailman List
Subject: Re: [lng-odp] [API-NEXT PATCHv4 1/2] api: define pktio statistics api
On 20 October 2015 at 10:09, Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com<mailto:petri.savolainen@nokia.com>> wrote:
+
+/** Bit in odp_pktio_stats_mask_t defines if in_octets counter
+ * of odp_pktio_stats_t struct is supported */
+#define ODP_PKTIO_STATS_IN_OCTETS (1 << 0)
+/** Bit in odp_pktio_stats_mask_t defines if in_ucast_pkts counter
+ * of odp_pktio_stats_t struct is supported */
+#define ODP_PKTIO_STATS_IN_UCAST_PKTS (1 << 1)
+/** Bit in odp_pktio_stats_mask_t defines if in_discards counter
+ * of odp_pktio_stats_t struct is supported */
+#define ODP_PKTIO_STATS_IN_DISCARDS (1 << 2)
+/** Bit in odp_pktio_stats_mask_t defines if in_errors counter
+ * of odp_pktio_stats_t struct is supported */
+#define ODP_PKTIO_STATS_IN_ERRORS (1 << 3)
+/** Bit in odp_pktio_stats_mask_t defines if in_unknown_protos
counter
+ * of odp_pktio_stats_t struct is supported */
+#define ODP_PKTIO_STATS_IN_UPROTOS (1 << 4)
+/** Bit in odp_pktio_stats_mask_t defines if out_octets counter
+ * of odp_pktio_stats_t struct is supported */
+#define ODP_PKTIO_STATS_OUT_OCTETS (1 << 5)
+/** Bit in odp_pktio_stats_mask_t defines if out_ucast_pkts counter
+ * of odp_pktio_stats_t struct is supported */
+#define ODP_PKTIO_STATS_OUT_UCAST_PKTS (1 << 6)
+/** Bit in odp_pktio_stats_mask_t defines if out_discards counter
+ * of odp_pktio_stats_t struct is supported */
+#define ODP_PKTIO_STATS_OUT_DISCARDS (1 << 7)
+/** Bit in odp_pktio_stats_mask_t defines if out_errors counter
+ * of odp_pktio_stats_t struct is supported */
+#define ODP_PKTIO_STATS_OUT_ERRORS (1 << 8)
These definitions should be explicitly defined to mirror the order in which the corresponding fields occur in the stats struct. They do mirror the order now but it is not defined that it must be so. The reason is that we might add fields to the stats struct and want to preserve some binary compatibility (think e.g. separately maintained drivers that provide these counters to the caller).
Ola, how to solve that? I added comment about 2 lines bellow the line I'm writing. Do you think we need something else?
Just add to the comment that the numbering of the mask bits must mirror the ordering of the fields in the stats structure and that new fields should be added to the end of the structure (I have seen people add new fields in the middle of structs, structs that were part of user/kernel space interfaces and thus must be backwards and forwards compatible... the horror).
It’s better to define another type (bit-field struct) for this and leave out the #defines.
typedef struct {
union {
uint32_t all;
uint32_t in_octets:1;
uint32_t in_ucast_pkts:1;
…
uint32_t reserved:15;
}
} odp_pktio_stat_capability_t;
+
+/**
+ * Mask type of supported counters by platform, defined as:
+ * ODP_PKTIO_STATS_xxxx
+ */
+typedef uint32_t odp_pktio_stats_mask_t;
+
+/**
+ * Get statistics for pktio handle
+ *
+ * @param pktio Packet IO handle
+ * @param[out] *stats Output buffer for counters
+ * @param[out] *mask Output buffer for counters supported mask
+ * @retval 0 on success
+ * @retval <0 on failure
+ */
+int odp_pktio_stats(odp_pktio_t pktio,
+ odp_pktio_stats_t *stats,
Mask should be a field in the stats structure, not a separate parameter.
ok, will do.
Capability query should be another API call, since capability will not change between statistics calls.
Yes but now you separate the stats counters from the mask or capabilities which tells you which stats counters are actually valid This is exactly what I wanted to avoid.
And the API is more complicated, three calls instead of one. What did we gain? Nothing I think. But we lost the simplicity.
We gain that capabilities are not copied in each statistics are polled and user does not have check each time which counter are valid and which are not… HW counters will not appear or disappear between statistics calls => no need to copy around and check for it.
// Get stats
int odp_pktio_stats(odp_pktio_t pktio, odp_pktio_stats_t *stats);
// Get stats capability
int odp_pktio_stats_capability(odp_pktio_t pktio, odp_pktio_stats_capability_t *capability);
// Check that everything is supported
// 1: all is supported, 0: some stats missing
int odp_pktio_stats_capability_all(odp_pktio_stats_capability_t *capability);
And what is the actual use case of this function?
If everything is supported (the common case), user does not have to check any further. It’s also forward compatible way to check that everything is supported. Without this helper call, user needs to check every bit (or #define) individually and maintain the check code over API versions.
-Petri
On 20 October 2015 at 10:29, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolainen@nokia.com> wrote: > > > > > *From:* EXT Ola Liljedahl [mailto:ola.liljedahl@linaro.org] > *Sent:* Tuesday, October 20, 2015 11:23 AM > *To:* Savolainen, Petri (Nokia - FI/Espoo) > *Cc:* Maxim Uvarov; LNG ODP Mailman List > *Subject:* Re: [lng-odp] [API-NEXT PATCHv4 1/2] api: define pktio > statistics api > > > > On 20 October 2015 at 10:09, Savolainen, Petri (Nokia - FI/Espoo) < > petri.savolainen@nokia.com> wrote: > > + > +/** Bit in odp_pktio_stats_mask_t defines if in_octets counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_IN_OCTETS (1 << 0) > +/** Bit in odp_pktio_stats_mask_t defines if in_ucast_pkts counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_IN_UCAST_PKTS (1 << 1) > +/** Bit in odp_pktio_stats_mask_t defines if in_discards counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_IN_DISCARDS (1 << 2) > +/** Bit in odp_pktio_stats_mask_t defines if in_errors counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_IN_ERRORS (1 << 3) > +/** Bit in odp_pktio_stats_mask_t defines if in_unknown_protos > counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_IN_UPROTOS (1 << 4) > +/** Bit in odp_pktio_stats_mask_t defines if out_octets counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_OUT_OCTETS (1 << 5) > +/** Bit in odp_pktio_stats_mask_t defines if out_ucast_pkts counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_OUT_UCAST_PKTS (1 << 6) > +/** Bit in odp_pktio_stats_mask_t defines if out_discards counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_OUT_DISCARDS (1 << 7) > +/** Bit in odp_pktio_stats_mask_t defines if out_errors counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_OUT_ERRORS (1 << 8) > > These definitions should be explicitly defined to mirror the order in > which the corresponding fields occur in the stats struct. They do mirror > the order now but it is not defined that it must be so. The reason is that > we might add fields to the stats struct and want to preserve some binary > compatibility (think e.g. separately maintained drivers that provide these > counters to the caller). > > > Ola, how to solve that? I added comment about 2 lines bellow the line I'm > writing. Do you think we need something else? > > Just add to the comment that the numbering of the mask bits must mirror > the ordering of the fields in the stats structure and that new fields > should be added to the end of the structure (I have seen people add new > fields in the middle of structs, structs that were part of user/kernel > space interfaces and thus must be backwards and forwards compatible... the > horror). > > > > > > It’s better to define another type (bit-field struct) for this and leave > out the #defines. > > > > typedef struct { > > > > union { > > uint32_t all; > > > > uint32_t in_octets:1; > > uint32_t in_ucast_pkts:1; > > … > > uint32_t reserved:15; > > } > > } odp_pktio_stat_capability_t; > > > > > > > > + > +/** > + * Mask type of supported counters by platform, defined as: > + * ODP_PKTIO_STATS_xxxx > + */ > +typedef uint32_t odp_pktio_stats_mask_t; > + > +/** > + * Get statistics for pktio handle > + * > + * @param pktio Packet IO handle > + * @param[out] *stats Output buffer for counters > + * @param[out] *mask Output buffer for counters supported mask > + * @retval 0 on success > + * @retval <0 on failure > + */ > +int odp_pktio_stats(odp_pktio_t pktio, > + odp_pktio_stats_t *stats, > > Mask should be a field in the stats structure, not a separate parameter. > > ok, will do. > > > > Capability query should be another API call, since capability will not > change between statistics calls. > > Yes but now you separate the stats counters from the mask or capabilities > which tells you which stats counters are actually valid This is exactly > what I wanted to avoid. > > And the API is more complicated, three calls instead of one. What did we > gain? Nothing I think. But we lost the simplicity. > > > > We gain that capabilities are not copied in each statistics > A minuscule price I am ready to pay. > are polled and user does not have check each time which counter are valid > and which are not… > I think we should request that unsupported counters are cleared (set to zero). But of course the caller can do this before the call. So in many cases we might not even have to check if a counter is supported, unsupported counters will just print as zero (depending on what you do with the counters). > HW counters will not appear or disappear between statistics calls => no > need to copy around and check for it. > Different interfaces may support different counters so there is still a need for the application to maintain a capset per interface. Why not do this as part of the counters struct? > > > > > > // Get stats > > int odp_pktio_stats(odp_pktio_t pktio, odp_pktio_stats_t *stats); > > > > // Get stats capability > > int odp_pktio_stats_capability(odp_pktio_t pktio, > odp_pktio_stats_capability_t *capability); > > > > // Check that everything is supported > > // 1: all is supported, 0: some stats missing > > int odp_pktio_stats_capability_all(odp_pktio_stats_capability_t > *capability); > > And what is the actual use case of this function? > > > > If everything is supported (the common case), user does not have to check > any further. It’s also forward compatible way to check that everything is > supported. Without this helper call, user needs to check every bit (or > #define) individually and maintain the check code over API versions. > Now you are creating multiple control paths in the application. Someone could be using this function and only support the case that all stats counters are supported. Then running on some other platform, all counters will not be supported and another path through the code (that checks whether the individual counters used are supported) must be taken. This path might not exist (because it was not needed) or might not work correctly (because never tested). Just provide one way of doing this. Better the return the supported counters as a bitmask (uint32_t). The caller can check (e.g. using bit-wise and operation) whether (all of) the counters of interest are supported. > > -Petri > > > > > > > > >
On 10/20/2015 12:01, Ola Liljedahl wrote: > > > On 20 October 2015 at 10:29, Savolainen, Petri (Nokia - FI/Espoo) > <petri.savolainen@nokia.com <mailto:petri.savolainen@nokia.com>> wrote: > > *From:*EXT Ola Liljedahl [mailto:ola.liljedahl@linaro.org > <mailto:ola.liljedahl@linaro.org>] > *Sent:* Tuesday, October 20, 2015 11:23 AM > *To:* Savolainen, Petri (Nokia - FI/Espoo) > *Cc:* Maxim Uvarov; LNG ODP Mailman List > *Subject:* Re: [lng-odp] [API-NEXT PATCHv4 1/2] api: define pktio > statistics api > > On 20 October 2015 at 10:09, Savolainen, Petri (Nokia - FI/Espoo) > <petri.savolainen@nokia.com <mailto:petri.savolainen@nokia.com>> > wrote: > > + > +/** Bit in odp_pktio_stats_mask_t defines if in_octets counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_IN_OCTETS (1 << 0) > +/** Bit in odp_pktio_stats_mask_t defines if in_ucast_pkts > counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_IN_UCAST_PKTS (1 << 1) > +/** Bit in odp_pktio_stats_mask_t defines if in_discards counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_IN_DISCARDS (1 << 2) > +/** Bit in odp_pktio_stats_mask_t defines if in_errors counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_IN_ERRORS (1 << 3) > +/** Bit in odp_pktio_stats_mask_t defines if in_unknown_protos > counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_IN_UPROTOS (1 << 4) > +/** Bit in odp_pktio_stats_mask_t defines if out_octets counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_OUT_OCTETS (1 << 5) > +/** Bit in odp_pktio_stats_mask_t defines if out_ucast_pkts > counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_OUT_UCAST_PKTS (1 << 6) > +/** Bit in odp_pktio_stats_mask_t defines if out_discards counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_OUT_DISCARDS (1 << 7) > +/** Bit in odp_pktio_stats_mask_t defines if out_errors counter > + * of odp_pktio_stats_t struct is supported */ > +#define ODP_PKTIO_STATS_OUT_ERRORS (1 << 8) > > These definitions should be explicitly defined to mirror the order > in which the corresponding fields occur in the stats struct. They > do mirror the order now but it is not defined that it must be so. > The reason is that we might add fields to the stats struct and > want to preserve some binary compatibility (think e.g. separately > maintained drivers that provide these counters to the caller). > > > Ola, how to solve that? I added comment about 2 lines bellow the > line I'm writing. Do you think we need something else? > > Just add to the comment that the numbering of the mask bits must > mirror the ordering of the fields in the stats structure and that > new fields should be added to the end of the structure (I have > seen people add new fields in the middle of structs, structs that > were part of user/kernel space interfaces and thus must be > backwards and forwards compatible... the horror). > > It’s better to define another type (bit-field struct) for this and > leave out the #defines. > > typedef struct { > > union { > > uint32_t all; > > uint32_t in_octets:1; > > uint32_t in_ucast_pkts:1; > > … > > uint32_t reserved:15; > > } > > } odp_pktio_stat_capability_t; > > + > +/** > + * Mask type of supported counters by platform, > defined as: > + * ODP_PKTIO_STATS_xxxx > + */ > +typedef uint32_t odp_pktio_stats_mask_t; > + > +/** > + * Get statistics for pktio handle > + * > + * @param pktio Packet IO handle > + * @param[out] *stats Output buffer for counters > + * @param[out] *mask Output buffer for counters > supported mask > + * @retval 0 on success > + * @retval <0 on failure > + */ > +int odp_pktio_stats(odp_pktio_t pktio, > + odp_pktio_stats_t *stats, > > Mask should be a field in the stats structure, not a > separate parameter. > > ok, will do. > > Capability query should be another API call, since capability > will not change between statistics calls. > > Yes but now you separate the stats counters from the mask or > capabilities which tells you which stats counters are actually > valid This is exactly what I wanted to avoid. > > And the API is more complicated, three calls instead of one. What > did we gain? Nothing I think. But we lost the simplicity. > > We gain that capabilities are not copied in each statistics > > A minuscule price I am ready to pay. > > are polled and user does not have check each time which counter > are valid and which are not… > > I think we should request that unsupported counters are cleared (set > to zero). But of course the caller can do this before the call. So in > many cases we might not even have to check if a counter is supported, > unsupported counters will just print as zero (depending on what you do > with the counters). > > HW counters will not appear or disappear between statistics calls > => no need to copy around and check for it. > > Different interfaces may support different counters so there is still > a need for the application to maintain a capset per interface. Why not > do this as part of the counters struct? > > // Get stats > > int odp_pktio_stats(odp_pktio_t pktio, odp_pktio_stats_t > *stats); > > // Get stats capability > > int odp_pktio_stats_capability(odp_pktio_t pktio, > odp_pktio_stats_capability_t *capability); > > // Check that everything is supported > > // 1: all is supported, 0: some stats missing > > int > odp_pktio_stats_capability_all(odp_pktio_stats_capability_t *capability); > > And what is the actual use case of this function? > > If everything is supported (the common case), user does not have > to check any further. It’s also forward compatible way to check > that everything is supported. Without this helper call, user needs > to check every bit (or #define) individually and maintain the > check code over API versions. > > Now you are creating multiple control paths in the application. > Someone could be using this function and only support the case that > all stats counters are supported. Then running on some other platform, > all counters will not be supported and another path through the code > (that checks whether the individual counters used are supported) must > be taken. This path might not exist (because it was not needed) or > might not work correctly (because never tested). > > Just provide one way of doing this. Better the return the supported > counters as a bitmask (uint32_t). The caller can check (e.g. using > bit-wise and operation) whether (all of) the counters of interest are > supported. pktio has struct. Maybe we need some storage in that struct filled on .init ? And some odp call which returns pointer to that storage: Something like: struct odp_pktio_caps_t { union { uint32t all <counters caps> <is loopback> <some other caps we will add in future> } static inline odp_pktio_caps_t * odp_pktio_caps(odp_pktio_t pktio); // NULL - pktio does not support caps // !NULL - access to different pktio caps I.e. my point is to avoid any call for accessing caps. Maxim. > > > -Petri > >
// Check that everything is supported
// 1: all is supported, 0: some stats missing
int odp_pktio_stats_capability_all(odp_pktio_stats_capability_t *capability);
And what is the actual use case of this function?
If everything is supported (the common case), user does not have to check any further. It’s also forward compatible way to check that everything is supported. Without this helper call, user needs to check every bit (or #define) individually and maintain the check code over API versions.
Now you are creating multiple control paths in the application. Someone could be using this function and only support the case that all stats counters are supported. Then running on some other platform, all counters will not be supported and another path through the code (that checks whether the individual counters used are supported) must be taken. This path might not exist (because it was not needed) or might not work correctly (because never tested).
Just provide one way of doing this. Better the return the supported counters as a bitmask (uint32_t). The caller can check (e.g. using bit-wise and operation) whether (all of) the counters of interest are supported.
all == 0 works up to 32/64 counters (after that it would be all[0] == 0 && all[1] == 0), which maybe a problem in the future. There are e.g. counters defined for different rx and tx packet sizes, etc. These counter may be many. When capability is per “get stats call”, in principle user needs to check every time if e.g. all 40 counters are still there, before reading those counters.
You can have e.g. two functions (code paths) one for “all supported” another for “some missing”. The first does not have any if’s in fast path, where as the second is full of those
func_all_supported()
{
use counter.x;
use counter.y;
}
func_some_missing()
{
if (capability.x)
use counter.x;
if (capability.y)
use counter.y;
}
-Petri
On 20 October 2015 at 12:08, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolainen@nokia.com> wrote: > // Check that everything is supported > > // 1: all is supported, 0: some stats missing > > int odp_pktio_stats_capability_all(odp_pktio_stats_capability_t > *capability); > > And what is the actual use case of this function? > > > > If everything is supported (the common case), user does not have to check > any further. It’s also forward compatible way to check that everything is > supported. Without this helper call, user needs to check every bit (or > #define) individually and maintain the check code over API versions. > > Now you are creating multiple control paths in the application. Someone > could be using this function and only support the case that all stats > counters are supported. Then running on some other platform, all counters > will not be supported and another path through the code (that checks > whether the individual counters used are supported) must be taken. This > path might not exist (because it was not needed) or might not work > correctly (because never tested). > > > > Just provide one way of doing this. Better the return the supported > counters as a bitmask (uint32_t). The caller can check (e.g. using > bit-wise and operation) whether (all of) the counters of interest are > supported. > > > > all == 0 works up to 32/64 counters (after that it would be all[0] == 0 && > all[1] == 0), which maybe a problem in the future. There are e.g. counters > defined for different rx and tx packet sizes, etc. These counter may be > many. When capability is per “get stats call”, in principle user needs to > check every time if e.g. all 40 counters are still there, before reading > those counters. > > > > You can have e.g. two functions (code paths) one for “all supported” > another for “some missing”. The first does not have any if’s in fast path, > where as the second is full of those > Is this an actual use case you need to optimise? If it is only hypothetical, I think this optimisation is premature. > > > func_all_supported() > > { > > use counter.x; > > use counter.y; > > } > > > > func_some_missing() > > { > > if (capability.x) > > use counter.x; > > if (capability.y) > > use counter.y; > > } > > > > > > -Petri > > >
diff --git a/include/odp/api/packet_io_stats.h b/include/odp/api/packet_io_stats.h new file mode 100644 index 0000000..94af18c --- /dev/null +++ b/include/odp/api/packet_io_stats.h @@ -0,0 +1,166 @@ +/* Copyright (c) 2015, Linaro Limited + * All rights reserved. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +/** + * @file + * + * ODP Packet IO + */ + +#ifndef ODP_API_PACKET_IO_STATS_H_ +#define ODP_API_PACKET_IO_STATS_H_ + +#ifdef __cplusplus +extern "C" { +#endif + +/** @defgroup odp_packet_io ODP PACKET IO + * @{ + */ + +/** + * Packet IO statistics + * + * Packet IO statictics counters follow RFCs for Management Information Base + * (MIB)for use with network management protocols in the Internet community: + * https://tools.ietf.org/html/rfc3635 + * https://tools.ietf.org/html/rfc2863 + * https://tools.ietf.org/html/rfc2819 + */ +typedef struct odp_pktio_stats_t { + /** + * The number of octets in valid MAC frames received on this interface, + * including the MAC header and FCS. See ifHCInOctets counter + * description in RFC 3635 for details. + */ + uint64_t in_octets; + /** + * The number of packets, delivered by this sub-layer to a higher + * (sub-)layer, which were not addressed to a multicast or broadcast + * address at this sub-layer. See InUcastPkts in RFC 2863. + */ + uint64_t in_ucast_pkts; + /** + * The number of inbound packets which were chosen to be discarded + * even though no errors had been detected to preven their being + * deliverable to a higher-layer protocol. One possible reason for + * discarding such a packet could be to free up buffer space. + * See InDiscards in RFC 2863. + */ + uint64_t in_discards; + /** + * The sum for this interface of AlignmentErrors, FCSErrors, FrameTooLongs, + * InternalMacReceiveErrors. See InErrors in RFC 3635. + */ + uint64_t in_errors; + /** + * For packet-oriented interfaces, the number of packets received via + * the interface which were discarded because of an unknown or + * unsupported protocol. For character-oriented or fixed-length + * interfaces that support protocol multiplexing the number of + * transmission units received via the interface which were discarded + * because of an unknown or unsupported protocol. For any interface + * that does not support protocol multiplexing, this counter will always + * be 0. See InUnknownProtos in RFC 2863. + */ + uint64_t in_unknown_protos; + /** + * The number of octets transmitted in valid MAC frames on this + * interface, including the MAC header and FCS. This does include + * the number of octets in valid MAC Control frames transmitted on + * this interface. See OutOctets in RFC 3635. + */ + uint64_t out_octets; + /** + * The total number of packets that higher-level protocols requested + * be transmitted, and which were not addressed to a multicast or + * broadcast address at this sub-layer, including those that were + * discarded or not sent. does not include MAC Control frames. + * See OutUcastPkts in RFC 2863, 3635. + */ + uint64_t out_ucast_pkts; + /** + * The number of outbound packets which were chosen to be discarded + * even though no errors had been detected to prevent their being + * transmitted. One possible reason for discarding such a packet could + * be to free up buffer space. See OutDiscards in RFC 2863. + */ + uint64_t out_discards; + /** + * The sum for this interface of SQETestErrors, LateCollisions, + * ExcessiveCollisions, InternalMacTransmitErrors and + * CarrierSenseErrors. See OutErrors in RFC 3635. + */ + uint64_t out_errors; +} odp_pktio_stats_t; + +/** Bit in odp_pktio_stats_mask_t defines if in_octets counter + * of odp_pktio_stats_t struct is supported */ +#define ODP_PKTIO_STATS_IN_OCTETS (1 << 0) +/** Bit in odp_pktio_stats_mask_t defines if in_ucast_pkts counter + * of odp_pktio_stats_t struct is supported */ +#define ODP_PKTIO_STATS_IN_UCAST_PKTS (1 << 1) +/** Bit in odp_pktio_stats_mask_t defines if in_discards counter + * of odp_pktio_stats_t struct is supported */ +#define ODP_PKTIO_STATS_IN_DISCARDS (1 << 2) +/** Bit in odp_pktio_stats_mask_t defines if in_errors counter + * of odp_pktio_stats_t struct is supported */ +#define ODP_PKTIO_STATS_IN_ERRORS (1 << 3) +/** Bit in odp_pktio_stats_mask_t defines if in_unknown_protos counter + * of odp_pktio_stats_t struct is supported */ +#define ODP_PKTIO_STATS_IN_UPROTOS (1 << 4) +/** Bit in odp_pktio_stats_mask_t defines if out_octets counter + * of odp_pktio_stats_t struct is supported */ +#define ODP_PKTIO_STATS_OUT_OCTETS (1 << 5) +/** Bit in odp_pktio_stats_mask_t defines if out_ucast_pkts counter + * of odp_pktio_stats_t struct is supported */ +#define ODP_PKTIO_STATS_OUT_UCAST_PKTS (1 << 6) +/** Bit in odp_pktio_stats_mask_t defines if out_discards counter + * of odp_pktio_stats_t struct is supported */ +#define ODP_PKTIO_STATS_OUT_DISCARDS (1 << 7) +/** Bit in odp_pktio_stats_mask_t defines if out_errors counter + * of odp_pktio_stats_t struct is supported */ +#define ODP_PKTIO_STATS_OUT_ERRORS (1 << 8) + +/** + * Mask type of supported counters by platform, defined as: + * ODP_PKTIO_STATS_xxxx + */ +typedef uint32_t odp_pktio_stats_mask_t; + +/** + * Get statistics for pktio handle + * + * @param pktio Packet IO handle + * @param[out] *stats Output buffer for counters + * @param[out] *mask Output buffer for counters supported mask + * @retval 0 on success + * @retval <0 on failure + */ +int odp_pktio_stats(odp_pktio_t pktio, + odp_pktio_stats_t *stats, + odp_pktio_stats_mask_t *mask); + +/** + * Reset statistics for pktio handle + * + * @param pktio Packet IO handle + * @param mask Counters mask to reset + * @retval 0 on success + * @retval <0 on failure + * + */ +int odp_pktio_stats_reset(odp_pktio_t pktio, odp_pktio_stats_mask_t mask); + +/** + * @} + */ + +#ifdef __cplusplus +} +#endif + +#endif diff --git a/platform/linux-generic/include/odp/packet_io.h b/platform/linux-generic/include/odp/packet_io.h index 1d690f5..18f8e78 100644 --- a/platform/linux-generic/include/odp/packet_io.h +++ b/platform/linux-generic/include/odp/packet_io.h @@ -33,6 +33,7 @@ extern "C" { */ #include <odp/api/packet_io.h> +#include <odp/api/packet_io_stats.h> #ifdef __cplusplus }
Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> --- include/odp/api/packet_io_stats.h | 166 +++++++++++++++++++++++++ platform/linux-generic/include/odp/packet_io.h | 1 + 2 files changed, 167 insertions(+) create mode 100644 include/odp/api/packet_io_stats.h