[PATCH] net: convert gro_count to bitmask

[PATCH] net: convert gro_count to bitmask

[Li RongQing]

gro_hash size is 192 bytes, and uses 3 cache lines, if there is few
flows, gro_hash may be not fully used, so it is unnecessary to iterate
all gro_hash in napi_gro_flush(), to occupy unnecessary cacheline.

convert gro_count to a bitmask, and rename it as gro_bitmask, each bit
represents a element of gro_hash, only flush a gro_hash element if the
related bit is set, to speed up napi_gro_flush().

and update gro_bitmask only if it will be changed, to reduce cache
update

Suggested-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 include/linux/netdevice.h |  2 +-
 net/core/dev.c            | 35 +++++++++++++++++++++++------------
 2 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index b683971e500d..df49b36ef378 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -322,7 +322,7 @@ struct napi_struct {
 
 	unsigned long		state;
 	int			weight;
-	unsigned int		gro_count;
+	unsigned long		gro_bitmask;
 	int			(*poll)(struct napi_struct *, int);
 #ifdef CONFIG_NETPOLL
 	int			poll_owner;
diff --git a/net/core/dev.c b/net/core/dev.c
index d13cddcac41f..a08dbdd217a6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5171,9 +5171,11 @@ static void __napi_gro_flush_chain(struct napi_struct *napi, u32 index,
 			return;
 		list_del_init(&skb->list);
 		napi_gro_complete(skb);
-		napi->gro_count--;
 		napi->gro_hash[index].count--;
 	}
+
+	if (!napi->gro_hash[index].count)
+		clear_bit(index, &napi->gro_bitmask);
 }
 
 /* napi->gro_hash[].list contains packets ordered by age.
@@ -5184,8 +5186,10 @@ void napi_gro_flush(struct napi_struct *napi, bool flush_old)
 {
 	u32 i;
 
-	for (i = 0; i < GRO_HASH_BUCKETS; i++)
-		__napi_gro_flush_chain(napi, i, flush_old);
+	for (i = 0; i < GRO_HASH_BUCKETS; i++) {
+		if (test_bit(i, &napi->gro_bitmask))
+			__napi_gro_flush_chain(napi, i, flush_old);
+	}
 }
 EXPORT_SYMBOL(napi_gro_flush);
 
@@ -5277,8 +5281,8 @@ static void gro_flush_oldest(struct list_head *head)
 	if (WARN_ON_ONCE(!oldest))
 		return;
 
-	/* Do not adjust napi->gro_count, caller is adding a new SKB to
-	 * the chain.
+	/* Do not adjust napi->gro_hash[].count, caller is adding a new
+	 * SKB to the chain.
 	 */
 	list_del(&oldest->list);
 	napi_gro_complete(oldest);
@@ -5352,7 +5356,6 @@ static enum gro_result dev_gro_receive(struct napi_struct *napi, struct sk_buff
 	if (pp) {
 		list_del_init(&pp->list);
 		napi_gro_complete(pp);
-		napi->gro_count--;
 		napi->gro_hash[hash].count--;
 	}
 
@@ -5365,7 +5368,6 @@ static enum gro_result dev_gro_receive(struct napi_struct *napi, struct sk_buff
 	if (unlikely(napi->gro_hash[hash].count >= MAX_GRO_SKBS)) {
 		gro_flush_oldest(gro_head);
 	} else {
-		napi->gro_count++;
 		napi->gro_hash[hash].count++;
 	}
 	NAPI_GRO_CB(skb)->count = 1;
@@ -5380,6 +5382,12 @@ static enum gro_result dev_gro_receive(struct napi_struct *napi, struct sk_buff
 	if (grow > 0)
 		gro_pull_from_frag0(skb, grow);
 ok:
+	if (napi->gro_hash[hash].count)
+		if (!test_bit(hash, &napi->gro_bitmask))
+			set_bit(hash, &napi->gro_bitmask);
+	else if (test_bit(hash, &napi->gro_bitmask))
+		clear_bit(hash, &napi->gro_bitmask);
+
 	return ret;
 
 normal:
@@ -5778,7 +5786,7 @@ bool napi_complete_done(struct napi_struct *n, int work_done)
 				 NAPIF_STATE_IN_BUSY_POLL)))
 		return false;
 
