netdev - Re: [PATCH] net/core: Export dev_core_stats_rx_dropped

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <48fe0346-40f9-b0f5-991c-8c17349849f7@intel.com>
Date: Thu, 14 Sep 2023 16:10:48 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Yajun Deng <yajun.deng@...ux.dev>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<pabeni@...hat.com>, <horms@...nel.org>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] net/core: Export dev_core_stats_rx_dropped_inc sets

From: Yajun Deng <yajun.deng@...ux.dev>
Date: Thu, 14 Sep 2023 10:44:14 +0800

> 
> On 2023/9/13 17:58, Alexander Lobakin wrote:
>> From: Yajun Deng <yajun.deng@...ux.dev>
>> Date: Wed, 13 Sep 2023 10:08:08 +0800
>>
>>> On 2023/9/13 00:22, Alexander Lobakin wrote:
>>>> From: Yajun Deng <yajun.deng@...ux.dev>
>>>> Date: Mon, 11 Sep 2023 16:20:16 +0800
>> [...]
>>
>>>> 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.
>>
>>>  
>>>> 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
>>
>>>
>>>> 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

> 
> 
>>>>>     /**
>>>>>    *    dev_get_stats    - get network device statistics
>>>> Thanks,
>>>> Olek
>> Thanks,
>> Olek

Thanks,
Olek