Re: [PATCH 01/20] net-core: extending (hw_/wanted_/vlan_)features fields to a bitmap.

[Mahesh Bandewar <maheshb@google.com>]

On Wed, Apr 6, 2011 at 3:29 AM, Michał Mirosław <mirqus@gmail.com> wrote:
> 2011/4/6 Mahesh Bandewar <maheshb@google.com>:
>> Converting current use of (hw_/wanted_/vlan_)features to
>> legacy_(hw_/wanted_/vlan_)features to differntiate from the proposed usage.
>>
>> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
>> ---
>>  include/linux/netdevice.h |  110 +++++++++++++++++++++++++++++++-------------
>>  net/core/dev.c            |   51 +++++++++++----------
>>  net/core/ethtool.c        |   97 ++++++++++++++++++++-------------------
>>  net/core/net-sysfs.c      |    4 +-
>>  net/core/sock.c           |    2 +-
>>  5 files changed, 155 insertions(+), 109 deletions(-)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 09d2624..637bf2a 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -980,6 +980,42 @@ struct net_device_ops {
>>                                                    u32 features);
>>  };
>>
>> +enum netdev_features {
>> +       NETIF_F_SG_BIT,                 /* Scatter/gather IO. */
>> +       NETIF_F_IP_CSUM_BIT,            /* Can checksum TCP/UDP over IPv4. */
>> +       NETIF_F_NO_CSUM_BIT,            /* Does not require checksum. F.e. loopack. */
>> +       NETIF_F_HW_CSUM_BIT,            /* Can checksum all the packets. */
>> +       NETIF_F_IPV6_CSUM_BIT,          /* Can checksum TCP/UDP over IPV6 */
>> +       NETIF_F_HIGHDMA_BIT,            /* Can DMA to high memory. */
>> +       NETIF_F_FRAGLIST_BIT,           /* Scatter/gather IO. */
>> +       NETIF_F_HW_VLAN_TX_BIT,         /* Transmit VLAN hw acceleration */
>> +       NETIF_F_HW_VLAN_RX_BIT,         /* Receive VLAN hw acceleration */
>> +       NETIF_F_HW_VLAN_FILTER_BIT,     /* Receive filtering on VLAN */
>> +       NETIF_F_VLAN_CHALLENGED_BIT,    /* Device cannot handle VLAN packets */
>> +       NETIF_F_GSO_BIT,                /* Enable software GSO. */
>> +       NETIF_F_LLTX_BIT,               /* LockLess TX - deprecated. Please */
>> +                                       /* do not use LLTX in new drivers */
>> +       NETIF_F_NETNS_LOCAL_BIT,        /* Does not change network namespaces */
>> +       NETIF_F_GRO_BIT,                /* Generic receive offload */
>> +       NETIF_F_LRO_BIT,                /* large receive offload */
>> +       /* the GSO_MASK reserves bits 16 through 23 */
>> +       RESERVED1_BIT,
>> +       RESERVED2_BIT,
>> +       RESERVED3_BIT,
>> +       RESERVED4_BIT,
>> +       RESERVED5_BIT,
>> +       RESERVED6_BIT,
>> +       RESERVED7_BIT,
>> +       RESERVED8_BIT,
>> +       NETIF_F_FCOE_CRC_BIT,           /* FCoE CRC32 */
>> +       NETIF_F_SCTP_CSUM_BIT,          /* SCTP checksum offload */
>> +       NETIF_F_FCOE_MTU_BIT,           /* Supports max FCoE MTU, 2158 bytes*/
>> +       NETIF_F_NTUPLE_BIT,             /* N-tuple filters supported */
>> +       NETIF_F_RXHASH_BIT,             /* Receive hashing offload */
>> +       NETIF_F_RXCSUM_BIT,             /* Receive checksumming offload */
>> +       NETIF_F_NOCACHE_COPY_BIT,       /* Use no-cache copyfromuser */
>> +};
>> +
>
> This should be a separate cleanup patch. And after that, for the
> conversion you would add as a last entry:
> NETIF_F_NUM_BITS and use it later (see below).
>
I like the idea of NETIF_F_NUM_BITS and I'll change the #defines
accordingly. Couple of bitmaps are defined in this patch and NUM_BITS
will be required to define those bitmaps. This is reason why I have
defined above enum now rather than waiting for the cleanup phase. Also
it should not change radically in that time span.

>>  /*
>>  *     The DEVICE structure.
>>  *     Actually, this whole structure is a big mistake.  It mixes I/O
>> @@ -1029,44 +1065,51 @@ struct net_device {
>>        struct list_head        napi_list;
>>        struct list_head        unreg_list;
>>
>> +#define DEV_FEATURE_WORDS      2
>> +#define        DEV_FEATURE_BITS        (DEV_FEATURE_WORDS*sizeof(long)*BITS_PER_BYTE)
>> +#define LEGACY_FEATURE_WORD    0
>> +
>
> #define DEV_FEATURE_WORDS BITS_TO_LONGS(NETIF_F_NUM_BITS)
> #define DEV_FEATURE_BITS (DEV_FEATURE_WORDS*BITS_PER_LONG)
>
Yes, this is good!

> Though using bitmaps will make a mess for 32 versus 64 bit archs. It
> would be better to stick to u32 as the base type instead of long.
>
Once it's a bitmap; type shouldn't matter and each arch and it's
specific macros/inlines should handle them properly, no?
(I changed the base type since DECLARE_BITMAP() declares 'unsigned
long' and we can make use of readily avaialble set/test/clear_bit
macros)

> [...]
>> @@ -2376,13 +2419,13 @@ static inline void netif_tx_unlock_bh(struct net_device *dev)
>>  }
>>
>>  #define HARD_TX_LOCK(dev, txq, cpu) {                  \
>> -       if ((dev->features & NETIF_F_LLTX) == 0) {      \
>> +       if ((dev->legacy_features & NETIF_F_LLTX) == 0) {       \
> [...]
>
> For those type of conversion there is really no point in using the
> macro. Changing it to
> dev->features[0] instead of dev->legacy_features needs the same effort
> but avoids the
> cleanup later. Flags in other feature words could have names line
> NETIF_F2_xxx so that
> it would be clear in which word they belong.
>
I know! But changing all at once in zillion places is hard. This will
let us make changes progressively. I'm expecting the following
progress path -

(1) (Current patch)
+ #define legacy_feactures          features
+ #define legacy_vlan_features    vlan_features

Slowly make all the changes (relatively easy but a lengthy process!).
So the places where vlan_features, features fields are used will
compile and at the same time updates, where legacy_vlan_features,
legacy_features fields are used, will compile too. Once all is changes
are in place -

(2) Relatively small change
-  unsigned long       features;
+ DECLARE_BITMAP(feature, DEV_FEATURE_BITS);
-  unsigned long        vlan_features;
+ DECLARE_BITMAP(vlan_feature, DEV_FEATURE_BITS);
-  #define   legacy_features          features
+ #define   legacy_features          feature[0]
- #define    legacy_vlan_features  vlan_features
+ #define   legacy_vlan_features  vlan_feature[0]

At this moment old fields are gone and are replaced by the bitmaps and
the legacy usage is indicated by the use "legacy_*" fields. This
should eventually be changed to use the (set/test/clear)_bit()
macros/inlines. So the above place should look like -

#define HARD_TX_LOCK(dev, txq, cpu) {                  \
-       if ((dev->legacy_features & NETIF_F_LLTX) == 0) {       \
+      if (!test_bit(dev->feature, NETIF_F_LLTX_BIT) {      \


Thanks,
--mahesh..