Re: [PATCH net-next] net: ipv6/gre: Add GRO support

[Tariq Toukan]

On 02/04/2018 6:19 PM, Eric Dumazet wrote:



On 04/02/2018 08:00 AM, Eran Ben Elisha wrote:

Seems good, but why isn't this handled directly in GRO native layer ?

ip6_tunnel and ip6_gre do not share initialization flow functions (unlike ipv4).
Changing the ipv6 init infrastructure should not be part of this
patch. we prefer to keep this one minimal, simple and safe.




Looking at gre_gro_receive() and gre_gro_complete() I could not see why they
could not be copied/pasted to IPv6.


These functions to handle GRO over GRE are already assigned in
gre_offload_init() (in net/ipv4/gre_offload.c under CONFIG_IPV6).
However without initializing the gro_cells, the receive path will not
go via napi_gro_receive path, but directly to netif_rx.
So AFAIU, only gcells->cells was missing for gro_cells_receive to
really go via GRO flow.



Maybe give more details on the changelog, it is really not obvious.

Hopefully the above filled this request.




Not really :/



So you're referring to native interface. We thought you meant native IP 
module.




gro_cells_receive() is not really useful with native GRO, since packet is 
already
a GRO packet by the time it reaches ip_tunnel_rcv() or __ip6_tnl_rcv()



Right. If GRO on native interface is ON, our patch doesn't help much.
The case we improve here is:
Native has GRO OFF, GRE has GRO ON.

Before this patch there were no GRO packets at all in this case, only 
MTU packets went up the stack.



Sure, it might be usefull if native GRO (happening on eth0 if you prefer) did 
not
handle a particular encapsulation.



Or it is turned OFF.


gro_cell was a work around before we extended GRO to be able to decap some 
tunnel headers.

It seems we have to extend this to also support GRE6.

Re: [PATCH net-next] net: ipv6/gre: Add GRO support

[Eric Dumazet]

On 04/02/2018 08:00 AM, Eran Ben Elisha wrote:
 Seems good, but why isn't this handled directly in GRO native layer ?
>>> ip6_tunnel and ip6_gre do not share initialization flow functions (unlike 
>>> ipv4).
>>> Changing the ipv6 init infrastructure should not be part of this
>>> patch. we prefer to keep this one minimal, simple and safe.
>>
>>
>>
>> Looking at gre_gro_receive() and gre_gro_complete() I could not see why they
>> could not be copied/pasted to IPv6.
> 
> These functions to handle GRO over GRE are already assigned in
> gre_offload_init() (in net/ipv4/gre_offload.c under CONFIG_IPV6).
> However without initializing the gro_cells, the receive path will not
> go via napi_gro_receive path, but directly to netif_rx.
> So AFAIU, only gcells->cells was missing for gro_cells_receive to
> really go via GRO flow.
> 
>>
>> Maybe give more details on the changelog, it is really not obvious.
> Hopefully the above filled this request.
>>

Not really :/

gro_cells_receive() is not really useful with native GRO, since packet is 
already
a GRO packet by the time it reaches ip_tunnel_rcv() or __ip6_tnl_rcv()

Sure, it might be usefull if native GRO (happening on eth0 if you prefer) did 
not
handle a particular encapsulation.

gro_cell was a work around before we extended GRO to be able to decap some 
tunnel headers.

It seems we have to extend this to also support GRE6.

Re: [PATCH net-next] net: ipv6/gre: Add GRO support

[Eran Ben Elisha]

>>> Seems good, but why isn't this handled directly in GRO native layer ?
>> ip6_tunnel and ip6_gre do not share initialization flow functions (unlike 
>> ipv4).
>> Changing the ipv6 init infrastructure should not be part of this
>> patch. we prefer to keep this one minimal, simple and safe.
>
>
>
> Looking at gre_gro_receive() and gre_gro_complete() I could not see why they
> could not be copied/pasted to IPv6.

