diff mbox series

[net-next] net: sched: refactor flower walk to iterate over idr

Message ID 1531132151-2321-1-git-send-email-vladbu@mellanox.com
State Accepted, archived
Delegated to: David Miller
Headers show
Series [net-next] net: sched: refactor flower walk to iterate over idr | expand

Commit Message

Vlad Buslov July 9, 2018, 10:29 a.m. UTC 
Extend struct tcf_walker with additional 'cookie' field. It is intended to
be used by classifier walk implementations to continue iteration directly
from particular filter, instead of iterating 'skip' number of times.

Change flower walk implementation to save filter handle in 'cookie'. Each
time flower walk is called, it looks up filter with saved handle directly
with idr, instead of iterating over filter linked list 'skip' number of
times. This change improves complexity of dumping flower classifier from
quadratic to linearithmic. (assuming idr lookup has logarithmic complexity)

Reviewed-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
 include/net/pkt_cls.h  |  1 +
 net/sched/cls_api.c    |  2 ++
 net/sched/cls_flower.c | 20 +++++++++-----------
 3 files changed, 12 insertions(+), 11 deletions(-)

Comments

Simon Horman July 10, 2018, 1:55 p.m. UTC | #1 
On Mon, Jul 09, 2018 at 01:29:11PM +0300, Vlad Buslov wrote:
> Extend struct tcf_walker with additional 'cookie' field. It is intended to
> be used by classifier walk implementations to continue iteration directly
> from particular filter, instead of iterating 'skip' number of times.
> 
> Change flower walk implementation to save filter handle in 'cookie'. Each
> time flower walk is called, it looks up filter with saved handle directly
> with idr, instead of iterating over filter linked list 'skip' number of
> times. This change improves complexity of dumping flower classifier from
> quadratic to linearithmic. (assuming idr lookup has logarithmic complexity)
> 
> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>

Reported-by: Simon Horman <simon.horman@netronome.com>

Thanks, I'm very pleased to see this change. I would appreciate it if
we could have a little time to test its impact on performance thoroughly.

One question: will this work as expected (i.e. be at least backwards
compatible) with existing user-space code?
Vlad Buslov July 10, 2018, 5:02 p.m. UTC | #2 
On Tue 10 Jul 2018 at 13:55, Simon Horman <simon.horman@netronome.com> wrote:
> On Mon, Jul 09, 2018 at 01:29:11PM +0300, Vlad Buslov wrote:
>> Extend struct tcf_walker with additional 'cookie' field. It is intended to
>> be used by classifier walk implementations to continue iteration directly
>> from particular filter, instead of iterating 'skip' number of times.
>> 
>> Change flower walk implementation to save filter handle in 'cookie'. Each
>> time flower walk is called, it looks up filter with saved handle directly
>> with idr, instead of iterating over filter linked list 'skip' number of
>> times. This change improves complexity of dumping flower classifier from
>> quadratic to linearithmic. (assuming idr lookup has logarithmic complexity)
>> 
>> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
>> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
>
> Reported-by: Simon Horman <simon.horman@netronome.com>
>
> Thanks, I'm very pleased to see this change. I would appreciate it if
> we could have a little time to test its impact on performance
> thoroughly.

For me it reduced time needed to dump 5m flows to ~50 seconds. Not a
very thorough benchmark, but performance improvement was so dramatic
that I decided to not investigate further.

>
> One question: will this work as expected (i.e. be at least backwards
> compatible) with existing user-space code?

