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
| ||
|
Message-ID: <444c4f87-ed36-721a-f619-97c7725e2c87@huawei.com> Date: Wed, 10 Aug 2022 19:32:28 +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> Subject: Re: [RFCv7 PATCH net-next 01/36] net: introduce operation helpers for netdev features 在 2022/8/10 17:43, Alexander Lobakin 写道: > From: Jian Shen <shenjian15@...wei.com> > Date: Wed, 10 Aug 2022 11:05:49 +0800 > >> Introduce a set of bitmap operation helpers for netdev features, >> then we can use them to replace the logical operation with them. >> >> The implementation of these helpers are based on the old prototype >> of netdev_features_t is still u64. These helpers will be rewritten >> 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 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> >> --- >> include/linux/netdev_features.h | 11 + >> include/linux/netdev_features_helper.h | 707 +++++++++++++++++++++++++ > 'netdev_feature_helpers.h' fits more I guess, doesn't it? It > contains several helpers, not only one. ok, will rename it. > And BTW, do you think it's worth to create a new file rather than > put everything just in netdev_features.h? Jakub suggested me to move them to a new file, then it can be includued at users appropriately. [https://www.spinics.net/lists/netdev/msg809370.html] And it's unable to put everything in netdev_features.h, because these helpers need to see the definition of struct net_device which is defined in netdevice.h. It leading interdependence for netdeice.h include netdev_features.h. >> include/linux/netdevice.h | 45 +- >> net/8021q/vlan_dev.c | 1 + >> net/core/dev.c | 1 + >> 5 files changed, 747 insertions(+), 18 deletions(-) >> create mode 100644 include/linux/netdev_features_helper.h >> >> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h >> index 7c2d77d75a88..9d434b4e6e6e 100644 >> --- a/include/linux/netdev_features.h >> +++ b/include/linux/netdev_features.h >> @@ -11,6 +11,17 @@ >> >> typedef u64 netdev_features_t; >> >> +struct netdev_feature_set { >> + unsigned int cnt; >> + unsigned short feature_bits[]; >> +}; >> + >> +#define DECLARE_NETDEV_FEATURE_SET(name, features...) \ >> + const struct netdev_feature_set name = { \ >> + .cnt = sizeof((unsigned short[]){ features }) / sizeof(unsigned short), \ >> + .feature_bits = { features }, \ >> + } >> + >> enum { >> NETIF_F_SG_BIT, /* Scatter/gather IO. */ >> NETIF_F_IP_CSUM_BIT, /* Can checksum TCP/UDP over IPv4. */ >> diff --git a/include/linux/netdev_features_helper.h b/include/linux/netdev_features_helper.h >> new file mode 100644 >> index 000000000000..5423927d139b >> --- /dev/null >> +++ b/include/linux/netdev_features_helper.h >> @@ -0,0 +1,707 @@ >> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >> +/* >> + * Network device features helpers. >> + */ >> +#ifndef _LINUX_NETDEV_FEATURES_HELPER_H >> +#define _LINUX_NETDEV_FEATURES_HELPER_H >> + >> +#include <linux/netdevice.h> >> + >> +static inline void netdev_features_zero(netdev_features_t *dst) >> +{ >> + *dst = 0; >> +} >> + >> +/* active_feature prefer to netdev->features */ >> +#define netdev_active_features_zero(ndev) \ >> + netdev_features_zero(&ndev->features) > netdev_features_t sometimes is being placed and used on the stack. > I think it's better to pass just `netdev_features_t *` to those > helpers, this way you wouldn't also need to create a new helper > for each net_device::*_features. My purpose of defining helpers for each net_device::*_features is to avoiding driver to change net_device::*_features directly. >> + >> +#define netdev_hw_features_zero(ndev) \ >> + netdev_features_zero(&ndev->hw_features) >> + >> +#define netdev_wanted_features_zero(ndev) \ > [...] > >> +#define netdev_gso_partial_features_and(ndev, __features) \ >> + netdev_features_and(ndev->gso_partial_features, __features) >> + >> +/* helpers for netdev features '&=' operation */ >> +static inline void >> +netdev_features_mask(netdev_features_t *dst, >> + const netdev_features_t features) >> +{ >> + *dst = netdev_features_and(*dst, features); > A small proposal: if you look at bitmap_and() for example, it > returns 1 if the resulting bitmap is non-empty and 0 if it is. What > about doing the same here? It would probably help to do reduce > boilerplating in the drivers where we only want to know if there's > anything left after masking. > Same for xor, toggle etc. Thanks for point this. Return whether empty, then I can remove netdev_features_intersects helpers. But there are also many places to use 'f1 & f2' as return value or input param, then I need to define more temporay features to store the result, and then return the temporay features or pass into it. >> +} >> + >> +static inline void >> +netdev_active_features_mask(struct net_device *ndev, >> + const netdev_features_t features) >> +{ >> + ndev->features = netdev_active_features_and(ndev, features); >> +} > [...] > >> +/* helpers for netdev features 'set bit array' operation */ >> +static inline void >> +netdev_features_set_array(const struct netdev_feature_set *set, >> + netdev_features_t *dst) >> +{ >> + int i; >> + >> + for (i = 0; i < set->cnt; i++) > Nit: kernel is C11 now, you can do just `for (u32 i = 0; i ...`. > (and yeah, it's better to use unsigned types when you don't plan > to store negative values there). ok, will fix it. >> + netdev_feature_add(set->feature_bits[i], dst); >> +} > [...] > >> -- >> 2.33.0 > Thanks, > Olek > > . >
Powered by blists - more mailing lists