These functions to handle GRO over GRE are already assigned in
gre_offload_init() (in net/ipv4/gre_offload.c under CONFIG_IPV6).
However without initializing the gro_cells, the receive path will not
go via napi_gro_receive path, but directly to netif_rx.
So AFAIU, only gcells->cells was missing for gro_cells_receive to
really go via GRO flow.

>
> Maybe give more details on the changelog, it is really not obvious.
Hopefully the above filled this request.
>

Re: [PATCH net-next] net: ipv6/gre: Add GRO support

[Eric Dumazet]

On 04/02/2018 05:40 AM, Eran Ben Elisha wrote:
> On Sun, Apr 1, 2018 at 7:35 PM, Eric Dumazet  wrote:
>>
>>
>> On 04/01/2018 06:17 AM, Tariq Toukan wrote:
>>> From: Eran Ben Elisha 
>>>
>>> Add GRO capability for IPv6 GRE tunnel and ip6erspan tap, via gro_cells
>>> infrastructure.
>>>
>>> Performance testing: 55% higher badwidth.
>>> Measuring bandwidth of 1 thread IPv4 TCP traffic over IPv6 GRE tunnel
>>> while GRO on the physical interface is disabled.
>>> CPU: Intel Xeon E312xx (Sandy Bridge)
>>> NIC: Mellanox Technologies MT27700 Family [ConnectX-4]
>>> Before (GRO not working in tunnel) : 2.47 Gbits/sec
>>> After  (GRO working in tunnel) : 3.85 Gbits/sec
>>>
>>> Signed-off-by: Eran Ben Elisha 
>>> Signed-off-by: Tariq Toukan 
>>> CC: Eric Dumazet 
>>> ---
>>
>>
>> Seems good, but why isn't this handled directly in GRO native layer ?
> ip6_tunnel and ip6_gre do not share initialization flow functions (unlike 
> ipv4).
> Changing the ipv6 init infrastructure should not be part of this
> patch. we prefer to keep this one minimal, simple and safe.



Looking at gre_gro_receive() and gre_gro_complete() I could not see why they
could not be copied/pasted to IPv6.

Maybe give more details on the changelog, it is really not obvious.

Re: [PATCH net-next] net: ipv6/gre: Add GRO support

[Eran Ben Elisha]

On Sun, Apr 1, 2018 at 7:35 PM, Eric Dumazet  wrote:
>
>
> On 04/01/2018 06:17 AM, Tariq Toukan wrote:
>> From: Eran Ben Elisha 
>>
>> Add GRO capability for IPv6 GRE tunnel and ip6erspan tap, via gro_cells
>> infrastructure.
>>
>> Performance testing: 55% higher badwidth.
>> Measuring bandwidth of 1 thread IPv4 TCP traffic over IPv6 GRE tunnel
>> while GRO on the physical interface is disabled.
>> CPU: Intel Xeon E312xx (Sandy Bridge)
>> NIC: Mellanox Technologies MT27700 Family [ConnectX-4]
>> Before (GRO not working in tunnel) : 2.47 Gbits/sec
>> After  (GRO working in tunnel) : 3.85 Gbits/sec
>>
>> Signed-off-by: Eran Ben Elisha 
>> Signed-off-by: Tariq Toukan 
>> CC: Eric Dumazet 
>> ---
>
>
> Seems good, but why isn't this handled directly in GRO native layer ?
ip6_tunnel and ip6_gre do not share initialization flow functions (unlike ipv4).
Changing the ipv6 init infrastructure should not be part of this
patch. we prefer to keep this one minimal, simple and safe.
>

Re: [PATCH net-next] net: ipv6/gre: Add GRO support

[Eric Dumazet]

On 04/01/2018 06:17 AM, Tariq Toukan wrote:
> From: Eran Ben Elisha 
> 
> Add GRO capability for IPv6 GRE tunnel and ip6erspan tap, via gro_cells
> infrastructure.
> 
> Performance testing: 55% higher badwidth.
> Measuring bandwidth of 1 thread IPv4 TCP traffic over IPv6 GRE tunnel
> while GRO on the physical interface is disabled.
> CPU: Intel Xeon E312xx (Sandy Bridge)
> NIC: Mellanox Technologies MT27700 Family [ConnectX-4]
> Before (GRO not working in tunnel) : 2.47 Gbits/sec
> After  (GRO working in tunnel) : 3.85 Gbits/sec
> 
> Signed-off-by: Eran Ben Elisha 
> Signed-off-by: Tariq Toukan 
> CC: Eric Dumazet 
> ---


