Re: [RFCv7 PATCH net-next 01/36] net: introduce operation helpers for netdev features

[Alexander Lobakin <alexandr.lobakin@intel.com>]

From: "shenjian (K)" <shenjian15@huawei.com>
Date: Wed, 10 Aug 2022 19:32:28 +0800

> 在 2022/8/10 17:43, Alexander Lobakin 写道:
> > From: Jian Shen <shenjian15@huawei.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@huawei.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