Re: [PATCHv2 net] i40e: Implement ndo_gso_check()

[Jesse Gross]

On Tue, Dec 2, 2014 at 10:26 AM, Jesse Gross <jesse@xxxxxxxxxx> wrote:
> On Mon, Dec 1, 2014 at 4:09 PM, Tom Herbert <therbert@xxxxxxxxxx> wrote:
>> On Mon, Dec 1, 2014 at 3:53 PM, Jesse Gross <jesse@xxxxxxxxxx> wrote:
>>> On Mon, Dec 1, 2014 at 3:47 PM, Tom Herbert <therbert@xxxxxxxxxx> wrote:
>>>> On Mon, Dec 1, 2014 at 3:35 PM, Joe Stringer <joestringer@xxxxxxxxxx> wrote:
>>>>> On 21 November 2014 at 09:59, Joe Stringer <joestringer@xxxxxxxxxx> wrote:
>>>>>> On 20 November 2014 16:19, Jesse Gross <jesse@xxxxxxxxxx> wrote:
>>>>>>> I don't know if we need to have the check at all for IPIP though -
>>>>>>> after all the driver doesn't expose support for it all (actually it
>>>>>>> doesn't expose GRE either). This raises kind of an interesting
>>>>>>> question about the checks though - it's pretty easy to add support to
>>>>>>> the driver for a new GSO type (and I imagine that people will be
>>>>>>> adding GRE soon) and forget to update the check.
>>>>>>
>>>>>> If the check is more conservative, then testing would show that it's
>>>>>> not working and lead people to figure out why (and update the check).
>>>>>
>>>>> More concretely, one suggestion would be something like following at
>>>>> the start of each gso_check():
>>>>>
>>>>> +       const int supported = SKB_GSO_TCPV4 | SKB_GSO_TCPV6 | SKB_GSO_FCOE |
>>>>> +                             SKB_GSO_UDP | SKB_GSO_UDP_TUNNEL;
>>>>> +
>>>>> +       if (skb_shinfo(skb)->gso_type & ~supported)
>>>>> +               return false;
>>>>
>>>> This should already be handled by net_gso_ok.
>>>
>>> My original point wasn't so much that this isn't handled at the moment
>>> but that it's easy to add a supported GSO type but then forget to
>>> update this check - i.e. if a driver already supports UDP_TUNNEL and
>>> adds support for GRE with the same constraints. It seems not entirely
>>> ideal that this function is acting as a blacklist rather than a
>>> whitelist.
>>
>> Agreed, it would be nice to have all the checking logic in one place.
>> If all the drivers end up implementing ndo_gso_check then we could
>> potentially get rid of the GSO types as features. This probably
>> wouldn't be a bad thing since we already know that the features
>> mechanism doesn't scale (for instance there's no way to indicate that
>> certain combinations of GSO types are supported by a device).
>
> This crossed my mind and I agree that it's pretty clear that the
> features mechanism isn't scaling very well. Presumably, the logical
> extension of this is that each driver would have a function that looks
> at a packet and returns a set of offload operations that it can
> support rather than exposing a set of protocols. However, it seems
> like it would probably result in a bunch of duplicate code in each
> driver.

I think a possible middleground here is to convert ndo_gso_check() to
ndo_features_check(). This would behave similarly to
netif_skb_features() and give drivers an opportunity to knock out
features for a given packet. This would allow us to avoid duplicate
code in the immediate case of tunnels where we need to handle both GSO
and checksums and potentially enable wider usage in the future if it
makes sense.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/