Seems good, but why isn't this handled directly in GRO native layer ?

[PATCH net-next] net: ipv6/gre: Add GRO support

[Tariq Toukan]

From: Eran Ben Elisha 

Add GRO capability for IPv6 GRE tunnel and ip6erspan tap, via gro_cells
infrastructure.

Performance testing: 55% higher badwidth.
Measuring bandwidth of 1 thread IPv4 TCP traffic over IPv6 GRE tunnel
while GRO on the physical interface is disabled.
CPU: Intel Xeon E312xx (Sandy Bridge)
NIC: Mellanox Technologies MT27700 Family [ConnectX-4]
Before (GRO not working in tunnel) : 2.47 Gbits/sec
After  (GRO working in tunnel) : 3.85 Gbits/sec

Signed-off-by: Eran Ben Elisha 
Signed-off-by: Tariq Toukan 
CC: Eric Dumazet 
---
 net/ipv6/ip6_gre.c | 37 +++--
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 22e86557aca4..8abc9d72993e 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -1306,6 +1306,7 @@ static void ip6gre_dev_free(struct net_device *dev)
 {
struct ip6_tnl *t = netdev_priv(dev);
 
+   gro_cells_destroy(>gro_cells);
dst_cache_destroy(>dst_cache);
free_percpu(dev->tstats);
 }
@@ -1373,11 +1374,12 @@ static int ip6gre_tunnel_init_common(struct net_device 
*dev)
return -ENOMEM;
 
ret = dst_cache_init(>dst_cache, GFP_KERNEL);
-   if (ret) {
-   free_percpu(dev->tstats);
-   dev->tstats = NULL;
-   return ret;
-   }
+   if (ret)
+   goto cleanup_alloc_pcpu_stats;
+
+   ret = gro_cells_init(>gro_cells, dev);
+   if (ret)
+   goto cleanup_dst_cache_init;
 
tunnel->tun_hlen = gre_calc_hlen(tunnel->parms.o_flags);
tunnel->hlen = tunnel->tun_hlen + tunnel->encap_hlen;
@@ -1397,6 +1399,13 @@ static int ip6gre_tunnel_init_common(struct net_device 
*dev)
ip6gre_tnl_init_features(dev);
 
return 0;
+
+cleanup_dst_cache_init:
+   dst_cache_destroy(>dst_cache);
+cleanup_alloc_pcpu_stats:
+   free_percpu(dev->tstats);
+   dev->tstats = NULL;
+   return ret;
 }
 
 static int ip6gre_tunnel_init(struct net_device *dev)
@@ -1743,11 +1752,12 @@ static int ip6erspan_tap_init(struct net_device *dev)
return -ENOMEM;
 
ret = dst_cache_init(>dst_cache, GFP_KERNEL);
-   if (ret) {
-   free_percpu(dev->tstats);
-   dev->tstats = NULL;
-   return ret;
-   }
+   if (ret)
+   goto cleanup_alloc_pcpu_stats;
+
+   ret = gro_cells_init(>gro_cells, dev);
+   if (ret)
+   goto cleanup_dst_cache_init;
 
tunnel->tun_hlen = 8;
tunnel->hlen = tunnel->tun_hlen + tunnel->encap_hlen +
@@ -1766,6 +1776,13 @@ static int ip6erspan_tap_init(struct net_device *dev)
ip6gre_tnl_link_config(tunnel, 1);
 
return 0;
+
+cleanup_dst_cache_init:
+   dst_cache_destroy(>dst_cache);
+cleanup_alloc_pcpu_stats:
+   free_percpu(dev->tstats);
+   dev->tstats = NULL;
+   return ret;
 }
 
 static const struct net_device_ops ip6erspan_netdev_ops = {
-- 
1.8.3.1