Re: [PATCH v4 net-next 08/18] net: make dev_get_stats return void

On Fri, Jan 8, 2021 at 11:31 AM Vladimir Oltean [off-list ref] wrote:

On Fri, Jan 08, 2021 at 11:14:50AM +0100, Eric Dumazet wrote:

quoted

On Fri, Jan 8, 2021 at 1:20 AM Vladimir Oltean [off-list ref] wrote:

quoted

From: Vladimir Oltean <vladimir.oltean@nxp.com>

After commit 28172739f0a2 ("net: fix 64 bit counters on 32 bit arches"),
dev_get_stats got an additional argument for storage of statistics. At
this point, dev_get_stats could return either the passed "storage"
argument, or the output of .ndo_get_stats64.

Then commit caf586e5f23c ("net: add a core netdev->rx_dropped counter")
came, and the output of .ndo_get_stats64 (still returning a pointer to
struct rtnl_link_stats64) started being ignored.

Then came commit bc1f44709cf2 ("net: make ndo_get_stats64 a void
function") which made .ndo_get_stats64 stop returning anything.

So now, dev_get_stats always reports the "storage" pointer received as
argument. This is useless. Some drivers are dealing with unnecessary
complexity due to this, so refactor them to ignore the return value
completely.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---

This seems like a lot of code churn.

Not sure there's something I can do to avoid that.

quoted

Ultimately we need this function to return an error code, so why keep
this patch with a void return ?

Please squash your patches a bit, to avoid having 18 patches to review.

Because the "make dev_get_stats return void" patch changes the callers
to poke through their stack-supplied struct rtnl_link_stats64 instead of
through the returned pointer. So all changes within this patch are of
the same type: replace a pointer dereference with a plain structure
field access. Whereas the "allow ndo_get_stats64 to return an int error
code" touches a completely unrelated portion: the ndo_get_stats64
callback. Again, that patch does one thing and one thing only. Then
there's the error checking, which is split in 3 patches:
- Special cases with non-trivial work to do: FCoE, OVS
- Propagation of errors from dev_get_stats
- Termination of errors from dev_get_stats

So you would like me to squash what exactly? I know there's a lot of
patches, but if I go ahead and combine them, it'll be even more
difficult to review, due to the mix of types of changes being applied.

quoted

Additionally I would suggest a __must_check attribute on
dev_get_stats() to make sure we converted all callers.

Ok, but that will mean even more patches (since the error checking is
done in 3 stages, the __must_check must be put at the end). And remember,
the inflation of this series from 12 to 18 patches came from your
suggestion to propagate the errors now.

I did not suggest adding more patches. where have you seen this exactly ?

quoted

I can not convince myself that after your patches, bonding does the
right thing...

Why?

Because of this ?

+
+       /* Recurse with no locks taken */
+       for (i = 0; i < num_slaves; i++)
+               dev_get_stats(slaves[i], &dev_stats[i]);

Look, it seems you rely on us testing your patches, and spending more
time on review than you .

Please sit back, and test a bit more before sending a new series.

I think I will not spend more time on this, because it is obvious to
me that you consider that I _have_ to accept whatever solution you
came up,
because you think it is the optimal one.