netdev - Re: [Patch net-next] net_sched: get rid of rcu_barrier() in tcf_block_put

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 2 Dec 2017 10:21:57 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     netdev@...r.kernel.org, pabeni@...hat.com,
        Eric Dumazet <edumazet@...gle.com>,
        Jiri Pirko <jiri@...lanox.com>,
        Jamal Hadi Salim <jhs@...atatu.com>
Subject: Re: [Patch net-next] net_sched: get rid of rcu_barrier() in
 tcf_block_put_ext()

Sat, Dec 02, 2017 at 01:18:04AM CET, xiyou.wangcong@...il.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@...hat.com>
>Cc: Eric Dumazet <edumazet@...gle.com>
>Cc: Jiri Pirko <jiri@...lanox.com>
>Cc: Jamal Hadi Salim <jhs@...atatu.com>
>Signed-off-by: Cong Wang <xiyou.wangcong@...il.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
>