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