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