diff mbox series

[net-next,v2] net: convert gro_count to bitmask

Message ID 1531464096-11319-1-git-send-email-lirongqing@baidu.com
State Accepted, archived
Delegated to: David Miller
Headers show
Series [net-next,v2] net: convert gro_count to bitmask | expand

Commit Message

Li RongQing July 13, 2018, 6:41 a.m. UTC 
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>
Cc: Stefano Brivio <sbrivio@redhat.com>
---
netperf shows no difference, maybe because my testing machine has large
cache
 
 include/linux/netdevice.h |  9 +++++++--
 net/core/dev.c            | 36 ++++++++++++++++++++++++------------
 2 files changed, 31 insertions(+), 14 deletions(-)

Comments

David Miller July 16, 2018, 8:41 p.m. UTC | #1 
From: Li RongQing <lirongqing@baidu.com>
Date: Fri, 13 Jul 2018 14:41:36 +0800

> 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>
> Cc: Stefano Brivio <sbrivio@redhat.com>
> ---
> netperf shows no difference, maybe because my testing machine has large
> cache

Applied.
Eric Dumazet July 16, 2018, 11:40 p.m. UTC | #2 
On 07/12/2018 11:41 PM, Li RongQing wrote:
> gro_hash size is 192 bytes, and uses 3 cache lines, if there is few

		 */
> @@ -9264,6 +9273,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));
> +

Sorry for the delay (patch is already merged)

This looks wrong to me. 

FIELD_SIZEOF() is in bytes not bits.

I guess we could either use BITS_PER_LONG or :

diff --git a/net/core/dev.c b/net/core/dev.c
index c883b17ee0fe2c8a7ca2f2867560ba74004790a7..4f8b92d81d107fc9acd2499297435cbd9e9b5c67 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9282,7 +9282,7 @@ static struct hlist_head * __net_init netdev_create_hash(void)
 static int __net_init netdev_init(struct net *net)
 {
        BUILD_BUG_ON(GRO_HASH_BUCKETS >
-                       FIELD_SIZEOF(struct napi_struct, gro_bitmask));
+                    8 * FIELD_SIZEOF(struct napi_struct, gro_bitmask));
 
        if (net != &init_net)
                INIT_LIST_HEAD(&net->dev_base_head);
David Miller July 17, 2018, 12:02 a.m. UTC | #3 
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 16 Jul 2018 16:40:52 -0700

> I guess we could either use BITS_PER_LONG or :
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index c883b17ee0fe2c8a7ca2f2867560ba74004790a7..4f8b92d81d107fc9acd2499297435cbd9e9b5c67 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -9282,7 +9282,7 @@ static struct hlist_head * __net_init netdev_create_hash(void)

Commited thusly:

====================
[PATCH] net: Fix GRO_HASH_BUCKETS assertion.

FIELD_SIZEOF() is in bytes, but we want bits.

Fixes: d9f37d01e294 ("net: convert gro_count to bitmask")
Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/core/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index c883b17ee0fe..4f8b92d81d10 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9282,7 +9282,7 @@ static struct hlist_head * __net_init netdev_create_hash(void)
 static int __net_init netdev_init(struct net *net)
 {
 	BUILD_BUG_ON(GRO_HASH_BUCKETS >
-			FIELD_SIZEOF(struct napi_struct, gro_bitmask));
+		     8 * FIELD_SIZEOF(struct napi_struct, gro_bitmask));
 
 	if (net != &init_net)
 		INIT_LIST_HEAD(&net->dev_base_head);
