From: "shenjian (K)" shenjian15@huawei.com Date: Wed, 10 Aug 2022 21:34:43 +0800
BTW, you replied in HTML instead of plain text and korg mail servers rejected it. So non-Ccs can't see it. Just be aware that LKML accepts plain text only :)
在 2022/8/10 19:35, Alexander Lobakin 写道:
From: Jian Shen shenjian15@huawei.com Date: Wed, 10 Aug 2022 11:06:24 +0800
For the prototype of netdev_features_t is u64, and the number of netdevice feature bits is 64 now. So there is no space to introduce new feature bit. Change the prototype of netdev_features_t from u64 to structure below: typedef struct { DECLARE_BITMAP(bits, NETDEV_FEATURE_COUNT); } netdev_features_t;
Rewrite the netdev_features helpers to adapt with new prototype.
To avoid mistake using NETIF_F_XXX as NETIF_F_XXX_BIT as input macroes for above helpers, remove all the macroes of NETIF_F_XXX for single feature bit. Serveal macroes remained temporarily, by some precompile dependency.
With the prototype is no longer u64, the implementation of print interface for netdev features(%pNF) is changed to bitmap. So does the implementation of net/ethtool/.
Signed-off-by: Jian Shen shenjian15@huawei.com
drivers/net/ethernet/amazon/ena/ena_netdev.c | 12 +- .../net/ethernet/intel/i40e/i40e_debugfs.c | 12 +- .../ethernet/netronome/nfp/nfp_net_common.c | 4 +- .../net/ethernet/pensando/ionic/ionic_lif.c | 4 +- include/linux/netdev_features.h | 101 ++---------- include/linux/netdev_features_helper.h | 149 +++++++++++------- include/linux/netdevice.h | 7 +- include/linux/skbuff.h | 4 +- include/net/ip_tunnels.h | 2 +- lib/vsprintf.c | 11 +- net/ethtool/features.c | 96 ++++------- net/ethtool/ioctl.c | 46 ++++-- net/mac80211/main.c | 3 +- 13 files changed, 201 insertions(+), 250 deletions(-)
[...]
-static inline int find_next_netdev_feature(u64 feature, unsigned long start) -{
- /* like BITMAP_LAST_WORD_MASK() for u64
* this sets the most significant 64 - start to 0.*/- feature &= ~0ULL >> (-start & ((sizeof(feature) * 8) - 1));
- return fls64(feature) - 1;
-} +#define NETIF_F_HW_VLAN_CTAG_TX +#define NETIF_F_IPV6_CSUM +#define NETIF_F_TSO +#define NETIF_F_GSO
Uhm, what are those empty definitions for? They look confusing.
I kept them temporary for some drivers use them like below: for example in drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c #ifdef NETIF_F_HW_VLAN_CTAG_TX #include <linux/if_vlan.h> #endif
So far I haven't got a good way to replace it.
I believe such constructs sneaked in from some development/draft versions of the code, as those definitions are always here, so this is just redundant/pointless. Just remove those ifdefs and always include the file. The empty definitions you left in netdev_features.h are confusing, I'd not keep them.
/* This goes for the MSB to the LSB through the set feature bits,
- mask_addr should be a u64 and bit an int
*/
[...]
+#define GSO_ENCAP_FEATURES (((u64)1 << NETIF_F_GSO_GRE_BIT) | \
((u64)1 << NETIF_F_GSO_GRE_CSUM_BIT) | \((u64)1 << NETIF_F_GSO_IPXIP4_BIT) | \((u64)1 << NETIF_F_GSO_IPXIP6_BIT) | \(((u64)1 << NETIF_F_GSO_UDP_TUNNEL_BIT) | \((u64)1 << NETIF_F_GSO_UDP_TUNNEL_CSUM_BIT)))
- 1ULL;
ok，will fix it
- what if we get a new GSO encap type which's bit will be higher than 64?
So far I prefer to use this. It's used to assgned to skb_shinfo(skb)->gso_type, which prototype is 'unsigned int'. Once new gso encap type introduced, we should extend the gso_type first.
But ::gso_type accepts flags like %SKB_GSO_DODGY and so on, not netdev_features, doesn't it?
- #endif /* _LINUX_NETDEV_FEATURES_H */
[...]
static inline netdev_features_t netdev_features_and(const netdev_features_t a, const netdev_features_t b) {
- return a & b;
- netdev_features_t dst;
- bitmap_and(dst.bits, a.bits, b.bits, NETDEV_FEATURE_COUNT);
- return dst;
Yeah, so as I wrote previously, not a good idea to return a whole bitmap/structure.
netdev_features_and(*dst, const *a, const *b) { return bitmap_and(); // bitmap_and() actually returns useful value }
I mean, 16 bytes (currently 8, but some new features will come pretty shortly, I'm sure) are probably okayish, but... let's see what other folks think, but even Linus wrote about this recently BTW.
Yes, Jakub also mentioned this.
But there are many existed features interfaces(e.g. ndo_fix_features, ndo_features_check), use netdev_features_t as return value. Then we have to change their prototype.
We have to do 12k lines of changes already :D You know, 16 bytes is probably fine to return directly and it will be enough for up to 128 features (+64 more comparing to the mainline). OTOH, using pointers removes that "what if/when", so it's more flexible in that term. So that's why I asked for other folks' opinions -- 2 PoVs doesn't seem enough here.
second problem is for the helpers' definition. For example: When we introduce helper like netdev_features_zero(netdev_features_t *features) without change prototype of netdev_features_t. once covert netdev_features_t from u64 to unsigned long *, then it becomes netdev_features_zero(unsigned long **features), result in much redundant work to adjust it to netdev_features_zero(unsigned long *features).
}
[...]
static noinline_for_stack -char *netdev_bits(char *buf, char *end, const void *addr, +char *netdev_bits(char *buf, char *end, void *addr, struct printf_spec spec, const char *fmt) {
- unsigned long long num;
- int size;
- netdev_features_t *features;
const? We're printing.
It will cause compile warning for bitmap_string use features->bits as input param without "const" definition in its prototype.
Oof, that's weird. I checked the function you mentioned and don't see any reason why it would require non-RO access to the bitmap it prints. Could you maybe please change its proto to take const bitmap, so that it won't complain on your code? As a separate patch going right before this one in your series.
if (check_pointer(&buf, end, addr, spec)) return buf;
switch (fmt[1]) { case 'F':
num = *(const netdev_features_t *)addr;size = sizeof(netdev_features_t);
features = (netdev_features_t *)addr;Casts are not needed when assigning from `void *`.
ok, will fix it
spec.field_width = NETDEV_FEATURE_COUNT;
- return special_hex_number(buf, end, num, size);
- return bitmap_string(buf, end, features->bits, spec, fmt); }
[...]
-- 2.33.0
That's my last review email for now. Insane amount of work, I'm glad someone did it finally. Thanks a lot!
Olek .
Hi Olek, Grateful for your review. You made a lot of valuable suggestions. I will check and continue refine the patchset.
Thanks again!
Jian
Thanks! Olek