| 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
| ||
|
Date: Wed, 20 Apr 2022 17:24:06 +0800
From: "shenjian (K)" <shenjian15@...wei.com>
To: Alexander Lobakin <alexandr.lobakin@...el.com>
CC: <davem@...emloft.net>, <kuba@...nel.org>, <andrew@...n.ch>,
<ecree.xilinx@...il.com>, <hkallweit1@...il.com>,
<saeed@...nel.org>, <leon@...nel.org>, <netdev@...r.kernel.org>,
<linuxarm@...neuler.org>, <lipeng321@...wei.com>
Subject: Re: [RFCv6 PATCH net-next 01/19] net: introduce operation helpers for
netdev features
在 2022/4/19 22:40, Alexander Lobakin 写道:
> From: Jian Shen <shenjian15@...wei.com>
> Date: Tue, 19 Apr 2022 10:21:48 +0800
>
>> Introduce a set of bitmap operation helpers for netdev features,
>> then we can use them to replace the logical operation with them.
>> As the nic driversare not supposed to modify netdev_features
>> directly, it also introduces wrappers helpers to this.
>>
>> The implementation of these helpers are based on the old prototype
>> of netdev_features_t is still u64. I will rewrite them on the last
>> patch, when the prototype changes.
>>
>> To avoid interdependencies between netdev_features_helper.h and
>> netdevice.h, put the helpers for testing feature is set in the
>> netdevice.h, and move advandced helpers like
>> netdev_get_wanted_features() and netdev_intersect_features() to
>> netdev_features_helper.h.
>>
>> Signed-off-by: Jian Shen <shenjian15@...wei.com>
>> ---
>> .../net/ethernet/netronome/nfp/nfp_net_repr.c | 1 +
>> include/linux/netdev_features.h | 12 +
>> include/linux/netdev_features_helper.h | 604 ++++++++++++++++++
>> include/linux/netdevice.h | 45 +-
>> net/8021q/vlan_dev.c | 1 +
>> net/core/dev.c | 1 +
>> 6 files changed, 646 insertions(+), 18 deletions(-)
>> create mode 100644 include/linux/netdev_features_helper.h
>>
>> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
>> index ba3fa7eac98d..08f2c54e0a11 100644
>> --- a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
>> +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
>> @@ -4,6 +4,7 @@
>> #include <linux/etherdevice.h>
>> #include <linux/io-64-nonatomic-hi-lo.h>
>> #include <linux/lockdep.h>
>> +#include <linux/netdev_features_helper.h>
>> #include <net/dst_metadata.h>
>>
>> #include "nfpcore/nfp_cpp.h"
>> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
>> index 2c6b9e416225..e2b66fa3d7d6 100644
>> --- a/include/linux/netdev_features.h
>> +++ b/include/linux/netdev_features.h
>> @@ -11,6 +11,18 @@
>>
>> typedef u64 netdev_features_t;
>>
>> +struct netdev_feature_set {
>> + unsigned int cnt;
>> + unsigned short feature_bits[];
>> +};
>> +
>> +#define DECLARE_NETDEV_FEATURE_SET(name, features...) \
>> + static unsigned short __##name##_s[] = {features}; \
>> + struct netdev_feature_set name = { \
> I suggest using `const` here. Those sets are needed only to
> initialize bitmaps, that's it. They are not supposed to be
> modified. This would be one more hardening here to avoid some weird
> usages of sets, and also would place them in .rodata instead of just
> .data.
>
> Function old new delta
> main 35 33 -2
> Total: Before=78, After=76, chg -2.56%
> add/remove: 0/2 grow/shrink: 0/0 up/down: 0/-14 (-14)
> Data old new delta
> arr1 6 - -6
> arr2 8 - -8
> Total: Before=15, After=1, chg -93.33%
> add/remove: 2/0 grow/shrink: 0/0 up/down: 14/0 (14)
> RO Data old new delta
> arr1 - 8 +8
> arr2 - 6 +6
> Total: Before=36, After=50, chg +38.89%
>
> As you can see, there's a 2-byte code optimization. And that was
> just a simpliest oneliner. The gains will be much bigger from the
> real usages.
thanks, will add it.
>> + .cnt = ARRAY_SIZE(__##name##_s), \
>> + .feature_bits = {features}, \
>> + }
> The problem with the current macro is that it doesn't allow to
> declare feature sets as static. Because the temporary array for
> counting the number of bits goes first, and doing
>
> static DECLARE_NETDEV_FEATURE_SET();
>
> wouldn't change anything.
Yes, it bothered me.
> But we want to have most feature sets static as they will be needed
> only inside one file. Making every of them global would hurt
> optimization.
>
> At the end, I came to
>
> #define DECLARE_NETDEV_FEATURE_SET(name, features...) \
> const struct netdev_feature_set name = { \
> .feature_bits = { features }, \
> .cnt = sizeof((u16 []){ features }) / sizeof(u16), \
> }
>
> because ARRAY_SIZE() can be taken only from a variable, not from
> a compound literal.
> But this one is actually OK. We don't need ARRAY_SIZE() in here
> since we define an unnamed array of an explicit type that we know
> for sure inline. So there's no chance to do it wrong as long as
> the @features argument is correct.
>
> The ability to make it static is important. For example, when I
> marked them both static, I got
>
> add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
> Function old new delta
> Total: Before=76, After=76, chg +0.00%
> add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
> Data old new delta
> Total: Before=1, After=1, chg +0.00%
> add/remove: 0/2 grow/shrink: 0/0 up/down: 0/-14 (-14)
> RO Data old new delta
> arr1 6 - -6
> arr2 8 - -8
> Total: Before=50, After=36, chg -28.00%
>
> i.e. both of the sets were removed, because my main() was:
>
> printf("cnt1: %u, cnt2: %u\n", arr1.cnt, arr2.cnt);
>
> The compiler saw that I don't use them, except for printing values
> which are actually compile-time constants, and wiped them.
> Previously, they were global so it didn't have a clue if they're
> used anywhere else.
> This was a simple stupid example, but it will bring a lot more value
> in real use cases. So please consider my variant :D
Make sense. I will fix it.
Thanks a lot!
>> +
>> enum {
>> NETIF_F_SG_BIT, /* Scatter/gather IO. */
>> NETIF_F_IP_CSUM_BIT, /* Can checksum TCP/UDP over IPv4. */
> --- 8< ---
>
>> --
>> 2.33.0
> Thanks,
> Al
> .
>
Powered by blists - more mailing lists