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

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

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.
And BTW, do you think it's worth to create a new file rather than
put everything just in 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.

> +
> +#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.

> +}
> +
> +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).

> +		netdev_feature_add(set->feature_bits[i], dst);
> +}

[...]

> -- 
> 2.33.0

Thanks,
Olek