From: EXT Ola Liljedahl [mailto:[email protected]] 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) <[email protected]<mailto:[email protected]>> 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
_______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
