Message-ID: <099147dd-555a-4f3b-bd75-3a3dcd7d942e@redhat.com>
Date: Tue, 25 Feb 2025 09:55:37 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Alexander Duyck <alexander.duyck@...il.com>,
 Tony Nguyen <anthony.l.nguyen@...el.com>, Joe Damato <jdamato@...tly.com>
Cc: Mengyuan Lou <mengyuanlou@...-swift.com>, jiawenwu@...stnetic.com,
 netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
 Eric Dumazet <edumazet@...gle.com>, Willem de Bruijn <willemb@...gle.com>
Subject: Re: [RESEND,PATCH net-next v7 7/8] net: txgbe: Add netdev features
 support

Adding Joe, which authored a change possibly relevant here.

On 2/25/25 12:59 AM, Alexander Duyck wrote:
> On Mon, Feb 24, 2025 at 2:17 PM Paolo Abeni <pabeni@...hat.com> wrote:
>> I just stumbled upon the following while working on something slightly
>> related. Adding a bunch of people hopefully interested.
>>
>> On 5/30/23 4:26 AM, Mengyuan Lou wrote:
>>> Add features and hw_features that ngbe can support.
>>>
>>> Signed-off-by: Mengyuan Lou <mengyuanlou@...-swift.com>
>>> ---
>>> @@ -596,11 +597,25 @@ static int txgbe_probe(struct pci_dev *pdev,
>>>               goto err_free_mac_table;
>>>       }
>>>
>>> -     netdev->features |= NETIF_F_HIGHDMA;
>>> -     netdev->features = NETIF_F_SG;
>>> -
>>> +     netdev->features = NETIF_F_SG |
>>> +                        NETIF_F_TSO |
>>> +                        NETIF_F_TSO6 |
>>> +                        NETIF_F_RXHASH |
>>> +                        NETIF_F_RXCSUM |
>>> +                        NETIF_F_HW_CSUM;
>>> +
>>> +     netdev->gso_partial_features =  NETIF_F_GSO_ENCAP_ALL;
>>> +     netdev->features |= netdev->gso_partial_features;
>>> +     netdev->features |= NETIF_F_SCTP_CRC;
>>> +     netdev->vlan_features |= netdev->features | NETIF_F_TSO_MANGLEID;
>>> +     netdev->hw_enc_features |= netdev->vlan_features;
>>
>> This driver does not implement the .ndo_features_check callback, meaning
>> it will happily accept TSO_V4 over any IP tunnel, even when ID mangling
>> is explicitly not allowed.
>>
>> The above in turn looks inconsistent. If the device is able to update
>> the (outer) IP (and IP csum) while performing TSO, than it should be
>> able to fully offload NETIF_F_GSO_GRE: such offload should not be
>> included in the partial ones.
>>
>> Otherwise, if the device is not able to perform the mentioned tasks, the
>> driver should implement a suitable ndo_features_check op, stripping
>> NETIF_F_TSO for tunneled packet that could be potentially fragmented,
>> that is, when `features` lacks the `NETIF_F_TSO_MANGLEID` bit.
>>
>> Alike what several intel drivers (ixgbe, igc, etc.) do.
>>
>> Assuming I did not misread something relevant, and the above is somewhat
>> correct, I'm wondering if we should move the mentioned check into the
>> core; preventing future similar errors and avoiding some driver code
>> duplication.
>>
>> Something alike the following, completely untested:
>> ---
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index d5ab9a4b318e..2fdfcddf9c3b 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -3668,15 +3668,6 @@ static netdev_features_t gso_features_check(const
>> struct sk_buff *skb,
>>                 return features & ~NETIF_F_GSO_MASK;
>>         }
>>
>> -       /* Support for GSO partial features requires software
>> -        * intervention before we can actually process the packets
>> -        * so we need to strip support for any partial features now
>> -        * and we can pull them back in after we have partially
>> -        * segmented the frame.
>> -        */
>> -       if (!(skb_shinfo(skb)->gso_type & SKB_GSO_PARTIAL))
>> -               features &= ~dev->gso_partial_features;
>> -
>>         /* Make sure to clear the IPv4 ID mangling feature if the
>>          * IPv4 header has the potential to be fragmented.
>>          */
>> @@ -3684,10 +3675,24 @@ static netdev_features_t
>> gso_features_check(const struct sk_buff *skb,
>>                 struct iphdr *iph = skb->encapsulation ?
>>                                     inner_ip_hdr(skb) : ip_hdr(skb);
>>
>> -               if (!(iph->frag_off & htons(IP_DF)))
>> +               if (!(iph->frag_off & htons(IP_DF))) {
>>                         features &= ~NETIF_F_TSO_MANGLEID;
>> +                       if (features & dev->gso_partial_features &
>> +                           (NETIF_F_GSO_GRE | NETIF_F_GSO_IPXIP4 |
>> +                            NETIF_F_GSO_IPXIP6 | NETIF_F_GSO_UDP_TUNNEL))
>> +                               features &= ~NETIF_F_TSO;
>> +               }
>>         }
>>
> 
> My main concern would be that there might be implementations of
> partial that are capable of doing a partial offload without the
> MANGLEID feature that would be impacted.
> 
> If I recall I think for the i40e it may have supported some logic to
> do the proper things while not knowing how to deal with the tunnel
> headers but correctly handling the IP headers.

Thanks for the pointer, I missed the i40e case. It's not clear to me how
such driver handles the special case.

If I understand correctly, such NIC is able to touch inner and outer IP
id and csum correctly, without being able to parse the encap header. If
so, I guess it needs explicit notion of the inner and outer header
offsets, provided by the driver.

But I don't see the driver passing to the H/W the offsets, so possibly
the current code is actually bugged for this specific case? What am I
missing here?

If i40e can handle the IP headers correctly, why it need the partial
feature for NETIF_F_GSO_GRE? Perhaps commit b4fb2d33514 ("i40e: Add
support for MPLS + TSO") increased the partial features set too
broadly?!? It looks like originally none of
NETIF_F_GSO_UDP_TUNNEL, NETIF_F_GSO_GRE, NETIF_F_GSO_IPXIP4,
NETIF_F_GSO_IPXIP6
belonged to the partial features set.

>> +       /* Support for GSO partial features requires software
>> +        * intervention before we can actually process the packets
>> +        * so we need to strip support for any partial features now
>> +        * and we can pull them back in after we have partially
>> +        * segmented the frame.
>> +        */
>> +       if (!(skb_shinfo(skb)->gso_type & SKB_GSO_PARTIAL))
>> +               features &= ~dev->gso_partial_features;
>> +
> 
> Why move this? I'm not sure it gains you anything if it is either
> before or after the result should be the same.

I'm sorry, I rushed the code out-of-the-door to try to get some
feedback; I moved that chunk so that in the previous block `features`
still contains the partials bits and the code can check for tunnels
looking at `features` contents.

Thinking again about it, it's not needed, as we could (and should) check
instead the skb wanted features (skb_shinfo(skb)->gso_type <<
NETIF_F_GSO_SHIFT).

Thanks,

Paolo

Powered by blists - more mailing lists