-	if (n->gro_count) {
+	if (n->gro_bitmask) {
 		unsigned long timeout = 0;
 
 		if (work_done)
@@ -5987,7 +5995,7 @@ static enum hrtimer_restart napi_watchdog(struct hrtimer *timer)
 	/* Note : we use a relaxed variant of napi_schedule_prep() not setting
 	 * NAPI_STATE_MISSED, since we do not react to a device IRQ.
 	 */
-	if (napi->gro_count && !napi_disable_pending(napi) &&
+	if (napi->gro_bitmask && !napi_disable_pending(napi) &&
 	    !test_and_set_bit(NAPI_STATE_SCHED, &napi->state))
 		__napi_schedule_irqoff(napi);
 
@@ -6002,7 +6010,7 @@ void netif_napi_add(struct net_device *dev, struct napi_struct *napi,
 	INIT_LIST_HEAD(&napi->poll_list);
 	hrtimer_init(&napi->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_PINNED);
 	napi->timer.function = napi_watchdog;
-	napi->gro_count = 0;
+	napi->gro_bitmask = 0;
 	for (i = 0; i < GRO_HASH_BUCKETS; i++) {
 		INIT_LIST_HEAD(&napi->gro_hash[i].list);
 		napi->gro_hash[i].count = 0;
@@ -6062,7 +6070,7 @@ void netif_napi_del(struct napi_struct *napi)
 	napi_free_frags(napi);
 
 	flush_gro_hash(napi);
-	napi->gro_count = 0;
+	napi->gro_bitmask = 0;
 }
 EXPORT_SYMBOL(netif_napi_del);
 
@@ -6104,7 +6112,7 @@ static int napi_poll(struct napi_struct *n, struct list_head *repoll)
 		goto out_unlock;
 	}
 
-	if (n->gro_count) {
+	if (n->gro_bitmask) {
 		/* flush too old packets
 		 * If HZ < 1000, flush all packets.
 		 */
@@ -9151,6 +9159,9 @@ static struct hlist_head * __net_init netdev_create_hash(void)
 /* Initialize per network namespace state */
 static int __net_init netdev_init(struct net *net)
 {
+	BUILD_BUG_ON(GRO_HASH_BUCKETS >
+			FIELD_SIZEOF(struct napi_struct, gro_bitmask));
+
 	if (net != &init_net)
 		INIT_LIST_HEAD(&net->dev_base_head);
 
-- 
2.16.2

Re: [PATCH] net: convert gro_count to bitmask

[Stefano Brivio]

On Wed, 11 Jul 2018 17:15:53 +0800
Li RongQing <lirongqing@baidu.com> wrote:

> @@ -5380,6 +5382,12 @@ static enum gro_result dev_gro_receive(struct napi_struct *napi, struct sk_buff
>  	if (grow > 0)
>  		gro_pull_from_frag0(skb, grow);
>  ok:
> +	if (napi->gro_hash[hash].count)
> +		if (!test_bit(hash, &napi->gro_bitmask))
> +			set_bit(hash, &napi->gro_bitmask);
> +	else if (test_bit(hash, &napi->gro_bitmask))
> +		clear_bit(hash, &napi->gro_bitmask);

This might not do what you want.

-- 
Stefano

Re: [PATCH] net: convert gro_count to bitmask

[Eric Dumazet]

On 07/11/2018 02:15 AM, Li RongQing wrote:
> gro_hash size is 192 bytes, and uses 3 cache lines, if there is few
> flows, gro_hash may be not fully used, so it is unnecessary to iterate
> all gro_hash in napi_gro_flush(), to occupy unnecessary cacheline.
> 
> convert gro_count to a bitmask, and rename it as gro_bitmask, each bit
> represents a element of gro_hash, only flush a gro_hash element if the
> related bit is set, to speed up napi_gro_flush().
> 
> and update gro_bitmask only if it will be changed, to reduce cache
> update
> 
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>  include/linux/netdevice.h |  2 +-
>  net/core/dev.c            | 35 +++++++++++++++++++++++------------
>  2 files changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index b683971e500d..df49b36ef378 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -322,7 +322,7 @@ struct napi_struct {
>  
>  	unsigned long		state;
>  	int			weight;
> -	unsigned int		gro_count;
> +	unsigned long		gro_bitmask;
>  	int			(*poll)(struct napi_struct *, int);
>  #ifdef CONFIG_NETPOLL
>  	int			poll_owner;
> diff --git a/net/core/dev.c b/net/core/dev.c
> index d13cddcac41f..a08dbdd217a6 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5171,9 +5171,11 @@ static void __napi_gro_flush_chain(struct napi_struct *napi, u32 index,
>  			return;
>  		list_del_init(&skb->list);
>  		napi_gro_complete(skb);
> -		napi->gro_count--;
>  		napi->gro_hash[index].count--;
>  	}
> +
> +	if (!napi->gro_hash[index].count)
> +		clear_bit(index, &napi->gro_bitmask);

I suggest you not add an atomic operation here.

Current cpu owns this NAPI after all.

Same remark for the whole patch.

->  __clear_bit(), __set_bit() and similar operators

Ideally you should provide TCP_RR number with busy polling enabled, to eventually catch regressions.

Thanks.

答复: [PATCH] net: convert gro_count to bitmask

[Li,Rongqing]

> -----邮件原件-----
> 发件人: Stefano Brivio [mailto:sbrivio@redhat.com]
> 发送时间: 2018年7月11日 18:52
> 收件人: Li,Rongqing <lirongqing@baidu.com>
> 抄送: netdev@vger.kernel.org; Eric Dumazet <edumazet@google.com>
> 主题: Re: [PATCH] net: convert gro_count to bitmask
> 
> On Wed, 11 Jul 2018 17:15:53 +0800
> Li RongQing <lirongqing@baidu.com> wrote:
> 
> > @@ -5380,6 +5382,12 @@ static enum gro_result dev_gro_receive(struct
> napi_struct *napi, struct sk_buff
> >  	if (grow > 0)
> >  		gro_pull_from_frag0(skb, grow);
> >  ok:
> > +	if (napi->gro_hash[hash].count)
> > +		if (!test_bit(hash, &napi->gro_bitmask))
> > +			set_bit(hash, &napi->gro_bitmask);
> > +	else if (test_bit(hash, &napi->gro_bitmask))
> > +		clear_bit(hash, &napi->gro_bitmask);
> 
> This might not do what you want.
> 
> --

could you show detail ?

-RongQing

> Stefano

答复: [PATCH] net: convert gro_count to bitmask

[Li,Rongqing]

> -----邮件原件-----
> 发件人: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> 发送时间: 2018年7月11日 19:32
> 收件人: Li,Rongqing <lirongqing@baidu.com>; netdev@vger.kernel.org
> 主题: Re: [PATCH] net: convert gro_count to bitmask
> 
> 
> 
> On 07/11/2018 02:15 AM, Li RongQing wrote:
> > gro_hash size is 192 bytes, and uses 3 cache lines, if there is few
> > flows, gro_hash may be not fully used, so it is unnecessary to iterate
> > all gro_hash in napi_gro_flush(), to occupy unnecessary cacheline.
> >
> > convert gro_count to a bitmask, and rename it as gro_bitmask, each bit
> > represents a element of gro_hash, only flush a gro_hash element if the
> > related bit is set, to speed up napi_gro_flush().
> >
> > and update gro_bitmask only if it will be changed, to reduce cache
> > update
> >
> > Suggested-by: Eric Dumazet <edumazet@google.com>
> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > ---
> >  include/linux/netdevice.h |  2 +-
> >  net/core/dev.c            | 35 +++++++++++++++++++++++------------
> >  2 files changed, 24 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index b683971e500d..df49b36ef378 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -322,7 +322,7 @@ struct napi_struct {
> >
> >  	unsigned long		state;
> >  	int			weight;
> > -	unsigned int		gro_count;
> > +	unsigned long		gro_bitmask;
> >  	int			(*poll)(struct napi_struct *, int);
> >  #ifdef CONFIG_NETPOLL
> >  	int			poll_owner;
> > diff --git a/net/core/dev.c b/net/core/dev.c index
> > d13cddcac41f..a08dbdd217a6 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -5171,9 +5171,11 @@ static void __napi_gro_flush_chain(struct
> napi_struct *napi, u32 index,
> >  			return;
> >  		list_del_init(&skb->list);
> >  		napi_gro_complete(skb);
> > -		napi->gro_count--;
> >  		napi->gro_hash[index].count--;
> >  	}
> > +
> > +	if (!napi->gro_hash[index].count)
> > +		clear_bit(index, &napi->gro_bitmask);
> 
> I suggest you not add an atomic operation here.
> 
> Current cpu owns this NAPI after all.
> 
> Same remark for the whole patch.
> 
> ->  __clear_bit(), __set_bit() and similar operators
> 
> Ideally you should provide TCP_RR number with busy polling enabled, to
> eventually catch regressions.
> 

I will change and do the test
Thank you.

-RongQing

> Thanks.

Re: [PATCH] net: convert gro_count to bitmask

[David Miller]

From: Li RongQing <lirongqing@baidu.com>
Date: Wed, 11 Jul 2018 17:15:53 +0800

> +		clear_bit(index, &napi->gro_bitmask);

Please don't use atomics here, at least use __clear_bit().

This is why I did the operations by hand in my version of the patch.
Also, if you are going to preempt my patch, at least retain the
comment I added around the GRO_HASH_BUCKETS definitions which warns
the reader about the limit.

Thanks.

答复: [PATCH] net: convert gro_count to bitmask

[Li,Rongqing]

> -----邮件原件-----
> 发件人: David Miller [mailto:davem@davemloft.net]
> 发送时间: 2018年7月12日 10:49
> 收件人: Li,Rongqing <lirongqing@baidu.com>
> 抄送: netdev@vger.kernel.org
> 主题: Re: [PATCH] net: convert gro_count to bitmask
> 
> From: Li RongQing <lirongqing@baidu.com>
> Date: Wed, 11 Jul 2018 17:15:53 +0800
> 
> > +		clear_bit(index, &napi->gro_bitmask);
> 
> Please don't use atomics here, at least use __clear_bit().
> 

Thanks, this is same as Eric's suggestion.


> This is why I did the operations by hand in my version of the patch.
> Also, if you are going to preempt my patch, at least retain the comment I
> added around the GRO_HASH_BUCKETS definitions which warns the reader
> about the limit.
> 

I add BUILD_BUG_ON in netdev_init, so I think we need not to add comment

@@ -9151,6 +9159,9 @@ static struct hlist_head * __net_init netdev_create_hash(void)
 /* Initialize per network namespace state */  static int __net_init netdev_init(struct net *net)  {
+	BUILD_BUG_ON(GRO_HASH_BUCKETS >
+			FIELD_SIZEOF(struct napi_struct, gro_bitmask));
+


-RongQing

> Thanks.

Re: 答复: [PATCH] net: convert gro_count to bitmask

[David Miller]

From: "Li,Rongqing" <lirongqing@baidu.com>
Date: Thu, 12 Jul 2018 03:03:51 +0000

> 
> 
>> -----邮件原件-----
>> 发件人: David Miller [mailto:davem@davemloft.net]
>> 发送时间: 2018年7月12日 10:49
>> 收件人: Li,Rongqing <lirongqing@baidu.com>
>> 抄送: netdev@vger.kernel.org
>> 主题: Re: [PATCH] net: convert gro_count to bitmask
>> 
>> From: Li RongQing <lirongqing@baidu.com>
>> Date: Wed, 11 Jul 2018 17:15:53 +0800
>> 
>> > +		clear_bit(index, &napi->gro_bitmask);
>> 
>> Please don't use atomics here, at least use __clear_bit().
>> 
> 
> Thanks, this is same as Eric's suggestion.
> 
> 
>> This is why I did the operations by hand in my version of the patch.
>> Also, if you are going to preempt my patch, at least retain the comment I
>> added around the GRO_HASH_BUCKETS definitions which warns the reader
>> about the limit.
>> 
> 
> I add BUILD_BUG_ON in netdev_init, so I think we need not to add comment

That's a good compile time check, but the person thinking about editing
the definition doesn't see the limit in the header file nor know why
the limit exists in the first place.

Re: [PATCH] net: convert gro_count to bitmask

[Stefano Brivio]

On Thu, 12 Jul 2018 02:31:10 +0000
"Li,Rongqing" <lirongqing@baidu.com> wrote:

> > -----邮件原件-----
> > 发件人: Stefano Brivio [mailto:sbrivio@redhat.com]
> > 发送时间: 2018年7月11日 18:52
> > 收件人: Li,Rongqing <lirongqing@baidu.com>
> > 抄送: netdev@vger.kernel.org; Eric Dumazet <edumazet@google.com>
> > 主题: Re: [PATCH] net: convert gro_count to bitmask
> > 
> > On Wed, 11 Jul 2018 17:15:53 +0800
> > Li RongQing <lirongqing@baidu.com> wrote:
> >   
> > > @@ -5380,6 +5382,12 @@ static enum gro_result dev_gro_receive(struct  
> > napi_struct *napi, struct sk_buff  
> > >  	if (grow > 0)
> > >  		gro_pull_from_frag0(skb, grow);
> > >  ok:
> > > +	if (napi->gro_hash[hash].count)
> > > +		if (!test_bit(hash, &napi->gro_bitmask))
> > > +			set_bit(hash, &napi->gro_bitmask);
> > > +	else if (test_bit(hash, &napi->gro_bitmask))
> > > +		clear_bit(hash, &napi->gro_bitmask);  
> > 
> > This might not do what you want.
> > 
> > --  
> 
> could you show detail ?

$ cat if1.c; gcc -o if1 if1.c
#include <stdio.h>

int main()
{
	if (1)
		if (0)
			;
	else if (2)
		printf("whoops\n");

	return 0;
}
$ ./if1
whoops

$ cat if2.c; gcc -o if2 if2.c
#include <stdio.h>

int main()
{
	if (1) {
		if (0)
			;
	} else if (2) {
		printf("whoops\n");
	}

	return 0;
}
$ ./if2

-- 
Stefano