[PATCH net-next 1/4] udp_offload: simplify error path

[PATCH net-next 1/4] udp_offload: simplify error path

[Paolo Abeni]

We can remove an unconditional jump, checking for the error
condition instead of checking for success

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/ipv4/udp_offload.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 81f253b..9c37338 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -268,11 +268,9 @@ struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *skb,
 	rcu_read_lock();
 	sk = (*lookup)(skb, uh->source, uh->dest);
 
-	if (sk && udp_sk(sk)->gro_receive)
-		goto unflush;
-	goto out_unlock;
+	if (!sk || !udp_sk(sk)->gro_receive)
+		goto out_unlock;
 
-unflush:
 	flush = 0;
 
 	for (p = *head; p; p = p->next) {
-- 
1.8.3.1

[PATCH net-next 2/4] udp offload: allow GRO on 0 checksum packets

[Paolo Abeni]

currently, UDP packets with zero checksum are not allowed to
use udp offload's GRO. This patch admits such packets to
GRO, if the related socket settings allow it: ipv6 packets
are not admitted if the sockets don't have the no_check6_rx
flag set.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/ipv4/udp_offload.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 9c37338..ac783f4 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -257,7 +257,7 @@ struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *skb,
 	struct sock *sk;
 
 	if (NAPI_GRO_CB(skb)->encap_mark ||
-	    (skb->ip_summed != CHECKSUM_PARTIAL &&
+	    (uh->check && skb->ip_summed != CHECKSUM_PARTIAL &&
 	     NAPI_GRO_CB(skb)->csum_cnt == 0 &&
 	     !NAPI_GRO_CB(skb)->csum_valid))
 		goto out;
@@ -271,6 +271,10 @@ struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *skb,
 	if (!sk || !udp_sk(sk)->gro_receive)
 		goto out_unlock;
 
+	if (!uh->check && skb->protocol == cpu_to_be16(ETH_P_IPV6) &&
+	    !udp_sk(sk)->no_check6_rx)
+		goto out_unlock;
+
 	flush = 0;
 
 	for (p = *head; p; p = p->next) {
-- 
1.8.3.1

Re: [PATCH net-next 2/4] udp offload: allow GRO on 0 checksum packets

[Alexander Duyck]

On Thu, Jul 7, 2016 at 8:58 AM, Paolo Abeni <pabeni@redhat.com> wrote:
> currently, UDP packets with zero checksum are not allowed to
> use udp offload's GRO. This patch admits such packets to
> GRO, if the related socket settings allow it: ipv6 packets
> are not admitted if the sockets don't have the no_check6_rx
> flag set.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  net/ipv4/udp_offload.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 9c37338..ac783f4 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -257,7 +257,7 @@ struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *skb,
>         struct sock *sk;
>
>         if (NAPI_GRO_CB(skb)->encap_mark ||
> -           (skb->ip_summed != CHECKSUM_PARTIAL &&
> +           (uh->check && skb->ip_summed != CHECKSUM_PARTIAL &&
>              NAPI_GRO_CB(skb)->csum_cnt == 0 &&
>              !NAPI_GRO_CB(skb)->csum_valid))
>                 goto out;

So now all zero checksum UDP traffic will be targeted for GRO if I am
understanding this right.  Have you looked into how much of an impact
this might have on performance for non-tunnel UDP traffic using a zero
checksum?  I'm thinking it will be negative.  The issue is you are now
going to be performing an extra socket lookup for all incoming UDP
frames that have a zero checksum.

Also in the line below this line we are setting the encap_mark.  That
will probably need to be moved down to the point just before we call
gro_receive so that we can avoid setting unneeded data in the
NAPI_GRO_CB.

> @@ -271,6 +271,10 @@ struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *skb,
>         if (!sk || !udp_sk(sk)->gro_receive)
>                 goto out_unlock;
>
> +       if (!uh->check && skb->protocol == cpu_to_be16(ETH_P_IPV6) &&
> +           !udp_sk(sk)->no_check6_rx)
> +               goto out_unlock;
> +
>         flush = 0;
>
>         for (p = *head; p; p = p->next) {

So I am pretty sure this check doesn't pass the sniff test.
Specifically I don't believe you can use skb->protocol like you
currently are as it could be an 8021q frame for instance that is being
aggregated so the skb->protocol would be invalid.  I think what you
should probably be using is NAPI_GRO_CB(skb)->is_ipv6 although it
occurs to me that in the case of tunnels I don't know if that value is
being reset for IPv4 like it should be.

- Alex

Re: [PATCH net-next 2/4] udp offload: allow GRO on 0 checksum packets

[Hannes Frederic Sowa]

On 08.07.2016 12:46, Alexander Duyck wrote:
> On Thu, Jul 7, 2016 at 8:58 AM, Paolo Abeni <pabeni@redhat.com> wrote:
>> currently, UDP packets with zero checksum are not allowed to
>> use udp offload's GRO. This patch admits such packets to
>> GRO, if the related socket settings allow it: ipv6 packets
>> are not admitted if the sockets don't have the no_check6_rx
>> flag set.
>>
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>> ---
>>  net/ipv4/udp_offload.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>> index 9c37338..ac783f4 100644
>> --- a/net/ipv4/udp_offload.c
>> +++ b/net/ipv4/udp_offload.c
>> @@ -257,7 +257,7 @@ struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *skb,
>>         struct sock *sk;
>>
>>         if (NAPI_GRO_CB(skb)->encap_mark ||
>> -           (skb->ip_summed != CHECKSUM_PARTIAL &&
>> +           (uh->check && skb->ip_summed != CHECKSUM_PARTIAL &&
>>              NAPI_GRO_CB(skb)->csum_cnt == 0 &&
>>              !NAPI_GRO_CB(skb)->csum_valid))
>>                 goto out;
> 
> So now all zero checksum UDP traffic will be targeted for GRO if I am
> understanding this right.  Have you looked into how much of an impact
> this might have on performance for non-tunnel UDP traffic using a zero
> checksum?  I'm thinking it will be negative.  The issue is you are now
> going to be performing an extra socket lookup for all incoming UDP
> frames that have a zero checksum.

Are zero checksummed UDP protocols rare and only happen in case where we
have tunneling protocols, which need the socket lookup anyway? That
said, we haven't really focused on the impact here and thought it
shouldn't matter to try to speed up zero checksum UDP protocols over
zero ones.

> Also in the line below this line we are setting the encap_mark.  That
> will probably need to be moved down to the point just before we call
> gro_receive so that we can avoid setting unneeded data in the
> NAPI_GRO_CB.

Ack.

>> @@ -271,6 +271,10 @@ struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *skb,
>>         if (!sk || !udp_sk(sk)->gro_receive)
>>                 goto out_unlock;
>>
>> +       if (!uh->check && skb->protocol == cpu_to_be16(ETH_P_IPV6) &&
>> +           !udp_sk(sk)->no_check6_rx)
>> +               goto out_unlock;
>> +
>>         flush = 0;
>>
>>         for (p = *head; p; p = p->next) {
> 
> So I am pretty sure this check doesn't pass the sniff test.
> Specifically I don't believe you can use skb->protocol like you
> currently are as it could be an 8021q frame for instance that is being
> aggregated so the skb->protocol would be invalid.  I think what you
> should probably be using is NAPI_GRO_CB(skb)->is_ipv6 although it
> occurs to me that in the case of tunnels I don't know if that value is
> being reset for IPv4 like it should be.

Thanks, we probably should switch to sk->sk_family (we don't allow dual
family sockets with tunnel drivers so far)?

Bye,
Hannes

Re: [PATCH net-next 2/4] udp offload: allow GRO on 0 checksum packets

[Alexander Duyck]

On Fri, Jul 8, 2016 at 9:56 AM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> On 08.07.2016 12:46, Alexander Duyck wrote:
>> On Thu, Jul 7, 2016 at 8:58 AM, Paolo Abeni <pabeni@redhat.com> wrote:
>>> currently, UDP packets with zero checksum are not allowed to
>>> use udp offload's GRO. This patch admits such packets to
>>> GRO, if the related socket settings allow it: ipv6 packets
>>> are not admitted if the sockets don't have the no_check6_rx
>>> flag set.
>>>
>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>> ---
>>>  net/ipv4/udp_offload.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>>> index 9c37338..ac783f4 100644
>>> --- a/net/ipv4/udp_offload.c
>>> +++ b/net/ipv4/udp_offload.c
>>> @@ -257,7 +257,7 @@ struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *skb,
>>>         struct sock *sk;
>>>
>>>         if (NAPI_GRO_CB(skb)->encap_mark ||
>>> -           (skb->ip_summed != CHECKSUM_PARTIAL &&
>>> +           (uh->check && skb->ip_summed != CHECKSUM_PARTIAL &&
>>>              NAPI_GRO_CB(skb)->csum_cnt == 0 &&
>>>              !NAPI_GRO_CB(skb)->csum_valid))
>>>                 goto out;
>>
>> So now all zero checksum UDP traffic will be targeted for GRO if I am
>> understanding this right.  Have you looked into how much of an impact
>> this might have on performance for non-tunnel UDP traffic using a zero
>> checksum?  I'm thinking it will be negative.  The issue is you are now
>> going to be performing an extra socket lookup for all incoming UDP
>> frames that have a zero checksum.
>
> Are zero checksummed UDP protocols rare and only happen in case where we
> have tunneling protocols, which need the socket lookup anyway? That
> said, we haven't really focused on the impact here and thought it
> shouldn't matter to try to speed up zero checksum UDP protocols over
> zero ones.

I'm not sure how rare they are, but I know they are used for more than
just tunnels, especially in the case of IPv4.  What I suspect will
happen with this being implemented is that we will end up with all
sorts of people coming forward complaining about performance
regressions when we add an extra socket lookup to their fast-path.
I'm sure Jesper's pktgen tests would show a some negatives with
something like this as pktgen uses a 0 UDP checksum as I recall.
However I would suspect he probably runs such tests with GRO already
disabled.

>> Also in the line below this line we are setting the encap_mark.  That
>> will probably need to be moved down to the point just before we call
>> gro_receive so that we can avoid setting unneeded data in the
>> NAPI_GRO_CB.
>
> Ack.
>
>>> @@ -271,6 +271,10 @@ struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *skb,
>>>         if (!sk || !udp_sk(sk)->gro_receive)
>>>                 goto out_unlock;
>>>
>>> +       if (!uh->check && skb->protocol == cpu_to_be16(ETH_P_IPV6) &&
>>> +           !udp_sk(sk)->no_check6_rx)
>>> +               goto out_unlock;
>>> +
>>>         flush = 0;
>>>
>>>         for (p = *head; p; p = p->next) {
>>
>> So I am pretty sure this check doesn't pass the sniff test.
>> Specifically I don't believe you can use skb->protocol like you
>> currently are as it could be an 8021q frame for instance that is being
>> aggregated so the skb->protocol would be invalid.  I think what you
>> should probably be using is NAPI_GRO_CB(skb)->is_ipv6 although it
>> occurs to me that in the case of tunnels I don't know if that value is
>> being reset for IPv4 like it should be.

I just looked at the function and verified the v4 path for UDP GRO is
setting this to zero as it should.

> Thanks, we probably should switch to sk->sk_family (we don't allow dual
> family sockets with tunnel drivers so far)?

I don't know what the situation there is.  I just now that for v4 vs
v6 UDP the NAPI_GRO_CB has a field called is_ipv6 which is populated
just before calling into the tunnel GRO path.  If you use that you can
guarantee that you are looking at the right type for the protocol
instead of guessing at it based on skb->protocol.

- Alex

Re: [PATCH net-next 2/4] udp offload: allow GRO on 0 checksum packets

[Tom Herbert]

On Thu, Jul 7, 2016 at 10:58 AM, Paolo Abeni <pabeni@redhat.com> wrote:
> currently, UDP packets with zero checksum are not allowed to
> use udp offload's GRO. This patch admits such packets to
> GRO, if the related socket settings allow it: ipv6 packets
> are not admitted if the sockets don't have the no_check6_rx
> flag set.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  net/ipv4/udp_offload.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 9c37338..ac783f4 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -257,7 +257,7 @@ struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *skb,
>         struct sock *sk;
>
>         if (NAPI_GRO_CB(skb)->encap_mark ||
> -           (skb->ip_summed != CHECKSUM_PARTIAL &&
> +           (uh->check && skb->ip_summed != CHECKSUM_PARTIAL &&
>              NAPI_GRO_CB(skb)->csum_cnt == 0 &&
>              !NAPI_GRO_CB(skb)->csum_valid))

Paolo,

I think you might be misunderstanding the intent of this conditional.
It is trying to deduce that that the inner TCP checksum has likely
been validated or can be validated without doing  packet checksum
calculation. This is trying to avoid doing host side checksum
calculation in the GRO path and really has little to do with rather
uh->check is zero or not. The assumption was that we shouldn't compute
whole packet checksums in the GRO path because of performance. If this
assumption is no longer valid (i.e. there's good data saying doing
checksums in GRO path is a benefit) then all the checksum parts of
this conditional should be removed.

Tom

>                 goto out;
> @@ -271,6 +271,10 @@ struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *skb,
>         if (!sk || !udp_sk(sk)->gro_receive)
>                 goto out_unlock;
>
> +       if (!uh->check && skb->protocol == cpu_to_be16(ETH_P_IPV6) &&
> +           !udp_sk(sk)->no_check6_rx)
> +               goto out_unlock;
> +
>         flush = 0;
>
>         for (p = *head; p; p = p->next) {
> --
> 1.8.3.1
>

Re: [PATCH net-next 2/4] udp offload: allow GRO on 0 checksum packets

[Paolo Abeni]

On Fri, 2016-07-08 at 16:03 -0500, Tom Herbert wrote:
> On Thu, Jul 7, 2016 at 10:58 AM, Paolo Abeni <pabeni@redhat.com> wrote:
> > currently, UDP packets with zero checksum are not allowed to
> > use udp offload's GRO. This patch admits such packets to
> > GRO, if the related socket settings allow it: ipv6 packets
> > are not admitted if the sockets don't have the no_check6_rx
> > flag set.
> >
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> >  net/ipv4/udp_offload.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> > index 9c37338..ac783f4 100644
> > --- a/net/ipv4/udp_offload.c
> > +++ b/net/ipv4/udp_offload.c
> > @@ -257,7 +257,7 @@ struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *skb,
> >         struct sock *sk;
> >
> >         if (NAPI_GRO_CB(skb)->encap_mark ||
> > -           (skb->ip_summed != CHECKSUM_PARTIAL &&
> > +           (uh->check && skb->ip_summed != CHECKSUM_PARTIAL &&
> >              NAPI_GRO_CB(skb)->csum_cnt == 0 &&
> >              !NAPI_GRO_CB(skb)->csum_valid))
> 
> Paolo,
> 
> I think you might be misunderstanding the intent of this conditional.
> It is trying to deduce that that the inner TCP checksum has likely
> been validated or can be validated without doing  packet checksum
> calculation. This is trying to avoid doing host side checksum
> calculation in the GRO path and really has little to do with rather
> uh->check is zero or not. The assumption was that we shouldn't compute
> whole packet checksums in the GRO path because of performance. If this
> assumption is no longer valid (i.e. there's good data saying doing
> checksums in GRO path is a benefit) then all the checksum parts of
> this conditional should be removed.

Oh, my bad! I was hit by an ixgbe errata (82599 sometimes marks zero
checksum udp packets with CHECKSUM_NONE), so in my tests the above
condition was matched by 0 csum UDP packets. Than I misread csum_cnt
documentation, assuming it was not incremented for zero checksum UDP
packets: I thought that the matches I saw were due to !uh->check
instead of missing offload.

Thank you for the clarification,

Paolo

[PATCH net-next 3/4] vxlan: remove gro_cell support

[Paolo Abeni]

GRO is now handled entirely by the udp_offload layer and  there is no need
for trying it again at the device level. We can drop gro_cell usage,
simplifying the driver a bit, while maintaining the same performance for
TCP and improving slightly for UDP.
This basically reverts the commit 58ce31cca1ff ("vxlan: GRO support
at tunnel layer")

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 drivers/net/vxlan.c | 9 ++-------
 include/net/vxlan.h | 1 -
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index ae7455d..050cf40 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1351,7 +1351,7 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
 	stats->rx_bytes += skb->len;
 	u64_stats_update_end(&stats->syncp);
 
-	gro_cells_receive(&vxlan->gro_cells, skb);
+	netif_rx(skb);
 	return 0;
 
 drop:
@@ -2533,8 +2533,6 @@ static void vxlan_setup(struct net_device *dev)
 
 	vxlan->dev = dev;
 
-	gro_cells_init(&vxlan->gro_cells, dev);
-
 	for (h = 0; h < FDB_HASH_SIZE; ++h)
 		INIT_HLIST_HEAD(&vxlan->fdb_head[h]);
 }
@@ -3044,7 +3042,6 @@ static void vxlan_dellink(struct net_device *dev, struct list_head *head)
 		hlist_del_rcu(&vxlan->hlist);
 	spin_unlock(&vn->sock_lock);
 
-	gro_cells_destroy(&vxlan->gro_cells);
 	list_del(&vxlan->next);
 	unregister_netdevice_queue(dev, head);
 }
@@ -3295,10 +3292,8 @@ static void __net_exit vxlan_exit_net(struct net *net)
 		/* If vxlan->dev is in the same netns, it has already been added
 		 * to the list by the previous loop.
 		 */
-		if (!net_eq(dev_net(vxlan->dev), net)) {
-			gro_cells_destroy(&vxlan->gro_cells);
+		if (!net_eq(dev_net(vxlan->dev), net))
 			unregister_netdevice_queue(vxlan->dev, &list);
-		}
 	}
 
 	unregister_netdevice_many(&list);
diff --git a/include/net/vxlan.h b/include/net/vxlan.h
index b96d036..9fa3247 100644
--- a/include/net/vxlan.h
+++ b/include/net/vxlan.h
@@ -237,7 +237,6 @@ struct vxlan_dev {
 	struct timer_list age_timer;
 	spinlock_t	  hash_lock;
 	unsigned int	  addrcnt;
-	struct gro_cells  gro_cells;
 
 	struct vxlan_config	cfg;
 
-- 
1.8.3.1

Re: [PATCH net-next 3/4] vxlan: remove gro_cell support

[Eric Dumazet]

On Thu, 2016-07-07 at 17:58 +0200, Paolo Abeni wrote:
> GRO is now handled entirely by the udp_offload layer and  there is no need
> for trying it again at the device level. We can drop gro_cell usage,
> simplifying the driver a bit, while maintaining the same performance for
> TCP and improving slightly for UDP.
> This basically reverts the commit 58ce31cca1ff ("vxlan: GRO support
> at tunnel layer")

Note that gro_cells provide GRO support after RPS, so this helps when we
must perform TCP checksum computation, if NIC lacks CHECKSUM_COMPLETE

(Say we receive packets all steered to a single RX queue due to RSS hash
being computed on outer header only)

Some people disable GRO on the physical device, but enable GRO on the
tunnels.

Re: [PATCH net-next 3/4] vxlan: remove gro_cell support

[Paolo Abeni]

On Thu, 2016-07-07 at 18:13 +0200, Eric Dumazet wrote:
> On Thu, 2016-07-07 at 17:58 +0200, Paolo Abeni wrote:
> > GRO is now handled entirely by the udp_offload layer and  there is no need
> > for trying it again at the device level. We can drop gro_cell usage,
> > simplifying the driver a bit, while maintaining the same performance for
> > TCP and improving slightly for UDP.
> > This basically reverts the commit 58ce31cca1ff ("vxlan: GRO support
> > at tunnel layer")
> 
> Note that gro_cells provide GRO support after RPS, so this helps when we
> must perform TCP checksum computation, if NIC lacks CHECKSUM_COMPLETE
> 
> (Say we receive packets all steered to a single RX queue due to RSS hash
> being computed on outer header only)

You can still allow steering to multiple queues configuring an rx flow
hash computed on the outer l3 and l4 headers for udp, if supported by
your nic.

> Some people disable GRO on the physical device, but enable GRO on the
> tunnels.

I see. I'll resubmit only the first two patches. Allowing GRO on ingress
nic even for zero udp tunnel is still beneficial.

Paolo

Re: [PATCH net-next 3/4] vxlan: remove gro_cell support

[Hannes Frederic Sowa]

Hi Eric,

On 07.07.2016 12:13, Eric Dumazet wrote:
> On Thu, 2016-07-07 at 17:58 +0200, Paolo Abeni wrote:
>> GRO is now handled entirely by the udp_offload layer and  there is no need
>> for trying it again at the device level. We can drop gro_cell usage,
>> simplifying the driver a bit, while maintaining the same performance for
>> TCP and improving slightly for UDP.
>> This basically reverts the commit 58ce31cca1ff ("vxlan: GRO support
>> at tunnel layer")
> 
> Note that gro_cells provide GRO support after RPS, so this helps when we
> must perform TCP checksum computation, if NIC lacks CHECKSUM_COMPLETE
> 
> (Say we receive packets all steered to a single RX queue due to RSS hash
> being computed on outer header only)
> 
> Some people disable GRO on the physical device, but enable GRO on the
> tunnels.

we are currently discussing your feedback and wonder how much it makes
sense to support such a scenario?

We have part of the inner hash in the outer UDP source port. So even the
outer hash does provide enough entropy to get frames of one tunnel on
multiple CPUs via hardware hashing - given that you don't care about OoO
for UDP (I infer that from the fact that RPS will also reorder UDP
frames in case of fragmentation).

I wonder why it makes sense to still take single RX queue nics into
consideration? We already provide support for multiqueue devices for
most VM-related interfaces as well. Can you describe why someone would
do such a scenario?

Thank you,
Hannes

Re: [PATCH net-next 3/4] vxlan: remove gro_cell support

[Eric Dumazet]

On Fri, 2016-07-08 at 11:12 -0400, Hannes Frederic Sowa wrote:
> Hi Eric,
> 
> On 07.07.2016 12:13, Eric Dumazet wrote:
> > On Thu, 2016-07-07 at 17:58 +0200, Paolo Abeni wrote:
> >> GRO is now handled entirely by the udp_offload layer and  there is no need
> >> for trying it again at the device level. We can drop gro_cell usage,
> >> simplifying the driver a bit, while maintaining the same performance for
> >> TCP and improving slightly for UDP.
> >> This basically reverts the commit 58ce31cca1ff ("vxlan: GRO support
> >> at tunnel layer")
> > 
> > Note that gro_cells provide GRO support after RPS, so this helps when we
> > must perform TCP checksum computation, if NIC lacks CHECKSUM_COMPLETE
> > 
> > (Say we receive packets all steered to a single RX queue due to RSS hash
> > being computed on outer header only)
> > 
> > Some people disable GRO on the physical device, but enable GRO on the
> > tunnels.
> 
> we are currently discussing your feedback and wonder how much it makes
> sense to support such a scenario?
> 
> We have part of the inner hash in the outer UDP source port. So even the
> outer hash does provide enough entropy to get frames of one tunnel on
> multiple CPUs via hardware hashing - given that you don't care about OoO
> for UDP (I infer that from the fact that RPS will also reorder UDP
> frames in case of fragmentation).
> 
> I wonder why it makes sense to still take single RX queue nics into
> consideration? We already provide support for multiqueue devices for
> most VM-related interfaces as well. Can you describe why someone would
> do such a scenario?

I was simply pointing out there are some use cases where the ability to
split incoming traffic on multiple cpus can help, especially with dumb
NIC.

Fact that GRO is already handled on the NIC itself is not something that
is hard coded. GRO can be enabled or disabled.

If you remove GRO support at tunnel, then you remove some flexibility.

For example, when GRO for GRE was added by Jerry Chu, we did not remove
GRO on GRE devices, because mlx4 NICs for example are unable to compute
TCP checksum when GRE encapsulation is used. A single CPU can not decap
at line rate on 40Gbit NIC without RX checksum offloading. An admin can
choose to use RPS to  split traffic coming on a single RX queue to X
cpus, and enable GRO after RPS, instead of before.

UDP might be different, if the sender properly adds entropy on outer
header (which is not something you can do with GRE)

You probably could default GRO on tunnels to off, since by default GRO
would already happen at the physical interface.

Re: [PATCH net-next 3/4] vxlan: remove gro_cell support

[Hannes Frederic Sowa]

On 08.07.2016 11:33, Eric Dumazet wrote:
> On Fri, 2016-07-08 at 11:12 -0400, Hannes Frederic Sowa wrote:
>> Hi Eric,
>>
>> On 07.07.2016 12:13, Eric Dumazet wrote:
>>> On Thu, 2016-07-07 at 17:58 +0200, Paolo Abeni wrote:
>>>> GRO is now handled entirely by the udp_offload layer and  there is no need
>>>> for trying it again at the device level. We can drop gro_cell usage,
>>>> simplifying the driver a bit, while maintaining the same performance for
>>>> TCP and improving slightly for UDP.
>>>> This basically reverts the commit 58ce31cca1ff ("vxlan: GRO support
>>>> at tunnel layer")
>>>
>>> Note that gro_cells provide GRO support after RPS, so this helps when we
>>> must perform TCP checksum computation, if NIC lacks CHECKSUM_COMPLETE
>>>
>>> (Say we receive packets all steered to a single RX queue due to RSS hash
>>> being computed on outer header only)
>>>
>>> Some people disable GRO on the physical device, but enable GRO on the
>>> tunnels.
>>
>> we are currently discussing your feedback and wonder how much it makes
>> sense to support such a scenario?
>>
>> We have part of the inner hash in the outer UDP source port. So even the
>> outer hash does provide enough entropy to get frames of one tunnel on
>> multiple CPUs via hardware hashing - given that you don't care about OoO
>> for UDP (I infer that from the fact that RPS will also reorder UDP
>> frames in case of fragmentation).
>>
>> I wonder why it makes sense to still take single RX queue nics into
>> consideration? We already provide support for multiqueue devices for
>> most VM-related interfaces as well. Can you describe why someone would
>> do such a scenario?
> 
> I was simply pointing out there are some use cases where the ability to
> split incoming traffic on multiple cpus can help, especially with dumb
> NIC.
> 
> Fact that GRO is already handled on the NIC itself is not something that
> is hard coded. GRO can be enabled or disabled.
>
> If you remove GRO support at tunnel, then you remove some flexibility.
>
> For example, when GRO for GRE was added by Jerry Chu, we did not remove
> GRO on GRE devices, because mlx4 NICs for example are unable to compute
> TCP checksum when GRE encapsulation is used. A single CPU can not decap
> at line rate on 40Gbit NIC without RX checksum offloading. An admin can
> choose to use RPS to  split traffic coming on a single RX queue to X
> cpus, and enable GRO after RPS, instead of before.
>
> UDP might be different, if the sender properly adds entropy on outer
> header (which is not something you can do with GRE)

Exactly, thus we are also only touching UDP tunneling protocols at the
moment. Did you nack the removal of gro_cell support from the udp
protocols or are you fine with it, given that we won't take away the
functionality to spread out skb_checksum to mulitple CPUs during GRO for
other protocols and didn't plan to do so?

> You probably could default GRO on tunnels to off, since by default GRO
> would already happen at the physical interface.

Thanks!

Re: [PATCH net-next 3/4] vxlan: remove gro_cell support

[Eric Dumazet]

On Fri, 2016-07-08 at 11:55 -0400, Hannes Frederic Sowa wrote:

> Exactly, thus we are also only touching UDP tunneling protocols at the
> moment. Did you nack the removal of gro_cell support from the udp
> protocols or are you fine with it, given that we won't take away the
> functionality to spread out skb_checksum to mulitple CPUs during GRO for
> other protocols and didn't plan to do so?

I am fine with it, but could you rephrase the changelog, otherwise some
people will think they can copy/paste this to other tunnels ?

[PATCH net-next 4/4] geneve: remove gro_cell support

[Paolo Abeni]

GRO is now handled entirely by the udp_offload layer and there is no need
for trying it again at the device level. We can drop gro_cell usage,
simplifying the driver a bit, while maintaining the same performance for
TCP and improving slightly for UDP.
This basically reverts the commit 8e816df87997 ("geneve: Use GRO cells
infrastructure.")

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 drivers/net/geneve.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 5de892f..ede46a0 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -15,7 +15,6 @@
 #include <linux/etherdevice.h>
 #include <linux/hash.h>
 #include <net/dst_metadata.h>
-#include <net/gro_cells.h>
 #include <net/rtnetlink.h>
 #include <net/geneve.h>
 #include <net/protocol.h>
@@ -70,7 +69,6 @@ struct geneve_dev {
 	__be32		   label;	/* IPv6 flowlabel override */
 	__be16		   dst_port;
 	bool		   collect_md;
-	struct gro_cells   gro_cells;
 	u32		   flags;
 	struct dst_cache   dst_cache;
 };
@@ -282,7 +280,7 @@ static void geneve_rx(struct geneve_dev *geneve, struct geneve_sock *gs,
 	stats->rx_bytes += skb->len;
 	u64_stats_update_end(&stats->syncp);
 
-	gro_cells_receive(&geneve->gro_cells, skb);
+	netif_rx(skb);
 	return;
 drop:
 	/* Consume bad packet */
@@ -299,16 +297,9 @@ static int geneve_init(struct net_device *dev)
 	if (!dev->tstats)
 		return -ENOMEM;
 
-	err = gro_cells_init(&geneve->gro_cells, dev);
-	if (err) {
-		free_percpu(dev->tstats);
-		return err;
-	}
-
 	err = dst_cache_init(&geneve->dst_cache, GFP_KERNEL);
 	if (err) {
 		free_percpu(dev->tstats);
-		gro_cells_destroy(&geneve->gro_cells);
 		return err;
 	}
 
@@ -320,7 +311,6 @@ static void geneve_uninit(struct net_device *dev)
 	struct geneve_dev *geneve = netdev_priv(dev);
 
 	dst_cache_destroy(&geneve->dst_cache);
-	gro_cells_destroy(&geneve->gro_cells);
 	free_percpu(dev->tstats);
 }
 
-- 
1.8.3.1