[PATCH RFC ipsec-next] IPsec GRO

[PATCH RFC ipsec-next] IPsec GRO

[Steffen Klassert]

This introduces a device independent napi instance that
handles GRO for IPsec. I was not able to use gro cells
directly because I don't have a netdevice where I can
hang off the napi instance. We now may have a secpath
at a GRO merged skb, so we need to drop it. This is the
only cange to the generic networking code.

The packet still travels two times through the stack,
but might be aggregated in the second round. We can
avoid the second round with implementing GRO callbacks
for the IPsec protocols. This will be a separate patchset
as this needs some more generic networking changes because
of the asynchronous nature of IPsec.

[PATCH RFC ipsec-next 1/2] net: Drop secpath on free after gro merge.

[Steffen Klassert]

With a followup patch, a gro merged skb can have a secpath.
So drop it before freeing or reusing the skb.

Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/core/dev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 56818f7..c9e541e 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4623,6 +4623,7 @@ static gro_result_t napi_skb_finish(gro_result_t ret, struct sk_buff *skb)
 	case GRO_MERGED_FREE:
 		if (NAPI_GRO_CB(skb)->free == NAPI_GRO_FREE_STOLEN_HEAD) {
 			skb_dst_drop(skb);
+			secpath_put(skb->sp);
 			kmem_cache_free(skbuff_head_cache, skb);
 		} else {
 			__kfree_skb(skb);
@@ -4663,6 +4664,7 @@ static void napi_reuse_skb(struct napi_struct *napi, struct sk_buff *skb)
 	skb->encapsulation = 0;
 	skb_shinfo(skb)->gso_type = 0;
 	skb->truesize = SKB_TRUESIZE(skb_end_offset(skb));
+	secpath_put(skb->sp);
 
 	napi->skb = skb;
 }
-- 
1.9.1

[PATCH RFC ipsec-next 2/2] xfrm: Add a device independent napi instance

[Steffen Klassert]

