在 2022/8/11 18:49, Alexander Lobakin 写道:

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

quoted

在 2022/8/10 17:43, Alexander Lobakin 写道:

quoted

From: Jian Shen <shenjian15@huawei.com>
Date: Wed, 10 Aug 2022 11:05:49 +0800

quoted

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 

+++++++++++++++++++++++++

quoted

'netdev_feature_helpers.h' fits more I guess, doesn't it? It
contains several helpers, not only one.

ok， will rename it.

quoted

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.

quoted

quoted

quoted

  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

quoted

quoted

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),    \

quoted

quoted

+        .feature_bits = { features },                \
+    }
+
  enum {
      NETIF_F_SG_BIT,            /* Scatter/gather IO. */
      NETIF_F_IP_CSUM_BIT,        /* Can checksum TCP/UDP over 

IPv4. */

quoted

quoted

diff --git a/include/linux/netdev_features_helper.h 

b/include/linux/netdev_features_helper.h

quoted

quoted

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.

I noticed that Jakub have done a lot work for avoiding driver to write 
netdev->dev_addr
directly. Also in earlier discuss, Saeed had suggested to hide hide the 
implementation
details and abstract it away from drivers using getters and manipulation 
APIs.
[https://lore.kernel.org/all/b335852ecaba3c86d1745b5021bb500798fc843b.camel@kernel.org/ (local)]

quoted

quoted

quoted

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

OK, will fix it.

quoted

quoted

quoted

+
+#define netdev_wanted_features_zero(ndev) \

[...]

quoted

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

OK, will follow the prototypes of bitmap_and and others.

quoted

quoted

quoted

+}
+
+static inline void
+netdev_active_features_mask(struct net_device *ndev,
+                const netdev_features_t features)
+{
+    ndev->features = netdev_active_features_and(ndev, features);
+}

[...]

quoted

+/* 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.

quoted

quoted

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

[...]

quoted

-- >> 2.33.0

Thanks,
Olek

.

Thanks,
Olek

.

Thanks,
Jian