Date:   Fri, 12 Aug 2022 19:30:13 +0800
From:   "shenjian (K)" <shenjian15@...wei.com>
To:     Alexander Lobakin <alexandr.lobakin@...el.com>
CC:     <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 36/36] net: redefine the prototype of
 netdev_features_t



在 2022/8/11 21:07, Alexander Lobakin 写道:
> From: "shenjian (K)" <shenjian15@...wei.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@...wei.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@...wei.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.
>
OK, I will add a new patch to remove 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)))
>> > 1) 1ULL;
>> ok，will fix it
>>
>> > 2) 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.
>
OK,  will do it.

>> >>   >>       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;
>> >>           break;
>> >>       default:
>> >>           return error_string(buf, end, "(%pN?)", spec);
>> >>       }
>> >>   >> -    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
> .
>
Thanks,
Jian

Powered by blists - more mailing lists