I considered that and didn't find any reason why it would break
compatibility. Basically with current flower flows are dumped in
arbitrary order, and with this change dump(accidentally) outputs flows
in sorted ascending order. User space shouldn't expect any ordering in
dumped flows anyway, right?
Simon Horman July 13, 2018, 11:16 a.m. UTC | #3 
On Tue, Jul 10, 2018 at 08:02:10PM +0300, Vlad Buslov wrote:
> 
> On Tue 10 Jul 2018 at 13:55, Simon Horman <simon.horman@netronome.com> wrote:
> > On Mon, Jul 09, 2018 at 01:29:11PM +0300, Vlad Buslov wrote:
> >> Extend struct tcf_walker with additional 'cookie' field. It is intended to
> >> be used by classifier walk implementations to continue iteration directly
> >> from particular filter, instead of iterating 'skip' number of times.
> >> 
> >> Change flower walk implementation to save filter handle in 'cookie'. Each
> >> time flower walk is called, it looks up filter with saved handle directly
> >> with idr, instead of iterating over filter linked list 'skip' number of
> >> times. This change improves complexity of dumping flower classifier from
> >> quadratic to linearithmic. (assuming idr lookup has logarithmic complexity)
> >> 
> >> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
> >> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
> >
> > Reported-by: Simon Horman <simon.horman@netronome.com>
> >
> > Thanks, I'm very pleased to see this change. I would appreciate it if
> > we could have a little time to test its impact on performance
> > thoroughly.
> 
> For me it reduced time needed to dump 5m flows to ~50 seconds. Not a
> very thorough benchmark, but performance improvement was so dramatic
> that I decided to not investigate further.

Thanks, we have also now measured a similar improvement in performance.

> > One question: will this work as expected (i.e. be at least backwards
> > compatible) with existing user-space code?
> 
> I considered that and didn't find any reason why it would break
> compatibility. Basically with current flower flows are dumped in
> arbitrary order, and with this change dump(accidentally) outputs flows
> in sorted ascending order. User space shouldn't expect any ordering in
> dumped flows anyway, right?

Agreed. I don't think we need to be worried about changes in
the order of dumped flows.

Reviewed-by: Simon Horman <simon.horman@netronome.com>
David Miller July 14, 2018, 1:24 a.m. UTC | #4 
From: Vlad Buslov <vladbu@mellanox.com>
Date: Mon,  9 Jul 2018 13:29:11 +0300

> Extend struct tcf_walker with additional 'cookie' field. It is intended to
> be used by classifier walk implementations to continue iteration directly
> from particular filter, instead of iterating 'skip' number of times.
> 
> Change flower walk implementation to save filter handle in 'cookie'. Each
> time flower walk is called, it looks up filter with saved handle directly
> with idr, instead of iterating over filter linked list 'skip' number of
> times. This change improves complexity of dumping flower classifier from
> quadratic to linearithmic. (assuming idr lookup has logarithmic complexity)
> 
> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>

Applied, thank you.
Or Gerlitz July 14, 2018, 3:50 p.m. UTC | #5 
On Tue, Jul 10, 2018 at 8:02 PM, Vlad Buslov <vladbu@mellanox.com> wrote:
>
> On Tue 10 Jul 2018 at 13:55, Simon Horman <simon.horman@netronome.com> wrote:
>> On Mon, Jul 09, 2018 at 01:29:11PM +0300, Vlad Buslov wrote:
>>> Extend struct tcf_walker with additional 'cookie' field. It is intended to
>>> be used by classifier walk implementations to continue iteration directly
>>> from particular filter, instead of iterating 'skip' number of times.
>>>
>>> Change flower walk implementation to save filter handle in 'cookie'. Each
>>> time flower walk is called, it looks up filter with saved handle directly
>>> with idr, instead of iterating over filter linked list 'skip' number of
>>> times. This change improves complexity of dumping flower classifier from
>>> quadratic to linearithmic. (assuming idr lookup has logarithmic complexity)
>>>
>>> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
>>> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
>>
>> Reported-by: Simon Horman <simon.horman@netronome.com>
>>
>> Thanks, I'm very pleased to see this change. I would appreciate it if
>> we could have a little time to test its impact on performance
>> thoroughly.
>
> For me it reduced time needed to dump 5m flows to ~50 seconds. Not a
> very thorough benchmark, but performance improvement was so dramatic
> that I decided to not investigate further.


