On 20 October 2015 at 10:29, Savolainen, Petri (Nokia - FI/Espoo) < [email protected]> wrote:
> > > > > *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]> 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 > > > > > > > > >
_______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
