Sat, Dec 02, 2017 at 01:18:04AM CET, xiyou.wangc...@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 <pab...@redhat.com>
>Cc: Eric Dumazet <eduma...@google.com>
>Cc: Jiri Pirko <j...@mellanox.com>
>Cc: Jamal Hadi Salim <j...@mojatatu.com>
>Signed-off-by: Cong Wang <xiyou.wangc...@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
>