This patch adds a napi instance for IPsec GRO.
With this, we handle IPsec GRO with no impact
on the generic networking code.

Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_input.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 86 insertions(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index 6e3f025..c5fc12d 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c
@@ -21,6 +21,90 @@
 static DEFINE_SPINLOCK(xfrm_input_afinfo_lock);
 static struct xfrm_input_afinfo __rcu *xfrm_input_afinfo[NPROTO];
 
+struct xfrm_napi {
+	struct sk_buff_head	napi_skbs;
+	struct napi_struct	napi;
+};
+
+static struct xfrm_napi __percpu *napis;
+
+static void xfrm_gro_receive(struct sk_buff *skb)
+{
+	struct xfrm_napi *xnapi;
+	struct net_device *dev = skb->dev;
+
+	if (!napis || skb_cloned(skb) || !(dev->features & NETIF_F_GRO)) {
+		netif_rx(skb);
+		return;
+	}
+
+	xnapi = this_cpu_ptr(napis);
+
+	if (skb_queue_len(&xnapi->napi_skbs) > netdev_max_backlog) {
+		atomic_long_inc(&dev->rx_dropped);
+		kfree_skb(skb);
+		return;
+	}
+
+	__skb_queue_tail(&xnapi->napi_skbs, skb);
+	if (skb_queue_len(&xnapi->napi_skbs) == 1)
+		napi_schedule(&xnapi->napi);
+}
+
+static int xfrm_gro_poll(struct napi_struct *napi, int budget)
+{
+	unsigned long flags;
+	struct sk_buff *skb;
+	int work_done = 0;
+	struct xfrm_napi *xnapi = container_of(napi, struct xfrm_napi, napi);
+
+	while (work_done < budget) {
+		skb = __skb_dequeue(&xnapi->napi_skbs);
+		if (!skb)
+			break;
+		napi_gro_receive(napi, skb);
+		work_done++;
+	}
+
+	if (work_done < budget) {
+		napi_gro_flush(napi, false);
+
+		if (likely(list_empty(&napi->poll_list))) {
+			WARN_ON_ONCE(!test_and_clear_bit(NAPI_STATE_SCHED, &napi->state));
+		} else {
+			/* If n->poll_list is not empty, we need to mask irqs */
+			local_irq_save(flags);
+			__napi_complete(napi);
+			local_irq_restore(flags);
+		}
+	}
+
+	return work_done;
+}
+
+static void xfrm_gro_init(void)
+{
+	int i;
+
+	/* Napi remains disabled if we can't alloc memory. */
+	napis = alloc_percpu(struct xfrm_napi);
+
+	for_each_possible_cpu(i) {
+		struct xfrm_napi *xnapi = per_cpu_ptr(napis, i);
+
+		__skb_queue_head_init(&xnapi->napi_skbs);
+
+		set_bit(NAPI_STATE_NO_BUSY_POLL, &xnapi->napi.state);
+
+		INIT_LIST_HEAD(&xnapi->napi.poll_list);
+		xnapi->napi.poll = xfrm_gro_poll;
+		xnapi->napi.weight = 64;
+		xnapi->napi.gro_list = NULL;
+		xnapi->napi.gro_count = 0;
+		xnapi->napi.skb = NULL;
+	}
+}
+
 int xfrm_input_register_afinfo(struct xfrm_input_afinfo *afinfo)
 {
 	int err = 0;
@@ -371,7 +455,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 
 	if (decaps) {
 		skb_dst_drop(skb);
-		netif_rx(skb);
+		xfrm_gro_receive(skb);
 		return 0;
 	} else {
 		return x->inner_mode->afinfo->transport_finish(skb, async);
@@ -394,6 +478,7 @@ int xfrm_input_resume(struct sk_buff *skb, int nexthdr)
 
 void __init xfrm_input_init(void)
 {
+	xfrm_gro_init();
 	secpath_cachep = kmem_cache_create("secpath_cache",
 					   sizeof(struct sec_path),
 					   0, SLAB_HWCACHE_ALIGN|SLAB_PANIC,
-- 
1.9.1

Re: [PATCH RFC ipsec-next 1/2] net: Drop secpath on free after gro merge.

[Steffen Klassert]

On Thu, Jan 26, 2017 at 07:50:55AM +0100, Steffen Klassert wrote:
> With a followup patch, a gro merged skb can have a secpath.
> So drop it before freeing or reusing the skb.
> 
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> ---
>  net/core/dev.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 56818f7..c9e541e 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4623,6 +4623,7 @@ static gro_result_t napi_skb_finish(gro_result_t ret, struct sk_buff *skb)
>  	case GRO_MERGED_FREE:
>  		if (NAPI_GRO_CB(skb)->free == NAPI_GRO_FREE_STOLEN_HEAD) {
>  			skb_dst_drop(skb);
> +			secpath_put(skb->sp);
>  			kmem_cache_free(skbuff_head_cache, skb);
>  		} else {
>  			__kfree_skb(skb);
> @@ -4663,6 +4664,7 @@ static void napi_reuse_skb(struct napi_struct *napi, struct sk_buff *skb)
>  	skb->encapsulation = 0;
>  	skb_shinfo(skb)->gso_type = 0;
>  	skb->truesize = SKB_TRUESIZE(skb_end_offset(skb));
> +	secpath_put(skb->sp);

I have to use secpath_reset(skb) here of course.
skb->sp is not even defined if CONFIG_XFRM is not set.

Thanks to "kbuild test robot <lkp@intel.com>" for
pointing to that.

Re: [PATCH RFC ipsec-next 2/2] xfrm: Add a device independent napi instance

[Eric Dumazet]

On Thu, 2017-01-26 at 07:50 +0100, Steffen Klassert wrote:
> This patch adds a napi instance for IPsec GRO.
> With this, we handle IPsec GRO with no impact
> on the generic networking code.
> 
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> ---
>  net/xfrm/xfrm_input.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 86 insertions(+), 1 deletion(-)
> 
> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> index 6e3f025..c5fc12d 100644
> --- a/net/xfrm/xfrm_input.c
> +++ b/net/xfrm/xfrm_input.c
> @@ -21,6 +21,90 @@
>  static DEFINE_SPINLOCK(xfrm_input_afinfo_lock);
>  static struct xfrm_input_afinfo __rcu *xfrm_input_afinfo[NPROTO];
>  
> +struct xfrm_napi {
> +	struct sk_buff_head	napi_skbs;
> +	struct napi_struct	napi;
> +};
> +
> +static struct xfrm_napi __percpu *napis;
> +
> +static void xfrm_gro_receive(struct sk_buff *skb)
> +{
> +	struct xfrm_napi *xnapi;
> +	struct net_device *dev = skb->dev;
> +
> +	if (!napis || skb_cloned(skb) || !(dev->features & NETIF_F_GRO)) {
> +		netif_rx(skb);
> +		return;
> +	}
> +
> +	xnapi = this_cpu_ptr(napis);
> +
> +	if (skb_queue_len(&xnapi->napi_skbs) > netdev_max_backlog) {
> +		atomic_long_inc(&dev->rx_dropped);
> +		kfree_skb(skb);
> +		return;
> +	}
> +
> +	__skb_queue_tail(&xnapi->napi_skbs, skb);
> +	if (skb_queue_len(&xnapi->napi_skbs) == 1)
> +		napi_schedule(&xnapi->napi);
> +}
> +
> +static int xfrm_gro_poll(struct napi_struct *napi, int budget)
> +{
> +	unsigned long flags;
> +	struct sk_buff *skb;
> +	int work_done = 0;
> +	struct xfrm_napi *xnapi = container_of(napi, struct xfrm_napi, napi);
> +
> +	while (work_done < budget) {
> +		skb = __skb_dequeue(&xnapi->napi_skbs);
> +		if (!skb)
> +			break;
> +		napi_gro_receive(napi, skb);
> +		work_done++;
> +	}
> +
> +	if (work_done < budget) {
> +		napi_gro_flush(napi, false);
> +
> +		if (likely(list_empty(&napi->poll_list))) {
> +			WARN_ON_ONCE(!test_and_clear_bit(NAPI_STATE_SCHED, &napi->state));
> +		} else {
> +			/* If n->poll_list is not empty, we need to mask irqs */
> +			local_irq_save(flags);
> +			__napi_complete(napi);
> +			local_irq_restore(flags);
> +		}
> +	}


The following should work and contain less copy/pasted code.

          if (work_done < budget)
               napi_complete(napi);


> +
> +	return work_done;
> +}
> +
> +static void xfrm_gro_init(void)
> +{
> +	int i;
> +
> +	/* Napi remains disabled if we can't alloc memory. */
> +	napis = alloc_percpu(struct xfrm_napi);

napis can be NULL here.

> +
> +	for_each_possible_cpu(i) {
> +		struct xfrm_napi *xnapi = per_cpu_ptr(napis, i);
> +
> +		__skb_queue_head_init(&xnapi->napi_skbs);
> +
> +		set_bit(NAPI_STATE_NO_BUSY_POLL, &xnapi->napi.state);
> +
> +		INIT_LIST_HEAD(&xnapi->napi.poll_list);
> +		xnapi->napi.poll = xfrm_gro_poll;
> +		xnapi->napi.weight = 64;
> +		xnapi->napi.gro_list = NULL;
> +		xnapi->napi.gro_count = 0;
> +		xnapi->napi.skb = NULL;
> +	}
> +}


Alternative would be to use a 

static struct net_device xfrm_napi_anchor_device;

and use gro_cell


I will present in next netdev conference a work based on NAPI,
and having less hand coded napi logic will help a lot.

Thanks.

Re: [PATCH RFC ipsec-next 2/2] xfrm: Add a device independent napi instance

[Eric Dumazet]

On Thu, 2017-01-26 at 06:26 -0800, Eric Dumazet wrote:

> 
> Alternative would be to use a 
> 
> static struct net_device xfrm_napi_anchor_device;
> 
> and use gro_cell

Also take a look at init_dummy_netdev()

Thanks !

Re: [PATCH RFC ipsec-next 2/2] xfrm: Add a device independent napi instance

[Steffen Klassert]

On Thu, Jan 26, 2017 at 07:10:22AM -0800, Eric Dumazet wrote:
> On Thu, 2017-01-26 at 06:26 -0800, Eric Dumazet wrote:
> 
> > 
> > Alternative would be to use a 
> > 
> > static struct net_device xfrm_napi_anchor_device;
> > 
> > and use gro_cell
> 
> Also take a look at init_dummy_netdev()

I already thought about using some dummy net_device for this,
but was not sure what I need to initialize. So it seemed
to be safer to use a private napi instance.

init_dummy_netdev() is exactly what I need for that, thanks a lot!