On Mon, Dec 4, 2017 at 2:15 PM, Yuval Mintz <yuv...@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.
