Hi Ferruh,

-----Original Message-----
From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
Sent: Thursday, September 28, 2017 5:07 AM
To: Shreyansh Jain <redacted>
Cc: dev@dpdk.org; Hemant Agrawal <redacted>
Subject: Re: [PATCH v4 41/41] net/dpaa: support for extended statistics

On 9/27/2017 9:26 AM, Shreyansh Jain wrote:

quoted

On Thursday 21 September 2017 06:56 PM, Shreyansh Jain wrote:

quoted

On Monday 18 September 2017 08:27 PM, Ferruh Yigit wrote:

quoted

On 9/9/2017 12:21 PM, Shreyansh Jain wrote:

quoted

From: Hemant Agrawal <redacted>

Signed-off-by: Hemant Agrawal <redacted>

<...>

quoted

+static int
+dpaa_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat
*xstats,
+            unsigned int n)
+{
+    struct dpaa_if *dpaa_intf = dev->data->dev_private;
+    unsigned int i = 0, num = RTE_DIM(dpaa_xstats_strings);
+    uint64_t values[sizeof(struct dpaa_if_stats) / 8];
+
+    if (xstats == NULL)
+        return 0;

This is a little not clear from API definition, but I guess when xstats
is NULL, it should return num of available stats, "num" for this case. I
guess there are PMDs implements both, can you please double check?

Ok. I will check again.

I checked a number of other ethev implementations. Some like i40e/e1000
also return 0 when xstats is NULL. Others, like bnx2x and qede don't
handle this situation.
All return "num" when passed argument is larger than number of elements
in the table.

Though, I think the logic that get_xstats should return its size (num)
when passed with NULL, looks good to me.
How does one standardize such semantics for existing APIs?

Thanks for checking, I guess first we should clarify the API and the
expected behavior [1] and later update required PMDs.

So for now I think PMD is OK as it is.


[1]
I double checked the rte_eth_xstats_get(). It does:

If xstats == NULL
	xcount = dev_ops->xstats_get(dev, NULL, 0);
	return count + xcount;

Intention looks like to returning number of available stats, otherwise
returning "count + 0" will be useless.

 
Makes sense. I missed this and kept looking for implementations.
I will at least fix dpaa code.

So it looks like expectation from eth_xstats_get_t for that case is
returning xstats size, but this not clear and not documented in API comment.

quoted

(I can add this info to the API document that you created - but only
once we know if others will agree to change)

 
Probably this info should be in Doxygen APIs [2].

[2] http://dpdk.org/doc/api/rte__ethdev_8h.html#adad5c65f659487db1fefba7d7d902973

quoted

quoted

quoted

quoted

+
+    if (n < num)
+        return num;
+
+    fman_if_stats_get_all(dpaa_intf->fif, values,
+                  sizeof(struct dpaa_if_stats) / 8);
+
+    for (i = 0; i < num; i++) {
+        xstats[i].id = i;
+        xstats[i].value = values[dpaa_xstats_strings[i].offset / 8];
+    }
+    return i;
+}

<...>