[PATCH net-next] net/sched: pkt_cls: change tc actions order to be as the user sets

Currently the created tc actions list is reversed against the order
set by the user.
Change the actions list order to be the same as was set by the user.

Signed-off-by: Hadar Hen Zion <redacted>
---
 include/net/pkt_cls.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 5ccaa4b..767b03a 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h

@@ -123,7 +123,7 @@ static inline void tcf_exts_to_list(const struct tcf_exts *exts,
 	for (i = 0; i < exts->nr_actions; i++) {
 		struct tc_action *a = exts->actions[i];
 
-		list_add(&a->list, actions);
+		list_add_tail(&a->list, actions);
 	}
 #endif
 }

-- 
1.8.3.1

On 16-09-25 10:08 AM, Hadar Hen Zion wrote:

Currently the created tc actions list is reversed against the order
set by the user.
Change the actions list order to be the same as was set by the user.

Did something break? It seems to matter most for dumping. But even that
didnt breaking. Looking at the latest net tree, i tried:

====
sudo $TC filter add dev $ETH parent 1: protocol ip prio 10 u32 \
match ip protocol 1 0xff flowid 1:2 \
action skbedit prio 33 \
action ife encode type 0xDEAD \
use mark 12 allow prio dst 02:15:15:15:15:15

Then I dump:
=====
jhs@jhs-foobar:~$ sudo $TC -s filter ls dev $ETH parent 1: protocol ip
filter pref 10 u32
filter pref 10 u32 fh 800: ht divisor 1
filter pref 10 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:2 (rule hit 12 success 2)
  match 00010000/00ff0000 at 8 (success 2 )
	action order 1:  skbedit priority :33
	 index 2 ref 1 bind 1 installed 604 sec used first 589 sec last 583 sec
 	Action statistics:
	Sent 196 bytes 2 pkt (dropped 0, overlimits 0 requeues 0)
	backlog 0b 0p requeues 0

	action order 2: ife encode action pipe type 0xDEAD
	 use mark 12 allow prio dst 02:15:15:15:15:15
	 index 2 ref 1 bind 1 installed 604 sec used first 589 sec last 583 sec
	Action statistics:
	Sent 196 bytes 2 pkt (dropped 0, overlimits 0 requeues 0)
	backlog 0b 0p requeues 0

-----

cheers,
jamal

On Sun, Sep 25, 2016 at 7:39 AM, Jamal Hadi Salim [off-list ref] wrote:

On 16-09-25 10:08 AM, Hadar Hen Zion wrote:

quoted

Currently the created tc actions list is reversed against the order
set by the user.
Change the actions list order to be the same as was set by the user.

Did something break? It seems to matter most for dumping. But even that
didnt breaking. Looking at the latest net tree, i tried:

The reason is we use action->order as an nested attribute, so
the order in the list doesn't matter, only action->order itself matters.

See tcf_action_dump():

   nest = nla_nest_start(skb, a->order);

On Mon, Sep 26, 2016 at 7:31 AM, Cong Wang [off-list ref] wrote:

On Sun, Sep 25, 2016 at 7:39 AM, Jamal Hadi Salim [off-list ref] wrote:

quoted

On 16-09-25 10:08 AM, Hadar Hen Zion wrote:

quoted

Currently the created tc actions list is reversed against the order
set by the user.
Change the actions list order to be the same as was set by the user.

Did something break? It seems to matter most for dumping. But even that
didnt breaking. Looking at the latest net tree, i tried:

The reason is we use action->order as an nested attribute, so
the order in the list doesn't matter, only action->order itself matters.

The order in the list matters for offload drivers who use the
"tcf_exts_to_list" function and action->order parameter isn't usable
for them.
Why not keeping the actions in the same order as the user? isn't it
more elegant?

Hadar

See tcf_action_dump():

   nest = nla_nest_start(skb, a->order);

On 16-09-26 02:02 AM, Hadar Hen Zion wrote:

On Mon, Sep 26, 2016 at 7:31 AM, Cong Wang [off-list ref] wrote:

quoted

The reason is we use action->order as an nested attribute, so
the order in the list doesn't matter, only action->order itself matters.

The order in the list matters for offload drivers who use the
"tcf_exts_to_list" function and action->order parameter isn't usable
for them.

Ok, thanks for the explanation.

Why not keeping the actions in the same order as the user? isn't it
more elegant?

I dont think it was intentional. I will ACK the patch.

cheers,
jamal

On 16-09-25 10:08 AM, Hadar Hen Zion wrote:

Currently the created tc actions list is reversed against the order
set by the user.
Change the actions list order to be the same as was set by the user.

Signed-off-by: Hadar Hen Zion <redacted>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

On Sun, Sep 25, 2016 at 11:02 PM, Hadar Hen Zion
[off-list ref] wrote:

On Mon, Sep 26, 2016 at 7:31 AM, Cong Wang [off-list ref] wrote:

quoted

On Sun, Sep 25, 2016 at 7:39 AM, Jamal Hadi Salim [off-list ref] wrote:

quoted

On 16-09-25 10:08 AM, Hadar Hen Zion wrote:

quoted

Currently the created tc actions list is reversed against the order
set by the user.
Change the actions list order to be the same as was set by the user.

Did something break? It seems to matter most for dumping. But even that
didnt breaking. Looking at the latest net tree, i tried:

The reason is we use action->order as an nested attribute, so
the order in the list doesn't matter, only action->order itself matters.

The order in the list matters for offload drivers who use the
"tcf_exts_to_list" function and action->order parameter isn't usable
for them.
Why not keeping the actions in the same order as the user? isn't it
more elegant?

I don't object this patch since it affects offloading, I just explained
why it doesn't affect dumping.

Please add this to your changelog, to make it obvious.

Thanks!

On Mon, Sep 26, 2016 at 11:34 PM, Cong Wang [off-list ref] wrote:

On Sun, Sep 25, 2016 at 11:02 PM, Hadar Hen Zion
[off-list ref] wrote:

quoted

On Mon, Sep 26, 2016 at 7:31 AM, Cong Wang [off-list ref] wrote:

quoted

On Sun, Sep 25, 2016 at 7:39 AM, Jamal Hadi Salim [off-list ref] wrote:

quoted

On 16-09-25 10:08 AM, Hadar Hen Zion wrote:

quoted

Currently the created tc actions list is reversed against the order
set by the user.
Change the actions list order to be the same as was set by the user.

Did something break? It seems to matter most for dumping. But even that
didnt breaking. Looking at the latest net tree, i tried:

The reason is we use action->order as an nested attribute, so
the order in the list doesn't matter, only action->order itself matters.

The order in the list matters for offload drivers who use the
"tcf_exts_to_list" function and action->order parameter isn't usable
for them.
Why not keeping the actions in the same order as the user? isn't it
more elegant?

I don't object this patch since it affects offloading, I just explained
why it doesn't affect dumping.

Please add this to your changelog, to make it obvious.

Sure, I'll add it.

Hadar

Thanks!