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 |
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.
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);
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);
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 --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);
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(-)