| Message ID | 1474812524-29089-1-git-send-email-hadarh@mellanox.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
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 <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);
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);
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
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
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!
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!
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 }
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(-)