down from how long to 50 seconds??
Vlad Buslov July 16, 2018, 7:03 a.m. UTC | #6 
On Sat 14 Jul 2018 at 15:50, Or Gerlitz <gerlitz.or@gmail.com> wrote:
> On Tue, Jul 10, 2018 at 8:02 PM, Vlad Buslov <vladbu@mellanox.com> wrote:
>>
>> On Tue 10 Jul 2018 at 13:55, Simon Horman <simon.horman@netronome.com> wrote:
>>> On Mon, Jul 09, 2018 at 01:29:11PM +0300, Vlad Buslov wrote:
>>>> Extend struct tcf_walker with additional 'cookie' field. It is intended to
>>>> be used by classifier walk implementations to continue iteration directly
>>>> from particular filter, instead of iterating 'skip' number of times.
>>>>
>>>> Change flower walk implementation to save filter handle in 'cookie'. Each
>>>> time flower walk is called, it looks up filter with saved handle directly
>>>> with idr, instead of iterating over filter linked list 'skip' number of
>>>> times. This change improves complexity of dumping flower classifier from
>>>> quadratic to linearithmic. (assuming idr lookup has logarithmic complexity)
>>>>
>>>> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
>>>> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
>>>
>>> Reported-by: Simon Horman <simon.horman@netronome.com>
>>>
>>> Thanks, I'm very pleased to see this change. I would appreciate it if
>>> we could have a little time to test its impact on performance
>>> thoroughly.
>>
>> For me it reduced time needed to dump 5m flows to ~50 seconds. Not a
>> very thorough benchmark, but performance improvement was so dramatic
>> that I decided to not investigate further.
>
>
> down from how long to 50 seconds??

From this:

gact$ time sudo tc -s filter show dev eth0_0 ingress | grep in_hw | wc -l
5000000                                                                  
                                                                         
real    2099m29.689s                                                     
user    0m42.036s                                                        
sys     2098m55.680s
diff mbox series

Patch

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index a3c1a2c47cd4..ff7708c40e22 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -13,6 +13,7 @@  struct tcf_walker {
 	int	stop;
 	int	skip;
 	int	count;
+	unsigned long cookie;
 	int	(*fn)(struct tcf_proto *, void *node, struct tcf_walker *);
 };
 
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index cdc3c87c53e6..9aa70e2035dc 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -1463,7 +1463,9 @@  static bool tcf_chain_dump(struct tcf_chain *chain, struct Qdisc *q, u32 parent,
 		arg.w.stop = 0;
 		arg.w.skip = cb->args[1] - 1;
 		arg.w.count = 0;
+		arg.w.cookie = cb->args[2];
 		tp->ops->walk(tp, &arg.w);
+		cb->args[2] = arg.w.cookie;
 		cb->args[1] = arg.w.count + 1;
 		if (arg.w.stop)
 			return false;
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 2b5be42a9f1c..754def20b432 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -1058,19 +1058,17 @@  static void fl_walk(struct tcf_proto *tp, struct tcf_walker *arg)
 {
 	struct cls_fl_head *head = rtnl_dereference(tp->root);
 	struct cls_fl_filter *f;
-	struct fl_flow_mask *mask;
 
-	list_for_each_entry_rcu(mask, &head->masks, list) {
-		list_for_each_entry_rcu(f, &mask->filters, list) {
-			if (arg->count < arg->skip)
-				goto skip;
-			if (arg->fn(tp, f, arg) < 0) {
-				arg->stop = 1;
-				break;
-			}
-skip:
-			arg->count++;
+	arg->count = arg->skip;
+
+	while ((f = idr_get_next_ul(&head->handle_idr,
+				    &arg->cookie)) != NULL) {
+		if (arg->fn(tp, f, arg) < 0) {
+			arg->stop = 1;
+			break;
 		}
+		arg->cookie = f->handle + 1;
+		arg->count++;
 	}
 }