| 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: <20220811104936.3675-1-alexandr.lobakin@intel.com>
Date: Thu, 11 Aug 2022 12:49:36 +0200
From: Alexander Lobakin <alexandr.lobakin@...el.com>
To: "shenjian (K)" <shenjian15@...wei.com>
Cc: Alexander Lobakin <alexandr.lobakin@...el.com>,
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
From: "shenjian (K)" <shenjian15@...wei.com>
Date: Wed, 10 Aug 2022 19:32:28 +0800
> 在 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.
Ah, correct then, sure! I missed that fact.
>
>
> >> 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.
But why? My point is that you have to create a whole bunch of
copy'n'paste functions differing only by the &net_device field
name.
>
> >> +
> >> +#define netdev_hw_features_zero(ndev) \
> >> + netdev_features_zero(&ndev->hw_features)
Oh BTW: wrap `ndev` in the netdev_features_zero() call into braces,
`netdev_feature_zero(&(ndev)->hw_features)`, otherwise it may cause
unwanted sneaky logical changes or build failures.
> >> +
> >> +#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.
No, netdev_features_intersects() is okay, leave it as it is. Just
look on bitmap_*() prototypes and return its values when applicable.
>
> >> +}
> >> +
> >> +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
> >
> > .
Thanks,
Olek
Powered by blists - more mailing lists