netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [Patch net-next] net_sched: get rid of rcu_barrier() in tcf_block_put_ext()
@ 2017-12-02  0:18 Cong Wang
  2017-12-02  9:21 ` Jiri Pirko
  0 siblings, 1 reply; 3+ messages in thread
From: Cong Wang @ 2017-12-02  0:18 UTC (permalink / raw)
  To: netdev; +Cc: pabeni, Cong Wang, Eric Dumazet, Jiri Pirko, Jamal Hadi Salim

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.

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)
+		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);
 
-- 
2.13.0

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [Patch net-next] net_sched: get rid of rcu_barrier() in tcf_block_put_ext()
  2017-12-02  0:18 [Patch net-next] net_sched: get rid of rcu_barrier() in tcf_block_put_ext() Cong Wang
@ 2017-12-02  9:21 ` Jiri Pirko
  2017-12-03 19:32   ` Cong Wang
  0 siblings, 1 reply; 3+ messages in thread
From: Jiri Pirko @ 2017-12-02  9:21 UTC (permalink / raw)
  To: Cong Wang; +Cc: netdev, pabeni, Eric Dumazet, Jiri Pirko, Jamal Hadi Salim

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
>

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Patch net-next] net_sched: get rid of rcu_barrier() in tcf_block_put_ext()
  2017-12-02  9:21 ` Jiri Pirko
@ 2017-12-03 19:32   ` Cong Wang
  0 siblings, 0 replies; 3+ messages in thread
From: Cong Wang @ 2017-12-03 19:32 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: Linux Kernel Network Developers, Paolo Abeni, Eric Dumazet,
	Jiri Pirko, Jamal Hadi Salim

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!

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2017-12-03 19:32 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-02  0:18 [Patch net-next] net_sched: get rid of rcu_barrier() in tcf_block_put_ext() Cong Wang
2017-12-02  9:21 ` Jiri Pirko
2017-12-03 19:32   ` Cong Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).