* [PATCH net-next] gso: enable udp gso for virtual devices
@ 2019-06-12 23:12 Jason Baron
2019-06-13 17:15 ` Alexander Duyck
0 siblings, 1 reply; 9+ messages in thread
From: Jason Baron @ 2019-06-12 23:12 UTC (permalink / raw)
To: davem; +Cc: netdev, Joshua Hunt, Alexander Duyck, Willem de Bruijn, Paolo Abeni
Now that the stack supports UDP GRO, we can enable udp gso for virtual
devices. If packets are looped back locally, and UDP GRO is not enabled
then they will be segmented to gso_size via udp_rcv_segment(). This
essentiallly just reverts: 8eea1ca gso: limit udp gso to egress-only
virtual devices.
Tested by connecting two namespaces via macvlan and then ran
udpgso_bench_tx:
before:
udp tx: 2068 MB/s 35085 calls/s 35085 msg/s
after (no UDP_GRO):
udp tx: 3438 MB/s 58319 calls/s 58319 msg/s
after (UDP_GRO):
udp tx: 8037 MB/s 136314 calls/s 136314 msg/s
Signed-off-by: Jason Baron <jbaron@akamai.com>
Co-developed-by: Joshua Hunt <johunt@akamai.com>
Signed-off-by: Joshua Hunt <johunt@akamai.com>
Cc: Alexander Duyck <alexander.duyck@gmail.com>
Cc: Willem de Bruijn <willemb@google.com>
Cc: Paolo Abeni <pabeni@redhat.com>
---
drivers/net/bonding/bond_main.c | 5 ++---
drivers/net/team/team.c | 5 ++---
include/linux/netdev_features.h | 1 +
3 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 4f5b3ba..c4260be 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1120,8 +1120,7 @@ static void bond_compute_features(struct bonding *bond)
done:
bond_dev->vlan_features = vlan_features;
- bond_dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL |
- NETIF_F_GSO_UDP_L4;
+ bond_dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL;
bond_dev->mpls_features = mpls_features;
bond_dev->gso_max_segs = gso_max_segs;
netif_set_gso_max_size(bond_dev, gso_max_size);
@@ -4308,7 +4307,7 @@ void bond_setup(struct net_device *bond_dev)
NETIF_F_HW_VLAN_CTAG_RX |
NETIF_F_HW_VLAN_CTAG_FILTER;
- bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4;
+ bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL;
bond_dev->features |= bond_dev->hw_features;
}
diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index b48006e..30299e3 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -1003,8 +1003,7 @@ static void __team_compute_features(struct team *team)
}
team->dev->vlan_features = vlan_features;
- team->dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL |
- NETIF_F_GSO_UDP_L4;
+ team->dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL;
team->dev->hard_header_len = max_hard_header_len;
team->dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
@@ -2132,7 +2131,7 @@ static void team_setup(struct net_device *dev)
NETIF_F_HW_VLAN_CTAG_RX |
NETIF_F_HW_VLAN_CTAG_FILTER;
- dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4;
+ dev->hw_features |= NETIF_F_GSO_ENCAP_ALL;
dev->features |= dev->hw_features;
}
diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 4b19c54..188127c 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -237,6 +237,7 @@ static inline int find_next_netdev_feature(u64 feature, unsigned long start)
NETIF_F_GSO_GRE_CSUM | \
NETIF_F_GSO_IPXIP4 | \
NETIF_F_GSO_IPXIP6 | \
+ NETIF_F_GSO_UDP_L4 | \
NETIF_F_GSO_UDP_TUNNEL | \
NETIF_F_GSO_UDP_TUNNEL_CSUM)
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] gso: enable udp gso for virtual devices
2019-06-12 23:12 [PATCH net-next] gso: enable udp gso for virtual devices Jason Baron
@ 2019-06-13 17:15 ` Alexander Duyck
2019-06-13 19:03 ` Jason Baron
0 siblings, 1 reply; 9+ messages in thread
From: Alexander Duyck @ 2019-06-13 17:15 UTC (permalink / raw)
To: Jason Baron
Cc: David Miller, Netdev, Joshua Hunt, Willem de Bruijn, Paolo Abeni
On Wed, Jun 12, 2019 at 4:14 PM Jason Baron <jbaron@akamai.com> wrote:
>
> Now that the stack supports UDP GRO, we can enable udp gso for virtual
> devices. If packets are looped back locally, and UDP GRO is not enabled
> then they will be segmented to gso_size via udp_rcv_segment(). This
> essentiallly just reverts: 8eea1ca gso: limit udp gso to egress-only
> virtual devices.
>
> Tested by connecting two namespaces via macvlan and then ran
> udpgso_bench_tx:
>
> before:
> udp tx: 2068 MB/s 35085 calls/s 35085 msg/s
>
> after (no UDP_GRO):
> udp tx: 3438 MB/s 58319 calls/s 58319 msg/s
>
> after (UDP_GRO):
> udp tx: 8037 MB/s 136314 calls/s 136314 msg/s
>
> Signed-off-by: Jason Baron <jbaron@akamai.com>
> Co-developed-by: Joshua Hunt <johunt@akamai.com>
> Signed-off-by: Joshua Hunt <johunt@akamai.com>
> Cc: Alexander Duyck <alexander.duyck@gmail.com>
> Cc: Willem de Bruijn <willemb@google.com>
> Cc: Paolo Abeni <pabeni@redhat.com>
> ---
> drivers/net/bonding/bond_main.c | 5 ++---
> drivers/net/team/team.c | 5 ++---
> include/linux/netdev_features.h | 1 +
> 3 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 4f5b3ba..c4260be 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1120,8 +1120,7 @@ static void bond_compute_features(struct bonding *bond)
>
> done:
> bond_dev->vlan_features = vlan_features;
> - bond_dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL |
> - NETIF_F_GSO_UDP_L4;
> + bond_dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL;
> bond_dev->mpls_features = mpls_features;
> bond_dev->gso_max_segs = gso_max_segs;
> netif_set_gso_max_size(bond_dev, gso_max_size);
> @@ -4308,7 +4307,7 @@ void bond_setup(struct net_device *bond_dev)
> NETIF_F_HW_VLAN_CTAG_RX |
> NETIF_F_HW_VLAN_CTAG_FILTER;
>
> - bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4;
> + bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL;
> bond_dev->features |= bond_dev->hw_features;
> }
>
> diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
> index b48006e..30299e3 100644
> --- a/drivers/net/team/team.c
> +++ b/drivers/net/team/team.c
> @@ -1003,8 +1003,7 @@ static void __team_compute_features(struct team *team)
> }
>
> team->dev->vlan_features = vlan_features;
> - team->dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL |
> - NETIF_F_GSO_UDP_L4;
> + team->dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL;
> team->dev->hard_header_len = max_hard_header_len;
>
> team->dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
> @@ -2132,7 +2131,7 @@ static void team_setup(struct net_device *dev)
> NETIF_F_HW_VLAN_CTAG_RX |
> NETIF_F_HW_VLAN_CTAG_FILTER;
>
> - dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4;
> + dev->hw_features |= NETIF_F_GSO_ENCAP_ALL;
> dev->features |= dev->hw_features;
> }
>
> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
> index 4b19c54..188127c 100644
> --- a/include/linux/netdev_features.h
> +++ b/include/linux/netdev_features.h
> @@ -237,6 +237,7 @@ static inline int find_next_netdev_feature(u64 feature, unsigned long start)
> NETIF_F_GSO_GRE_CSUM | \
> NETIF_F_GSO_IPXIP4 | \
> NETIF_F_GSO_IPXIP6 | \
> + NETIF_F_GSO_UDP_L4 | \
> NETIF_F_GSO_UDP_TUNNEL | \
> NETIF_F_GSO_UDP_TUNNEL_CSUM)
Are you adding this to NETIF_F_GSO_ENCAP_ALL? Wouldn't it make more
sense to add it to NETIF_F_GSO_SOFTWARE?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] gso: enable udp gso for virtual devices
2019-06-13 17:15 ` Alexander Duyck
@ 2019-06-13 19:03 ` Jason Baron
2019-06-13 21:20 ` Willem de Bruijn
0 siblings, 1 reply; 9+ messages in thread
From: Jason Baron @ 2019-06-13 19:03 UTC (permalink / raw)
To: Alexander Duyck
Cc: David Miller, Netdev, Joshua Hunt, Willem de Bruijn, Paolo Abeni
On 6/13/19 1:15 PM, Alexander Duyck wrote:
> On Wed, Jun 12, 2019 at 4:14 PM Jason Baron <jbaron@akamai.com> wrote:
>>
>> Now that the stack supports UDP GRO, we can enable udp gso for virtual
>> devices. If packets are looped back locally, and UDP GRO is not enabled
>> then they will be segmented to gso_size via udp_rcv_segment(). This
>> essentiallly just reverts: 8eea1ca gso: limit udp gso to egress-only
>> virtual devices.
>>
>> Tested by connecting two namespaces via macvlan and then ran
>> udpgso_bench_tx:
>>
>> before:
>> udp tx: 2068 MB/s 35085 calls/s 35085 msg/s
>>
>> after (no UDP_GRO):
>> udp tx: 3438 MB/s 58319 calls/s 58319 msg/s
>>
>> after (UDP_GRO):
>> udp tx: 8037 MB/s 136314 calls/s 136314 msg/s
>>
>> Signed-off-by: Jason Baron <jbaron@akamai.com>
>> Co-developed-by: Joshua Hunt <johunt@akamai.com>
>> Signed-off-by: Joshua Hunt <johunt@akamai.com>
>> Cc: Alexander Duyck <alexander.duyck@gmail.com>
>> Cc: Willem de Bruijn <willemb@google.com>
>> Cc: Paolo Abeni <pabeni@redhat.com>
>> ---
>> drivers/net/bonding/bond_main.c | 5 ++---
>> drivers/net/team/team.c | 5 ++---
>> include/linux/netdev_features.h | 1 +
>> 3 files changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 4f5b3ba..c4260be 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1120,8 +1120,7 @@ static void bond_compute_features(struct bonding *bond)
>>
>> done:
>> bond_dev->vlan_features = vlan_features;
>> - bond_dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL |
>> - NETIF_F_GSO_UDP_L4;
>> + bond_dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL;
>> bond_dev->mpls_features = mpls_features;
>> bond_dev->gso_max_segs = gso_max_segs;
>> netif_set_gso_max_size(bond_dev, gso_max_size);
>> @@ -4308,7 +4307,7 @@ void bond_setup(struct net_device *bond_dev)
>> NETIF_F_HW_VLAN_CTAG_RX |
>> NETIF_F_HW_VLAN_CTAG_FILTER;
>>
>> - bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4;
>> + bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL;
>> bond_dev->features |= bond_dev->hw_features;
>> }
>>
>> diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
>> index b48006e..30299e3 100644
>> --- a/drivers/net/team/team.c
>> +++ b/drivers/net/team/team.c
>> @@ -1003,8 +1003,7 @@ static void __team_compute_features(struct team *team)
>> }
>>
>> team->dev->vlan_features = vlan_features;
>> - team->dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL |
>> - NETIF_F_GSO_UDP_L4;
>> + team->dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL;
>> team->dev->hard_header_len = max_hard_header_len;
>>
>> team->dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
>> @@ -2132,7 +2131,7 @@ static void team_setup(struct net_device *dev)
>> NETIF_F_HW_VLAN_CTAG_RX |
>> NETIF_F_HW_VLAN_CTAG_FILTER;
>>
>> - dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4;
>> + dev->hw_features |= NETIF_F_GSO_ENCAP_ALL;
>> dev->features |= dev->hw_features;
>> }
>>
>> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
>> index 4b19c54..188127c 100644
>> --- a/include/linux/netdev_features.h
>> +++ b/include/linux/netdev_features.h
>> @@ -237,6 +237,7 @@ static inline int find_next_netdev_feature(u64 feature, unsigned long start)
>> NETIF_F_GSO_GRE_CSUM | \
>> NETIF_F_GSO_IPXIP4 | \
>> NETIF_F_GSO_IPXIP6 | \
>> + NETIF_F_GSO_UDP_L4 | \
>> NETIF_F_GSO_UDP_TUNNEL | \
>> NETIF_F_GSO_UDP_TUNNEL_CSUM)
>
> Are you adding this to NETIF_F_GSO_ENCAP_ALL? Wouldn't it make more
> sense to add it to NETIF_F_GSO_SOFTWARE?
>
Yes, I'm adding to NETIF_F_GSO_ENCAP_ALL (not very clear from the
context). I will fix the commit log.
In: 83aa025 udp: add gso support to virtual devices, the support was
also added to NETIF_F_GSO_ENCAP_ALL (although subsequently reverted due
to UDP GRO not being in place), so I wonder what the reason was for that?
I agree that NETIF_F_GSO_SOFTWARE seems conceptually more logical and
further I think it adds support for more 'virtual' devices. For example,
I tested loopback with NETIF_F_GSO_UDP_L4 being added to
NETIF_F_GSO_SOFTWARE and it shows a nice performance gain, whereas
NETIF_F_GSO_ENCAP_ALL isn't included for loopback.
Thanks,
-Jason
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] gso: enable udp gso for virtual devices
2019-06-13 19:03 ` Jason Baron
@ 2019-06-13 21:20 ` Willem de Bruijn
2019-06-14 20:53 ` Jason Baron
0 siblings, 1 reply; 9+ messages in thread
From: Willem de Bruijn @ 2019-06-13 21:20 UTC (permalink / raw)
To: Jason Baron
Cc: Alexander Duyck, David Miller, Netdev, Joshua Hunt,
Willem de Bruijn, Paolo Abeni
> >> @@ -237,6 +237,7 @@ static inline int find_next_netdev_feature(u64 feature, unsigned long start)
> >> NETIF_F_GSO_GRE_CSUM | \
> >> NETIF_F_GSO_IPXIP4 | \
> >> NETIF_F_GSO_IPXIP6 | \
> >> + NETIF_F_GSO_UDP_L4 | \
> >> NETIF_F_GSO_UDP_TUNNEL | \
> >> NETIF_F_GSO_UDP_TUNNEL_CSUM)
> >
> > Are you adding this to NETIF_F_GSO_ENCAP_ALL? Wouldn't it make more
> > sense to add it to NETIF_F_GSO_SOFTWARE?
> >
>
> Yes, I'm adding to NETIF_F_GSO_ENCAP_ALL (not very clear from the
> context). I will fix the commit log.
>
> In: 83aa025 udp: add gso support to virtual devices, the support was
> also added to NETIF_F_GSO_ENCAP_ALL (although subsequently reverted due
> to UDP GRO not being in place), so I wonder what the reason was for that?
That was probably just a bad choice on my part.
It worked in practice, but if NETIF_F_GSO_SOFTWARE works the same
without unexpected side effects, then I agree that it is the better choice.
That choice does appear to change behavior when sending over tunnel
devices. Might it send tunneled GSO packets over loopback?
> I agree that NETIF_F_GSO_SOFTWARE seems conceptually more logical and
> further I think it adds support for more 'virtual' devices. For example,
> I tested loopback with NETIF_F_GSO_UDP_L4 being added to
> NETIF_F_GSO_SOFTWARE and it shows a nice performance gain, whereas
> NETIF_F_GSO_ENCAP_ALL isn't included for loopback.
>
> Thanks,
>
> -Jason
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] gso: enable udp gso for virtual devices
2019-06-13 21:20 ` Willem de Bruijn
@ 2019-06-14 20:53 ` Jason Baron
2019-06-26 19:15 ` Jason Baron
0 siblings, 1 reply; 9+ messages in thread
From: Jason Baron @ 2019-06-14 20:53 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Alexander Duyck, David Miller, Netdev, Joshua Hunt,
Willem de Bruijn, Paolo Abeni
On 6/13/19 5:20 PM, Willem de Bruijn wrote:
>>>> @@ -237,6 +237,7 @@ static inline int find_next_netdev_feature(u64 feature, unsigned long start)
>>>> NETIF_F_GSO_GRE_CSUM | \
>>>> NETIF_F_GSO_IPXIP4 | \
>>>> NETIF_F_GSO_IPXIP6 | \
>>>> + NETIF_F_GSO_UDP_L4 | \
>>>> NETIF_F_GSO_UDP_TUNNEL | \
>>>> NETIF_F_GSO_UDP_TUNNEL_CSUM)
>>>
>>> Are you adding this to NETIF_F_GSO_ENCAP_ALL? Wouldn't it make more
>>> sense to add it to NETIF_F_GSO_SOFTWARE?
>>>
>>
>> Yes, I'm adding to NETIF_F_GSO_ENCAP_ALL (not very clear from the
>> context). I will fix the commit log.
>>
>> In: 83aa025 udp: add gso support to virtual devices, the support was
>> also added to NETIF_F_GSO_ENCAP_ALL (although subsequently reverted due
>> to UDP GRO not being in place), so I wonder what the reason was for that?
>
> That was probably just a bad choice on my part.
>
> It worked in practice, but if NETIF_F_GSO_SOFTWARE works the same
> without unexpected side effects, then I agree that it is the better choice.
>
> That choice does appear to change behavior when sending over tunnel
> devices. Might it send tunneled GSO packets over loopback?
>
>
I set up a test case using fou tunneling through a bridge device using
the udpgso_bench_tx test where packets are not received correctly if
NETIF_F_GSO_UDP_L4 is added to NETIF_F_GSO_SOFTWARE. If I have it added
to NETIF_F_GSO_ENCAP_ALL, it does work correctly. So there are more
fixes required to include it in NETIF_F_GSO_SOFTWARE.
The use-case I have only requires it to be in NETIF_F_GSO_ENCAP_ALL, but
if it needs to go in NETIF_F_GSO_SOFTWARE, I can look at what's required
more next week.
Thanks,
-Jason
>
>> I agree that NETIF_F_GSO_SOFTWARE seems conceptually more logical and
>> further I think it adds support for more 'virtual' devices. For example,
>> I tested loopback with NETIF_F_GSO_UDP_L4 being added to
>> NETIF_F_GSO_SOFTWARE and it shows a nice performance gain, whereas
>> NETIF_F_GSO_ENCAP_ALL isn't included for loopback.
>>
>> Thanks,
>>
>> -Jason
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] gso: enable udp gso for virtual devices
2019-06-14 20:53 ` Jason Baron
@ 2019-06-26 19:15 ` Jason Baron
2019-06-26 23:41 ` Willem de Bruijn
0 siblings, 1 reply; 9+ messages in thread
From: Jason Baron @ 2019-06-26 19:15 UTC (permalink / raw)
To: Willem de Bruijn, Alexander Duyck
Cc: David Miller, Netdev, Joshua Hunt, Willem de Bruijn, Paolo Abeni
On 6/14/19 4:53 PM, Jason Baron wrote:
>
>
> On 6/13/19 5:20 PM, Willem de Bruijn wrote:
>>>>> @@ -237,6 +237,7 @@ static inline int find_next_netdev_feature(u64 feature, unsigned long start)
>>>>> NETIF_F_GSO_GRE_CSUM | \
>>>>> NETIF_F_GSO_IPXIP4 | \
>>>>> NETIF_F_GSO_IPXIP6 | \
>>>>> + NETIF_F_GSO_UDP_L4 | \
>>>>> NETIF_F_GSO_UDP_TUNNEL | \
>>>>> NETIF_F_GSO_UDP_TUNNEL_CSUM)
>>>>
>>>> Are you adding this to NETIF_F_GSO_ENCAP_ALL? Wouldn't it make more
>>>> sense to add it to NETIF_F_GSO_SOFTWARE?
>>>>
>>>
>>> Yes, I'm adding to NETIF_F_GSO_ENCAP_ALL (not very clear from the
>>> context). I will fix the commit log.
>>>
>>> In: 83aa025 udp: add gso support to virtual devices, the support was
>>> also added to NETIF_F_GSO_ENCAP_ALL (although subsequently reverted due
>>> to UDP GRO not being in place), so I wonder what the reason was for that?
>>
>> That was probably just a bad choice on my part.
>>
>> It worked in practice, but if NETIF_F_GSO_SOFTWARE works the same
>> without unexpected side effects, then I agree that it is the better choice.
>>
>> That choice does appear to change behavior when sending over tunnel
>> devices. Might it send tunneled GSO packets over loopback?
>>
>>
>
> I set up a test case using fou tunneling through a bridge device using
> the udpgso_bench_tx test where packets are not received correctly if
> NETIF_F_GSO_UDP_L4 is added to NETIF_F_GSO_SOFTWARE. If I have it added
> to NETIF_F_GSO_ENCAP_ALL, it does work correctly. So there are more
> fixes required to include it in NETIF_F_GSO_SOFTWARE.
>
> The use-case I have only requires it to be in NETIF_F_GSO_ENCAP_ALL, but
> if it needs to go in NETIF_F_GSO_SOFTWARE, I can look at what's required
> more next week.
>
Hi,
I haven't had a chance to investigate what goes wrong with including
NETIF_F_GSO_UDP_L4 in NETIF_F_GSO_SOFTWARE - but I was just wondering if
people are ok with NETIF_F_GSO_UDP_L4 being added to
NETIF_F_GSO_ENCAP_ALL and not NETIF_F_GSO_SOFTWARE (ie the original
patch as posted)?
As I mentioned that is sufficient for my use-case, and its how Willem
originally proposed this.
Thanks,
-Jason
> Thanks,
>
> -Jason
>
>>
>>> I agree that NETIF_F_GSO_SOFTWARE seems conceptually more logical and
>>> further I think it adds support for more 'virtual' devices. For example,
>>> I tested loopback with NETIF_F_GSO_UDP_L4 being added to
>>> NETIF_F_GSO_SOFTWARE and it shows a nice performance gain, whereas
>>> NETIF_F_GSO_ENCAP_ALL isn't included for loopback.
>>>
>>> Thanks,
>>>
>>> -Jason
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] gso: enable udp gso for virtual devices
2019-06-26 19:15 ` Jason Baron
@ 2019-06-26 23:41 ` Willem de Bruijn
2019-08-09 18:58 ` Josh Hunt
0 siblings, 1 reply; 9+ messages in thread
From: Willem de Bruijn @ 2019-06-26 23:41 UTC (permalink / raw)
To: Jason Baron
Cc: Alexander Duyck, David Miller, Netdev, Joshua Hunt,
Willem de Bruijn, Paolo Abeni
On Wed, Jun 26, 2019 at 3:17 PM Jason Baron <jbaron@akamai.com> wrote:
>
>
>
> On 6/14/19 4:53 PM, Jason Baron wrote:
> >
> >
> > On 6/13/19 5:20 PM, Willem de Bruijn wrote:
> >>>>> @@ -237,6 +237,7 @@ static inline int find_next_netdev_feature(u64 feature, unsigned long start)
> >>>>> NETIF_F_GSO_GRE_CSUM | \
> >>>>> NETIF_F_GSO_IPXIP4 | \
> >>>>> NETIF_F_GSO_IPXIP6 | \
> >>>>> + NETIF_F_GSO_UDP_L4 | \
> >>>>> NETIF_F_GSO_UDP_TUNNEL | \
> >>>>> NETIF_F_GSO_UDP_TUNNEL_CSUM)
> >>>>
> >>>> Are you adding this to NETIF_F_GSO_ENCAP_ALL? Wouldn't it make more
> >>>> sense to add it to NETIF_F_GSO_SOFTWARE?
> >>>>
> >>>
> >>> Yes, I'm adding to NETIF_F_GSO_ENCAP_ALL (not very clear from the
> >>> context). I will fix the commit log.
> >>>
> >>> In: 83aa025 udp: add gso support to virtual devices, the support was
> >>> also added to NETIF_F_GSO_ENCAP_ALL (although subsequently reverted due
> >>> to UDP GRO not being in place), so I wonder what the reason was for that?
> >>
> >> That was probably just a bad choice on my part.
> >>
> >> It worked in practice, but if NETIF_F_GSO_SOFTWARE works the same
> >> without unexpected side effects, then I agree that it is the better choice.
> >>
> >> That choice does appear to change behavior when sending over tunnel
> >> devices. Might it send tunneled GSO packets over loopback?
> >>
> >>
> >
> > I set up a test case using fou tunneling through a bridge device using
> > the udpgso_bench_tx test where packets are not received correctly if
> > NETIF_F_GSO_UDP_L4 is added to NETIF_F_GSO_SOFTWARE. If I have it added
> > to NETIF_F_GSO_ENCAP_ALL, it does work correctly. So there are more
> > fixes required to include it in NETIF_F_GSO_SOFTWARE.
> >
> > The use-case I have only requires it to be in NETIF_F_GSO_ENCAP_ALL, but
> > if it needs to go in NETIF_F_GSO_SOFTWARE, I can look at what's required
> > more next week.
> >
>
> Hi,
>
> I haven't had a chance to investigate what goes wrong with including
> NETIF_F_GSO_UDP_L4 in NETIF_F_GSO_SOFTWARE - but I was just wondering if
> people are ok with NETIF_F_GSO_UDP_L4 being added to
> NETIF_F_GSO_ENCAP_ALL and not NETIF_F_GSO_SOFTWARE (ie the original
> patch as posted)?
>
> As I mentioned that is sufficient for my use-case, and its how Willem
> originally proposed this.
Indeed, based on the previous discussion this sounds fine to me.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] gso: enable udp gso for virtual devices
2019-06-26 23:41 ` Willem de Bruijn
@ 2019-08-09 18:58 ` Josh Hunt
2019-08-09 19:13 ` Willem de Bruijn
0 siblings, 1 reply; 9+ messages in thread
From: Josh Hunt @ 2019-08-09 18:58 UTC (permalink / raw)
To: Willem de Bruijn, Jason Baron
Cc: Alexander Duyck, David Miller, Netdev, Willem de Bruijn, Paolo Abeni
On 6/26/19 4:41 PM, Willem de Bruijn wrote:
> On Wed, Jun 26, 2019 at 3:17 PM Jason Baron <jbaron@akamai.com> wrote:
>>
>>
>>
>> On 6/14/19 4:53 PM, Jason Baron wrote:
>>>
>>>
>>> On 6/13/19 5:20 PM, Willem de Bruijn wrote:
>>>>>>> @@ -237,6 +237,7 @@ static inline int find_next_netdev_feature(u64 feature, unsigned long start)
>>>>>>> NETIF_F_GSO_GRE_CSUM | \
>>>>>>> NETIF_F_GSO_IPXIP4 | \
>>>>>>> NETIF_F_GSO_IPXIP6 | \
>>>>>>> + NETIF_F_GSO_UDP_L4 | \
>>>>>>> NETIF_F_GSO_UDP_TUNNEL | \
>>>>>>> NETIF_F_GSO_UDP_TUNNEL_CSUM)
>>>>>>
>>>>>> Are you adding this to NETIF_F_GSO_ENCAP_ALL? Wouldn't it make more
>>>>>> sense to add it to NETIF_F_GSO_SOFTWARE?
>>>>>>
>>>>>
>>>>> Yes, I'm adding to NETIF_F_GSO_ENCAP_ALL (not very clear from the
>>>>> context). I will fix the commit log.
>>>>>
>>>>> In: 83aa025 udp: add gso support to virtual devices, the support was
>>>>> also added to NETIF_F_GSO_ENCAP_ALL (although subsequently reverted due
>>>>> to UDP GRO not being in place), so I wonder what the reason was for that?
>>>>
>>>> That was probably just a bad choice on my part.
>>>>
>>>> It worked in practice, but if NETIF_F_GSO_SOFTWARE works the same
>>>> without unexpected side effects, then I agree that it is the better choice.
>>>>
>>>> That choice does appear to change behavior when sending over tunnel
>>>> devices. Might it send tunneled GSO packets over loopback?
>>>>
>>>>
>>>
>>> I set up a test case using fou tunneling through a bridge device using
>>> the udpgso_bench_tx test where packets are not received correctly if
>>> NETIF_F_GSO_UDP_L4 is added to NETIF_F_GSO_SOFTWARE. If I have it added
>>> to NETIF_F_GSO_ENCAP_ALL, it does work correctly. So there are more
>>> fixes required to include it in NETIF_F_GSO_SOFTWARE.
>>>
>>> The use-case I have only requires it to be in NETIF_F_GSO_ENCAP_ALL, but
>>> if it needs to go in NETIF_F_GSO_SOFTWARE, I can look at what's required
>>> more next week.
>>>
>>
>> Hi,
>>
>> I haven't had a chance to investigate what goes wrong with including
>> NETIF_F_GSO_UDP_L4 in NETIF_F_GSO_SOFTWARE - but I was just wondering if
>> people are ok with NETIF_F_GSO_UDP_L4 being added to
>> NETIF_F_GSO_ENCAP_ALL and not NETIF_F_GSO_SOFTWARE (ie the original
>> patch as posted)?
>>
>> As I mentioned that is sufficient for my use-case, and its how Willem
>> originally proposed this.
>
> Indeed, based on the previous discussion this sounds fine to me.
>
Willem
Are you OK to ACK this? If not, is there something else you'd rather see
here?
Thanks
Josh
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] gso: enable udp gso for virtual devices
2019-08-09 18:58 ` Josh Hunt
@ 2019-08-09 19:13 ` Willem de Bruijn
0 siblings, 0 replies; 9+ messages in thread
From: Willem de Bruijn @ 2019-08-09 19:13 UTC (permalink / raw)
To: Josh Hunt
Cc: Willem de Bruijn, Jason Baron, Alexander Duyck, David Miller,
Netdev, Paolo Abeni
On Fri, Aug 9, 2019 at 2:58 PM Josh Hunt <johunt@akamai.com> wrote:
>
> On 6/26/19 4:41 PM, Willem de Bruijn wrote:
> > On Wed, Jun 26, 2019 at 3:17 PM Jason Baron <jbaron@akamai.com> wrote:
> >>
> >>
> >>
> >> On 6/14/19 4:53 PM, Jason Baron wrote:
> >>>
> >>>
> >>> On 6/13/19 5:20 PM, Willem de Bruijn wrote:
> >>>>>>> @@ -237,6 +237,7 @@ static inline int find_next_netdev_feature(u64 feature, unsigned long start)
> >>>>>>> NETIF_F_GSO_GRE_CSUM | \
> >>>>>>> NETIF_F_GSO_IPXIP4 | \
> >>>>>>> NETIF_F_GSO_IPXIP6 | \
> >>>>>>> + NETIF_F_GSO_UDP_L4 | \
> >>>>>>> NETIF_F_GSO_UDP_TUNNEL | \
> >>>>>>> NETIF_F_GSO_UDP_TUNNEL_CSUM)
> >>>>>>
> >>>>>> Are you adding this to NETIF_F_GSO_ENCAP_ALL? Wouldn't it make more
> >>>>>> sense to add it to NETIF_F_GSO_SOFTWARE?
> >>>>>>
> >>>>>
> >>>>> Yes, I'm adding to NETIF_F_GSO_ENCAP_ALL (not very clear from the
> >>>>> context). I will fix the commit log.
> >>>>>
> >>>>> In: 83aa025 udp: add gso support to virtual devices, the support was
> >>>>> also added to NETIF_F_GSO_ENCAP_ALL (although subsequently reverted due
> >>>>> to UDP GRO not being in place), so I wonder what the reason was for that?
> >>>>
> >>>> That was probably just a bad choice on my part.
> >>>>
> >>>> It worked in practice, but if NETIF_F_GSO_SOFTWARE works the same
> >>>> without unexpected side effects, then I agree that it is the better choice.
> >>>>
> >>>> That choice does appear to change behavior when sending over tunnel
> >>>> devices. Might it send tunneled GSO packets over loopback?
> >>>>
> >>>>
> >>>
> >>> I set up a test case using fou tunneling through a bridge device using
> >>> the udpgso_bench_tx test where packets are not received correctly if
> >>> NETIF_F_GSO_UDP_L4 is added to NETIF_F_GSO_SOFTWARE. If I have it added
> >>> to NETIF_F_GSO_ENCAP_ALL, it does work correctly. So there are more
> >>> fixes required to include it in NETIF_F_GSO_SOFTWARE.
> >>>
> >>> The use-case I have only requires it to be in NETIF_F_GSO_ENCAP_ALL, but
> >>> if it needs to go in NETIF_F_GSO_SOFTWARE, I can look at what's required
> >>> more next week.
> >>>
> >>
> >> Hi,
> >>
> >> I haven't had a chance to investigate what goes wrong with including
> >> NETIF_F_GSO_UDP_L4 in NETIF_F_GSO_SOFTWARE - but I was just wondering if
> >> people are ok with NETIF_F_GSO_UDP_L4 being added to
> >> NETIF_F_GSO_ENCAP_ALL and not NETIF_F_GSO_SOFTWARE (ie the original
> >> patch as posted)?
> >>
> >> As I mentioned that is sufficient for my use-case, and its how Willem
> >> originally proposed this.
> >
> > Indeed, based on the previous discussion this sounds fine to me.
> >
>
> Willem
>
> Are you OK to ACK this? If not, is there something else you'd rather see
> here?
Sure. Unless Alex still has objections, feel free to resubmit with my Acked-by.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-08-09 19:13 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-12 23:12 [PATCH net-next] gso: enable udp gso for virtual devices Jason Baron
2019-06-13 17:15 ` Alexander Duyck
2019-06-13 19:03 ` Jason Baron
2019-06-13 21:20 ` Willem de Bruijn
2019-06-14 20:53 ` Jason Baron
2019-06-26 19:15 ` Jason Baron
2019-06-26 23:41 ` Willem de Bruijn
2019-08-09 18:58 ` Josh Hunt
2019-08-09 19:13 ` Willem de Bruijn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).