Re: [PATCH] net/core: Export dev_core_stats_rx_dropped_inc sets

From: Yajun Deng <redacted>
Date: Thu, 14 Sep 2023 10:44:14 +0800

On 2023/9/13 17:58, Alexander Lobakin wrote:

quoted

From: Yajun Deng <redacted>
Date: Wed, 13 Sep 2023 10:08:08 +0800

quoted

On 2023/9/13 00:22, Alexander Lobakin wrote:

quoted

From: Yajun Deng <redacted>
Date: Mon, 11 Sep 2023 16:20:16 +0800

[...]

quoted

quoted

EXPORT_SYMBOL_GPL(dev_core_stats_inc); // Why not GPL BTW?

This may be a better option.

Just because EXPORT_SYMBOL(netdev_core_stats_alloc) before,  but I think

EXPORT_SYMBOL_GPL is better.

Ah I see. BTW, if you will still define increment functions as
externals, there will be no reason to export netdev_core_stats_alloc()
or even make it non-static at all.

quoted

quoted

And then build inlines:

#define DEV_CORE_STATS_INC(FIELD)                \
static inline void                        \
dev_core_stats_##FIELD##_inc(struct net_device *dev)        \
{                                \
    dev_core_stats_inc(dev,                    \
        offsetof(struct net_device_core_stats, FIELD));    \
}

DEV_CORE_STATS_INC(rx_dropped);
...

OR even just make them macros

#define __DEV_CORE_STATS_INC(dev, field)            \
    dev_core_stats_inc(dev,                    \
        offsetof(struct net_device_core_stats, field))

#define dev_core_stats_rx_dropped_inc(dev)            \
    __DEV_CORE_STATS_INC(dev, rx_dropped)
...

I would like the former.  Keep it the same as before.

By "the former" you mean to build static inlines or externals? Seems
like the first one, but I got confused by your "the same as before" :D

quoted

quoted

Just don't copy that awful Thunderbird's line wrap and don't assume
this
code builds and works and that is something finished/polished.

You'll be able to trace functions and you'll be able to understand
which
counter has been incremented by checking the second argument, i.e. the
field offset (IIRC tracing shows you arguments).
And that way you wouldn't geometrically increase the number of symbol
exports and deal with its consequences.

I agree that.

Ok, after this one I guess you meant "I'd like to use your approach with
static inlines".

Finally, I give up this approach.

The new function dev_core_stats_inc() didn't called by external modules
directly.

If it's called via an inline or macro or whatever, it still needs to be
exported.
Double-check that modpost doesn't complain on allmodconfig build.

So EXPORT_SYMBOL_GPL(dev_core_stats_inc) can be removed by anyone.

That doesn't mean it won't be needed tomorrow.
And I don't feel like it's a good excuse to define 1 external function
per counter instead of 1 external + static inlines for the rest. It's
not only about the exports.
Esp. given that I wrote almost the whole code needed for it to work in
one of my previous replies.

If you don't want to do that, I could take it over xD

quoted

quoted

quoted

quoted

    /**
   *    dev_get_stats    - get network device statistics

Thanks,
Olek

Thanks,
Olek

Thanks,
Olek