Message ID | 20171202001804.11210-1-xiyou.wangcong@gmail.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
Series | [net-next] net_sched: get rid of rcu_barrier() in tcf_block_put_ext() | expand |
Sat, Dec 02, 2017 at 01:18:04AM CET, xiyou.wangcong@gmail.com wrote: >Both Eric and Paolo noticed the rcu_barrier() we use in >tcf_block_put_ext() could be a performance bottleneck when >we have lots of filters. The problem is not a lots of filters, the problem is lots of classes and therefore tcf_blocks > >Paolo provided the following to demonstrate the issue: > >tc qdisc add dev lo root htb >for I in `seq 1 1000`; do > tc class add dev lo parent 1: classid 1:$I htb rate 100kbit > tc qdisc add dev lo parent 1:$I handle $((I + 1)): htb > for J in `seq 1 10`; do > tc filter add dev lo parent $((I + 1)): u32 match ip src 1.1.1.$J > done >done >time tc qdisc del dev root > >real 0m54.764s >user 0m0.023s >sys 0m0.000s > >The rcu_barrier() there is to ensure we free the block after all chains >are gone, that is, to queue tcf_block_put_final() at the tail of workqueue. >We can achieve this ordering requirement by refcnt'ing tcf block instead, >that is, the tcf block is freed only when the last chain in this block is >gone. This also simplifies the code. > >Paolo reported after this patch we get: > >real 0m0.017s >user 0m0.000s >sys 0m0.017s > >Tested-by: Paolo Abeni <pabeni@redhat.com> >Cc: Eric Dumazet <edumazet@google.com> >Cc: Jiri Pirko <jiri@mellanox.com> >Cc: Jamal Hadi Salim <jhs@mojatatu.com> >Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> >--- > include/net/sch_generic.h | 2 +- > net/sched/cls_api.c | 31 +++++++++---------------------- > 2 files changed, 10 insertions(+), 23 deletions(-) > >diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h >index 65d0d25f2648..b013ded1a38d 100644 >--- a/include/net/sch_generic.h >+++ b/include/net/sch_generic.h >@@ -278,7 +278,7 @@ struct tcf_block { > struct net *net; > struct Qdisc *q; > struct list_head cb_list; >- struct work_struct work; >+ unsigned int nr_chains; > }; > > static inline void qdisc_cb_private_validate(const struct sk_buff *skb, int sz) >diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c >index ddcf04b4ab43..dec0d36078c8 100644 >--- a/net/sched/cls_api.c >+++ b/net/sched/cls_api.c >@@ -190,6 +190,7 @@ static struct tcf_chain *tcf_chain_create(struct tcf_block *block, > return NULL; > list_add_tail(&chain->list, &block->chain_list); > chain->block = block; >+ block->nr_chains++; > chain->index = chain_index; > chain->refcnt = 1; > return chain; >@@ -218,8 +219,12 @@ static void tcf_chain_flush(struct tcf_chain *chain) > > static void tcf_chain_destroy(struct tcf_chain *chain) > { >+ struct tcf_block *block = chain->block; >+ > list_del(&chain->list); > kfree(chain); >+ if (!--block->nr_chains) You don't need this counter. You can just check list_empty(block->chain_list); >+ kfree(block); > } > > static void tcf_chain_hold(struct tcf_chain *chain) >@@ -330,27 +335,13 @@ int tcf_block_get(struct tcf_block **p_block, > } > EXPORT_SYMBOL(tcf_block_get); > >-static void tcf_block_put_final(struct work_struct *work) >-{ >- struct tcf_block *block = container_of(work, struct tcf_block, work); >- struct tcf_chain *chain, *tmp; >- >- rtnl_lock(); >- >- /* At this point, all the chains should have refcnt == 1. */ >- list_for_each_entry_safe(chain, tmp, &block->chain_list, list) >- tcf_chain_put(chain); >- rtnl_unlock(); >- kfree(block); >-} >- > /* XXX: Standalone actions are not allowed to jump to any chain, and bound > * actions should be all removed after flushing. > */ > void tcf_block_put_ext(struct tcf_block *block, struct Qdisc *q, > struct tcf_block_ext_info *ei) > { >- struct tcf_chain *chain; >+ struct tcf_chain *chain, *tmp; > > /* Hold a refcnt for all chains, except 0, so that they don't disappear > * while we are iterating. >@@ -364,13 +355,9 @@ void tcf_block_put_ext(struct tcf_block *block, struct Qdisc *q, > > tcf_block_offload_unbind(block, q, ei); > >- INIT_WORK(&block->work, tcf_block_put_final); >- /* Wait for existing RCU callbacks to cool down, make sure their works >- * have been queued before this. We can not flush pending works here >- * because we are holding the RTNL lock. >- */ >- rcu_barrier(); >- tcf_queue_work(&block->work); >+ /* At this point, all the chains should have refcnt >= 1. */ >+ list_for_each_entry_safe(chain, tmp, &block->chain_list, list) >+ tcf_chain_put(chain); I think this is correct. Would be probably good to elaborate a bit more about what is happening. Perhaps a comment? > } > EXPORT_SYMBOL(tcf_block_put_ext); > >-- >2.13.0 >
On Sat, Dec 2, 2017 at 1:21 AM, Jiri Pirko <jiri@resnulli.us> wrote: > Sat, Dec 02, 2017 at 01:18:04AM CET, xiyou.wangcong@gmail.com wrote: >>Both Eric and Paolo noticed the rcu_barrier() we use in >>tcf_block_put_ext() could be a performance bottleneck when >>we have lots of filters. > > The problem is not a lots of filters, the problem is lots of classes and > therefore tcf_blocks Fixed. [...] >>@@ -218,8 +219,12 @@ static void tcf_chain_flush(struct tcf_chain *chain) >> >> static void tcf_chain_destroy(struct tcf_chain *chain) >> { >>+ struct tcf_block *block = chain->block; >>+ >> list_del(&chain->list); >> kfree(chain); >>+ if (!--block->nr_chains) > > You don't need this counter. You can just check > list_empty(block->chain_list); Makes sense! Done. [...] >>@@ -364,13 +355,9 @@ void tcf_block_put_ext(struct tcf_block *block, struct Qdisc *q, >> >> tcf_block_offload_unbind(block, q, ei); >> >>- INIT_WORK(&block->work, tcf_block_put_final); >>- /* Wait for existing RCU callbacks to cool down, make sure their works >>- * have been queued before this. We can not flush pending works here >>- * because we are holding the RTNL lock. >>- */ >>- rcu_barrier(); >>- tcf_queue_work(&block->work); >>+ /* At this point, all the chains should have refcnt >= 1. */ >>+ list_for_each_entry_safe(chain, tmp, &block->chain_list, list) >>+ tcf_chain_put(chain); > > I think this is correct. Would be probably good to elaborate a bit more > about what is happening. Perhaps a comment? OK, I will add a comment about this refcnt. Thanks!
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h index 65d0d25f2648..b013ded1a38d 100644 --- a/include/net/sch_generic.h +++ b/include/net/sch_generic.h @@ -278,7 +278,7 @@ struct tcf_block { struct net *net; struct Qdisc *q; struct list_head cb_list; - struct work_struct work; + unsigned int nr_chains; }; static inline void qdisc_cb_private_validate(const struct sk_buff *skb, int sz) diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index ddcf04b4ab43..dec0d36078c8 100644 --- a/net/sched/cls_api.c +++ b/net/sched/cls_api.c @@ -190,6 +190,7 @@ static struct tcf_chain *tcf_chain_create(struct tcf_block *block, return NULL; list_add_tail(&chain->list, &block->chain_list); chain->block = block; + block->nr_chains++; chain->index = chain_index; chain->refcnt = 1; return chain; @@ -218,8 +219,12 @@ static void tcf_chain_flush(struct tcf_chain *chain) static void tcf_chain_destroy(struct tcf_chain *chain) { + struct tcf_block *block = chain->block; + list_del(&chain->list); kfree(chain); + if (!--block->nr_chains) + kfree(block); } static void tcf_chain_hold(struct tcf_chain *chain) @@ -330,27 +335,13 @@ int tcf_block_get(struct tcf_block **p_block, } EXPORT_SYMBOL(tcf_block_get); -static void tcf_block_put_final(struct work_struct *work) -{ - struct tcf_block *block = container_of(work, struct tcf_block, work); - struct tcf_chain *chain, *tmp; - - rtnl_lock(); - - /* At this point, all the chains should have refcnt == 1. */ - list_for_each_entry_safe(chain, tmp, &block->chain_list, list) - tcf_chain_put(chain); - rtnl_unlock(); - kfree(block); -} - /* XXX: Standalone actions are not allowed to jump to any chain, and bound * actions should be all removed after flushing. */ void tcf_block_put_ext(struct tcf_block *block, struct Qdisc *q, struct tcf_block_ext_info *ei) { - struct tcf_chain *chain; + struct tcf_chain *chain, *tmp; /* Hold a refcnt for all chains, except 0, so that they don't disappear * while we are iterating. @@ -364,13 +355,9 @@ void tcf_block_put_ext(struct tcf_block *block, struct Qdisc *q, tcf_block_offload_unbind(block, q, ei); - INIT_WORK(&block->work, tcf_block_put_final); - /* Wait for existing RCU callbacks to cool down, make sure their works - * have been queued before this. We can not flush pending works here - * because we are holding the RTNL lock. - */ - rcu_barrier(); - tcf_queue_work(&block->work); + /* At this point, all the chains should have refcnt >= 1. */ + list_for_each_entry_safe(chain, tmp, &block->chain_list, list) + tcf_chain_put(chain); } EXPORT_SYMBOL(tcf_block_put_ext);