ndo_gso_check() was recently introduced to allow NICs to report the
offloading support that they have on a per-skb basis. Add an
implementation for this driver which checks for IPIP, GRE, UDP tunnels.
Signed-off-by: Joe Stringer <[email protected]>
---
v2: Expand to include IP in IP and IPv4/IPv6 inside GRE/UDP tunnels.
Add MAX_INNER_LENGTH (as 80).
---
drivers/net/ethernet/intel/i40e/i40e_main.c | 31 +++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index c3a7f4a..2b01c8d 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -7447,6 +7447,36 @@ static int i40e_ndo_fdb_dump(struct sk_buff *skb,
#endif /* USE_DEFAULT_FDB_DEL_DUMP */
#endif /* HAVE_FDB_OPS */
+static bool i40e_gso_check(struct sk_buff *skb, struct net_device *dev)
+{
+ if ((skb_shinfo(skb)->gso_type & SKB_GSO_IPIP) &&
+ (skb->inner_protocol_type != ENCAP_TYPE_IPPROTO ||
+ skb->inner_protocol != htons(IPPROTO_IPIP))) {
+ return false;
+ }
+
+ if (skb_shinfo(skb)->gso_type & (SKB_GSO_GRE | SKB_GSO_UDP_TUNNEL)) {
+ unsigned char *ihdr;
+
+ if (skb->inner_protocol_type != ENCAP_TYPE_ETHER)
+ return false;
+
+ if (skb->inner_protocol == htons(ETH_P_TEB))
+ ihdr = skb_inner_mac_header(skb);
+ else if (skb->inner_protocol == htons(ETH_P_IP) ||
+ skb->inner_protocol == htons(ETH_P_IPV6))
+ ihdr = skb_inner_network_header(skb);
+ else
+ return false;
+
+#define MAX_TUNNEL_HDR_LEN 80
+ if (ihdr - skb_transport_header(skb) > MAX_TUNNEL_HDR_LEN)
+ return false;
+ }
+
+ return true;
+}
+
static const struct net_device_ops i40e_netdev_ops = {
.ndo_open = i40e_open,
.ndo_stop = i40e_close,
@@ -7487,6 +7517,7 @@ static const struct net_device_ops i40e_netdev_ops = {
.ndo_fdb_dump = i40e_ndo_fdb_dump,
#endif
#endif
+ .ndo_gso_check = i40e_gso_check,
};
/**
--
1.7.10.4
On Thu, 2014-11-20 at 15:11 -0800, Joe Stringer wrote:
> ndo_gso_check() was recently introduced to allow NICs to report the
> offloading support that they have on a per-skb basis. Add an
> implementation for this driver which checks for IPIP, GRE, UDP
> tunnels.
>
> Signed-off-by: Joe Stringer <[email protected]>
> ---
> v2: Expand to include IP in IP and IPv4/IPv6 inside GRE/UDP tunnels.
> Add MAX_INNER_LENGTH (as 80).
> ---
> drivers/net/ethernet/intel/i40e/i40e_main.c | 31
> +++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
If Jesse is good with the updated patch, I will add it to my queue.
Thanks Joe!
On Thu, Nov 20, 2014 at 3:11 PM, Joe Stringer <[email protected]> wrote:
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index c3a7f4a..2b01c8d 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -7447,6 +7447,36 @@ static int i40e_ndo_fdb_dump(struct sk_buff *skb,
>
> #endif /* USE_DEFAULT_FDB_DEL_DUMP */
> #endif /* HAVE_FDB_OPS */
> +static bool i40e_gso_check(struct sk_buff *skb, struct net_device *dev)
> +{
> + if ((skb_shinfo(skb)->gso_type & SKB_GSO_IPIP) &&
> + (skb->inner_protocol_type != ENCAP_TYPE_IPPROTO ||
> + skb->inner_protocol != htons(IPPROTO_IPIP))) {
I think this check on inner_protocol isn't really semantically correct
- that field is a union with inner_ipproto, so it would be more
correct to check against that and then you wouldn't need to use htons
either.
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 (skb_shinfo(skb)->gso_type & (SKB_GSO_GRE | SKB_GSO_UDP_TUNNEL)) {
> + unsigned char *ihdr;
> +
> + if (skb->inner_protocol_type != ENCAP_TYPE_ETHER)
> + return false;
I guess if you want to be nice, it might be good to check the
equivalent IP protocol types as well.
On 20 November 2014 16:19, Jesse Gross <[email protected]> wrote:
> On Thu, Nov 20, 2014 at 3:11 PM, Joe Stringer <[email protected]> wrote:
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
>> index c3a7f4a..2b01c8d 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
>> @@ -7447,6 +7447,36 @@ static int i40e_ndo_fdb_dump(struct sk_buff *skb,
>>
>> #endif /* USE_DEFAULT_FDB_DEL_DUMP */
>> #endif /* HAVE_FDB_OPS */
>> +static bool i40e_gso_check(struct sk_buff *skb, struct net_device *dev)
>> +{
>> + if ((skb_shinfo(skb)->gso_type & SKB_GSO_IPIP) &&
>> + (skb->inner_protocol_type != ENCAP_TYPE_IPPROTO ||
>> + skb->inner_protocol != htons(IPPROTO_IPIP))) {
>
> I think this check on inner_protocol isn't really semantically correct
> - that field is a union with inner_ipproto, so it would be more
> correct to check against that and then you wouldn't need to use htons
> either.
Yes, translation error on my part, but if IPIP isn't exposed I'll just
drop this section.
> 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).
>> + if (skb_shinfo(skb)->gso_type & (SKB_GSO_GRE | SKB_GSO_UDP_TUNNEL)) {
>> + unsigned char *ihdr;
>> +
>> + if (skb->inner_protocol_type != ENCAP_TYPE_ETHER)
>> + return false;
>
> I guess if you want to be nice, it might be good to check the
> equivalent IP protocol types as well.
Can do (just UDP as I'll drop GRE as per the above).
On 21 November 2014 at 09:59, Joe Stringer <[email protected]> wrote:
> On 20 November 2014 16:19, Jesse Gross <[email protected]> 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;
..followed by checking the specifics for each. So far the patches have
only been concerned with further checking on UDP tunnels.
On Mon, Dec 1, 2014 at 3:35 PM, Joe Stringer <[email protected]> wrote:
> On 21 November 2014 at 09:59, Joe Stringer <[email protected]> wrote:
>> On 20 November 2014 16:19, Jesse Gross <[email protected]> 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.
>
> ..followed by checking the specifics for each. So far the patches have
> only been concerned with further checking on UDP tunnels.
On Mon, Dec 1, 2014 at 3:47 PM, Tom Herbert <[email protected]> wrote:
> On Mon, Dec 1, 2014 at 3:35 PM, Joe Stringer <[email protected]> wrote:
>> On 21 November 2014 at 09:59, Joe Stringer <[email protected]> wrote:
>>> On 20 November 2014 16:19, Jesse Gross <[email protected]> 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.
On 1 December 2014 at 15:53, Jesse Gross <[email protected]> wrote:
> On Mon, Dec 1, 2014 at 3:47 PM, Tom Herbert <[email protected]> wrote:
>> On Mon, Dec 1, 2014 at 3:35 PM, Joe Stringer <[email protected]> wrote:
>>> On 21 November 2014 at 09:59, Joe Stringer <[email protected]> wrote:
>>>> On 20 November 2014 16:19, Jesse Gross <[email protected]> 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.
How much less ideal is it to forget to update the check than to make
this a blacklist?
On Mon, Dec 1, 2014 at 3:53 PM, Jesse Gross <[email protected]> wrote:
> On Mon, Dec 1, 2014 at 3:47 PM, Tom Herbert <[email protected]> wrote:
>> On Mon, Dec 1, 2014 at 3:35 PM, Joe Stringer <[email protected]> wrote:
>>> On 21 November 2014 at 09:59, Joe Stringer <[email protected]> wrote:
>>>> On 20 November 2014 16:19, Jesse Gross <[email protected]> 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).
On Mon, Dec 1, 2014 at 4:09 PM, Tom Herbert <[email protected]> wrote:
> On Mon, Dec 1, 2014 at 3:53 PM, Jesse Gross <[email protected]> wrote:
>> On Mon, Dec 1, 2014 at 3:47 PM, Tom Herbert <[email protected]> wrote:
>>> On Mon, Dec 1, 2014 at 3:35 PM, Joe Stringer <[email protected]> wrote:
>>>> On 21 November 2014 at 09:59, Joe Stringer <[email protected]> wrote:
>>>>> On 20 November 2014 16:19, Jesse Gross <[email protected]> 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.
On 2 December 2014 at 10:26, Jesse Gross <[email protected]> wrote:
> On Mon, Dec 1, 2014 at 4:09 PM, Tom Herbert <[email protected]> wrote:
>> On Mon, Dec 1, 2014 at 3:53 PM, Jesse Gross <[email protected]> wrote:
>>> On Mon, Dec 1, 2014 at 3:47 PM, Tom Herbert <[email protected]> wrote:
>>>> On Mon, Dec 1, 2014 at 3:35 PM, Joe Stringer <[email protected]> wrote:
>>>>> On 21 November 2014 at 09:59, Joe Stringer <[email protected]> wrote:
>>>>>> On 20 November 2014 16:19, Jesse Gross <[email protected]> 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.
Given the discussion is still pretty open-ended, I've made the basic
feedback changes for v3 and haven't tried to address the concern about
forgetting to update this check when a driver adds support.
On Tue, Dec 2, 2014 at 10:26 AM, Jesse Gross <[email protected]> wrote:
> On Mon, Dec 1, 2014 at 4:09 PM, Tom Herbert <[email protected]> wrote:
>> On Mon, Dec 1, 2014 at 3:53 PM, Jesse Gross <[email protected]> wrote:
>>> On Mon, Dec 1, 2014 at 3:47 PM, Tom Herbert <[email protected]> wrote:
>>>> On Mon, Dec 1, 2014 at 3:35 PM, Joe Stringer <[email protected]> wrote:
>>>>> On 21 November 2014 at 09:59, Joe Stringer <[email protected]> wrote:
>>>>>> On 20 November 2014 16:19, Jesse Gross <[email protected]> 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.