Re: [PATCH net-next 1/4] net: Introduce NETIF_F_GRO_HW

[Michael Chan <michael.chan@broadcom.com>]

On Mon, Dec 4, 2017 at 2:15 PM, Yuval Mintz <yuvalm@mellanox.com> wrote:
>> @@ -96,6 +98,7 @@ enum {
>>  #define NETIF_F_FRAGLIST     __NETIF_F(FRAGLIST)
>>  #define NETIF_F_FSO          __NETIF_F(FSO)
>>  #define NETIF_F_GRO          __NETIF_F(GRO)
>> +#define NETIF_F_GRO_HW               __NETIF_F(GRO_HW)
>>  #define NETIF_F_GSO          __NETIF_F(GSO)
>>  #define NETIF_F_GSO_ROBUST   __NETIF_F(GSO_ROBUST)
>>  #define NETIF_F_HIGHDMA              __NETIF_F(HIGHDMA)
>> @@ -193,7 +196,7 @@ enum {
>>   * If upper/master device has these features disabled, they must be disabled
>>   * on all lower/slave devices as well.
>>   */
>> -#define NETIF_F_UPPER_DISABLES       NETIF_F_LRO
>> +#define NETIF_F_UPPER_DISABLES       (NETIF_F_LRO | NETIF_F_GRO_HW)
>>
>>  /* changeable features with no special hardware requirements */
>>  #define NETIF_F_SOFT_FEATURES        (NETIF_F_GSO | NETIF_F_GRO)
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 30b5fe3..09c2ad0 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -7392,6 +7392,19 @@ static netdev_features_t
>> netdev_fix_features(struct net_device *dev,
>>               features &= ~dev->gso_partial_features;
>>       }
>>
>> +     if (features & NETIF_F_GRO_HW) {
>> +             /* Hardware GRO depends on GRO. */
>> +             if (!(features & NETIF_F_GRO)) {
>
> While at it, perhaps also make it dependent on NETIF_F_RXCSUM?

OK.  Makes sense.

>
>> +                     netdev_dbg(dev, "Dropping NETIF_F_GSO_HW since
>> no GRO feature.\n");
>> +                     features &= ~NETIF_F_GRO_HW;
>> +             }
>> +             /* Hardware GRO and LRO are mutually exclusive. */
>> +             if (features & NETIF_F_LRO) {
>> +                     netdev_dbg(dev, "Dropping NETIF_F_LRO since
>> GRO_HW is set.\n");
>> +                     features &= ~NETIF_F_LRO;
>
> Isn't this considered to be breaking an existing API?
> After this, while NETIF_F_GRO_HW is published an application trying to
> set NETIF_F_LRO and then query its state would discover it failed
> [while previously it could have succeeded, such as for bnx2]
>
> While I understand the need to make sure core doesn't enable
> two competing aggregation offloads, why make GRO_HW > LRO?
> I understand it's probably the better one, but until LRO gets deprecated
> isn't it safer to do this limitation the opposite way?
> I.e., make sure NETIF_F_GRO_HW can't be set as long as NETIF_F_LRO is set?

I am just following precedents in the netdev_fix_features() logic to
drop incompatible features.  I can make LRO and GRO_HW have equal
standing by dropping the other when one is set.  So if I do that, user
will be able to always enable LRO.  The code will just drop GRO_HW if
it is set.