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

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

[Hadar Hen Zion]

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 <hadarh@mellanox.com>
---
 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

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

[Jamal Hadi Salim]

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

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

[Cong Wang]

On Sun, Sep 25, 2016 at 7:39 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> 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:
>

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

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

[Hadar Hen Zion]

On Mon, Sep 26, 2016 at 7:31 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Sun, Sep 25, 2016 at 7:39 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> 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:
>>
>
> 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);

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

[Jamal Hadi Salim]

On 16-09-26 02:02 AM, Hadar Hen Zion wrote:
> On Mon, Sep 26, 2016 at 7:31 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:

>>
>> 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

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

[Jamal Hadi Salim]

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 <hadarh@mellanox.com>

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

cheers,
jamal

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

[Cong Wang]

On Sun, Sep 25, 2016 at 11:02 PM, Hadar Hen Zion
<hadarh@dev.mellanox.co.il> wrote:
> On Mon, Sep 26, 2016 at 7:31 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Sun, Sep 25, 2016 at 7:39 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>> 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:
>>>
>>
>> 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!

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

[Hadar Hen Zion]

On Mon, Sep 26, 2016 at 11:34 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Sun, Sep 25, 2016 at 11:02 PM, Hadar Hen Zion
> <hadarh@dev.mellanox.co.il> wrote:
>> On Mon, Sep 26, 2016 at 7:31 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>> On Sun, Sep 25, 2016 at 7:39 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>>> 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:
>>>>
>>>
>>> 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!