kernel test robot July 17, 2018, 1:04 a.m. UTC | #4 
Hi Li,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on next-20180713]
[cannot apply to v4.18-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Li-RongQing/net-convert-gro_count-to-bitmask/20180715-233722
config: i386-randconfig-s1-201828 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 
:::::: branch date: 15 hours ago
:::::: commit date: 15 hours ago

All errors (new ones prefixed by >>):

   In file included from arch/x86/include/asm/current.h:5:0,
                    from include/linux/sched.h:12,
                    from include/linux/uaccess.h:5,
                    from net/core/dev.c:75:
   net/core/dev.c: In function 'netdev_init':
>> include/linux/compiler.h:339:38: error: call to '__compiletime_assert_9285' declared with attribute error: BUILD_BUG_ON failed: GRO_HASH_BUCKETS > FIELD_SIZEOF(struct napi_struct, gro_bitmask)
     _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
                                         ^
   include/linux/compiler.h:319:4: note: in definition of macro '__compiletime_assert'
       prefix ## suffix();    \
       ^~~~~~
   include/linux/compiler.h:339:2: note: in expansion of macro '_compiletime_assert'
     _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
     ^~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:45:37: note: in expansion of macro 'compiletime_assert'
    #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
                                        ^~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:69:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
     BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
     ^~~~~~~~~~~~~~~~
   net/core/dev.c:9284:2: note: in expansion of macro 'BUILD_BUG_ON'
     BUILD_BUG_ON(GRO_HASH_BUCKETS >
     ^~~~~~~~~~~~
--
   In file included from arch/x86/include/asm/current.h:5:0,
                    from include/linux/sched.h:12,
                    from include/linux/uaccess.h:5,
                    from net//core/dev.c:75:
   net//core/dev.c: In function 'netdev_init':
>> include/linux/compiler.h:339:38: error: call to '__compiletime_assert_9285' declared with attribute error: BUILD_BUG_ON failed: GRO_HASH_BUCKETS > FIELD_SIZEOF(struct napi_struct, gro_bitmask)
     _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
                                         ^
   include/linux/compiler.h:319:4: note: in definition of macro '__compiletime_assert'
       prefix ## suffix();    \
       ^~~~~~
   include/linux/compiler.h:339:2: note: in expansion of macro '_compiletime_assert'
     _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
     ^~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:45:37: note: in expansion of macro 'compiletime_assert'
    #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
                                        ^~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:69:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
     BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
     ^~~~~~~~~~~~~~~~
   net//core/dev.c:9284:2: note: in expansion of macro 'BUILD_BUG_ON'
     BUILD_BUG_ON(GRO_HASH_BUCKETS >
     ^~~~~~~~~~~~

# https://github.com/0day-ci/linux/commit/b4ba3db381100e1869270a58dd2d9950ef0923de
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout b4ba3db381100e1869270a58dd2d9950ef0923de
vim +/__compiletime_assert_9285 +339 include/linux/compiler.h

9a8ab1c3 Daniel Santos 2013-02-21  325  
9a8ab1c3 Daniel Santos 2013-02-21  326  #define _compiletime_assert(condition, msg, prefix, suffix) \
9a8ab1c3 Daniel Santos 2013-02-21  327  	__compiletime_assert(condition, msg, prefix, suffix)
9a8ab1c3 Daniel Santos 2013-02-21  328  
9a8ab1c3 Daniel Santos 2013-02-21  329  /**
9a8ab1c3 Daniel Santos 2013-02-21  330   * compiletime_assert - break build and emit msg if condition is false
9a8ab1c3 Daniel Santos 2013-02-21  331   * @condition: a compile-time constant condition to check
9a8ab1c3 Daniel Santos 2013-02-21  332   * @msg:       a message to emit if condition is false
9a8ab1c3 Daniel Santos 2013-02-21  333   *
9a8ab1c3 Daniel Santos 2013-02-21  334   * In tradition of POSIX assert, this macro will break the build if the
9a8ab1c3 Daniel Santos 2013-02-21  335   * supplied condition is *false*, emitting the supplied error message if the
9a8ab1c3 Daniel Santos 2013-02-21  336   * compiler has support to do so.
9a8ab1c3 Daniel Santos 2013-02-21  337   */
9a8ab1c3 Daniel Santos 2013-02-21  338  #define compiletime_assert(condition, msg) \
9a8ab1c3 Daniel Santos 2013-02-21 @339  	_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
9a8ab1c3 Daniel Santos 2013-02-21  340  

:::::: The code at line 339 was first introduced by commit
:::::: 9a8ab1c39970a4938a72d94e6fd13be88a797590 bug.h, compiler.h: introduce compiletime_assert & BUILD_BUG_ON_MSG

:::::: TO: Daniel Santos <daniel.santos@pobox.com>
:::::: CC: Linus Torvalds <torvalds@linux-foundation.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
diff mbox series

Patch

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 2daf2fa6554f..8837a998de3f 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -308,9 +308,14 @@  struct gro_list {
 };
 
 /*
- * Structure for NAPI scheduling similar to tasklet but with weighting
+ * size of gro hash buckets, must less than bit number of
+ * napi_struct::gro_bitmask
  */
 #define GRO_HASH_BUCKETS	8
+
+/*
+ * Structure for NAPI scheduling similar to tasklet but with weighting
+ */
 struct napi_struct {
 	/* The poll_list must only be managed by the entity which
 	 * changes the state of the NAPI_STATE_SCHED bit.  This means
@@ -322,7 +327,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 14a748ee8cc9..e39fef62e285 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5283,9 +5283,11 @@  static void __napi_gro_flush_chain(struct napi_struct *napi, u32 index,
 		list_del(&skb->list);
 		skb->next = NULL;
 		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.
@@ -5296,8 +5298,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);
 
@@ -5389,8 +5393,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);
@@ -5465,7 +5469,6 @@  static enum gro_result dev_gro_receive(struct napi_struct *napi, struct sk_buff
 		list_del(&pp->list);
 		pp->next = NULL;
 		napi_gro_complete(pp);
-		napi->gro_count--;
 		napi->gro_hash[hash].count--;
 	}
 
@@ -5478,7 +5481,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;
@@ -5493,6 +5495,13 @@  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:
@@ -5891,7 +5900,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)
@@ -6100,7 +6109,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);
 
@@ -6115,7 +6124,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;
@@ -6175,7 +6184,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);
 
@@ -6217,7 +6226,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.
 		 */
@@ -9264,6 +9273,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);