* Fwd: u32 ht filters [not found] <9c8f997d-339c-5088-0bb5-124e9f55f02d@itcare.pl> @ 2018-02-07 5:09 ` Cong Wang 2018-02-07 7:01 ` Jiri Pirko 0 siblings, 1 reply; 8+ messages in thread From: Cong Wang @ 2018-02-07 5:09 UTC (permalink / raw) To: Jiri Pirko; +Cc: Linux Kernel Network Developers, Paweł Staszewski Hi, Jiri Your commit 7fa9d974f3c2a016b9accb18f4ee2ed2a738585c breaks the tc script by Paweł. Please find below for details. commit 7fa9d974f3c2a016b9accb18f4ee2ed2a738585c Author: Jiri Pirko <jiri@mellanox.com> Date: Fri Oct 13 14:01:02 2017 +0200 net: sched: cls_u32: use block instead of q in tc_u_common tc_u_common is now per-q. With blocks, it has to be converted to be per-block. Signed-off-by: Jiri Pirko <jiri@mellanox.com> Signed-off-by: David S. Miller <davem@davemloft.net> Before this commit, u32 hashtables are per-qdisc, after this commit it becomes per-block or per-class... this is why the script below is broken. ---------- Forwarded message ---------- From: Paweł Staszewski <pstaszewski@itcare.pl> Date: Tue, Feb 6, 2018 at 8:05 AM Subject: u32 ht filters To: Cong Wang <xiyou.wangcong@gmail.com> Hi Is there something changed in kernek 4.15 that makes problem with old configuration of tc filters with hashing filters ? for example this : tc qdisc del root dev ifb1 tc qdisc add dev ifb1 root handle 1:0 hfsc default 8000 tc filter add dev ifb1 parent 1:0 protocol ip u32 tc class add dev ifb1 parent 1:0 classid 1:1 hfsc ls m2 10000Mbit ul m2 10000Mbit tc class add dev ifb1 parent 1:1 classid 1:2 hfsc ls m2 10000Mbit ul m2 10000Mbit tc class add dev ifb1 parent 1:1 classid 1:3 hfsc ls m2 5000Mbit ul m2 5000Mbit tc class add dev ifb1 parent 1:2 classid 1:8000 hfsc ls m2 10000Mbit ul m2 10000Mbit tc qdisc add dev ifb1 parent 1:8000 handle 8000: sfq perturb 60 tc qdisc add dev ifb1 parent 1:3 handle 3: pfifo limit 10000 tc filter add dev ifb1 protocol ip parent 1:0 handle 9: u32 divisor 256 tc filter add dev ifb1 protocol ip parent 1:0 u32 ht 800:: match ip dst 192.168.0.0/24 hashkey mask 0x000000ff at 16 link 9: tc class add dev ifb1 parent 1:2 classid 1:60 hfsc ls m2 8kbit ul m2 51200kbit echo 1 tc filter add dev ifb1 parent 1:2 protocol ip u32 ht 9:22 match ip dst 192.168.0.34 flowid 1:60 echo 2 tc qdisc add dev ifb1 parent 1:60 handle 60: pfifo limit 8192 Is working with 4.13 But it is not working with 4.15 error is when adding: tc filter add dev ifb1 protocol ip parent 1:2 prio 4 u32 ht 9:0x22 match ip dst 192.168.0.34 flowid 1:60 RTNETLINK answers: Invalid argument We have an error talking to the kernel Thanks Paweł Staszewski ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Fwd: u32 ht filters 2018-02-07 5:09 ` Fwd: u32 ht filters Cong Wang @ 2018-02-07 7:01 ` Jiri Pirko 2018-02-07 23:08 ` Cong Wang 0 siblings, 1 reply; 8+ messages in thread From: Jiri Pirko @ 2018-02-07 7:01 UTC (permalink / raw) To: Cong Wang Cc: Jiri Pirko, Linux Kernel Network Developers, Paweł Staszewski Wed, Feb 07, 2018 at 06:09:15AM CET, xiyou.wangcong@gmail.com wrote: >Hi, Jiri > >Your commit 7fa9d974f3c2a016b9accb18f4ee2ed2a738585c >breaks the tc script by Paweł. Please find below for details. Did you do the bisection? The commit just uses block struct instead of q, but since they are in 1:1 relation, that should be equvivalent. So basically you still have per-qdisc hashtables for u32. > > >commit 7fa9d974f3c2a016b9accb18f4ee2ed2a738585c >Author: Jiri Pirko <jiri@mellanox.com> >Date: Fri Oct 13 14:01:02 2017 +0200 > > net: sched: cls_u32: use block instead of q in tc_u_common > > tc_u_common is now per-q. With blocks, it has to be converted to be > per-block. > > Signed-off-by: Jiri Pirko <jiri@mellanox.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > >Before this commit, u32 hashtables are per-qdisc, after this commit >it becomes per-block or per-class... this is why the script below is broken. > > >---------- Forwarded message ---------- >From: Paweł Staszewski <pstaszewski@itcare.pl> >Date: Tue, Feb 6, 2018 at 8:05 AM >Subject: u32 ht filters >To: Cong Wang <xiyou.wangcong@gmail.com> > > >Hi > > >Is there something changed in kernek 4.15 that makes problem with old >configuration of tc filters with hashing filters ? > >for example this : > >tc qdisc del root dev ifb1 > >tc qdisc add dev ifb1 root handle 1:0 hfsc default 8000 >tc filter add dev ifb1 parent 1:0 protocol ip u32 >tc class add dev ifb1 parent 1:0 classid 1:1 hfsc ls m2 10000Mbit ul >m2 10000Mbit >tc class add dev ifb1 parent 1:1 classid 1:2 hfsc ls m2 10000Mbit ul >m2 10000Mbit >tc class add dev ifb1 parent 1:1 classid 1:3 hfsc ls m2 5000Mbit ul m2 5000Mbit >tc class add dev ifb1 parent 1:2 classid 1:8000 hfsc ls m2 10000Mbit >ul m2 10000Mbit >tc qdisc add dev ifb1 parent 1:8000 handle 8000: sfq perturb 60 >tc qdisc add dev ifb1 parent 1:3 handle 3: pfifo limit 10000 > > >tc filter add dev ifb1 protocol ip parent 1:0 handle 9: u32 divisor 256 >tc filter add dev ifb1 protocol ip parent 1:0 u32 ht 800:: match ip >dst 192.168.0.0/24 hashkey mask 0x000000ff at 16 link 9: >tc class add dev ifb1 parent 1:2 classid 1:60 hfsc ls m2 8kbit ul m2 51200kbit >echo 1 >tc filter add dev ifb1 parent 1:2 protocol ip u32 ht 9:22 match ip dst >192.168.0.34 flowid 1:60 >echo 2 >tc qdisc add dev ifb1 parent 1:60 handle 60: pfifo limit 8192 > > >Is working with 4.13 > > >But it is not working with 4.15 > >error is when adding: > >tc filter add dev ifb1 protocol ip parent 1:2 prio 4 u32 ht 9:0x22 >match ip dst 192.168.0.34 flowid 1:60 >RTNETLINK answers: Invalid argument >We have an error talking to the kernel > > > > >Thanks > >Paweł Staszewski ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Fwd: u32 ht filters 2018-02-07 7:01 ` Jiri Pirko @ 2018-02-07 23:08 ` Cong Wang 2018-02-08 7:38 ` Jiri Pirko 0 siblings, 1 reply; 8+ messages in thread From: Cong Wang @ 2018-02-07 23:08 UTC (permalink / raw) To: Jiri Pirko Cc: Jiri Pirko, Linux Kernel Network Developers, Paweł Staszewski On Tue, Feb 6, 2018 at 11:01 PM, Jiri Pirko <jiri@resnulli.us> wrote: > Wed, Feb 07, 2018 at 06:09:15AM CET, xiyou.wangcong@gmail.com wrote: >>Hi, Jiri >> >>Your commit 7fa9d974f3c2a016b9accb18f4ee2ed2a738585c >>breaks the tc script by Paweł. Please find below for details. > > Did you do the bisection? > The commit just uses block struct instead of q, but since they > are in 1:1 relation, that should be equvivalent. So basically you still > have per-qdisc hashtables for u32. Well, at least the following fixes the problem here. But I am not sure if it is expected too for shared block among multiple qdiscs. @@ -338,7 +330,7 @@ static struct hlist_head *tc_u_common_hash; static unsigned int tc_u_hash(const struct tcf_proto *tp) { - return hash_ptr(tp->chain->block, U32_HASH_SHIFT); + return hash_ptr(tp->chain->block->q, U32_HASH_SHIFT); } static struct tc_u_common *tc_u_common_find(const struct tcf_proto *tp) @@ -348,7 +340,7 @@ static struct tc_u_common *tc_u_common_find(const struct tcf_proto *tp) h = tc_u_hash(tp); hlist_for_each_entry(tc, &tc_u_common_hash[h], hnode) { - if (tc->block == tp->chain->block) + if (tc->block->q == tp->chain->block->q) return tc; } return NULL; ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Fwd: u32 ht filters 2018-02-07 23:08 ` Cong Wang @ 2018-02-08 7:38 ` Jiri Pirko 2018-02-10 20:41 ` Cong Wang 0 siblings, 1 reply; 8+ messages in thread From: Jiri Pirko @ 2018-02-08 7:38 UTC (permalink / raw) To: Cong Wang Cc: Jiri Pirko, Linux Kernel Network Developers, Paweł Staszewski Thu, Feb 08, 2018 at 12:08:36AM CET, xiyou.wangcong@gmail.com wrote: >On Tue, Feb 6, 2018 at 11:01 PM, Jiri Pirko <jiri@resnulli.us> wrote: >> Wed, Feb 07, 2018 at 06:09:15AM CET, xiyou.wangcong@gmail.com wrote: >>>Hi, Jiri >>> >>>Your commit 7fa9d974f3c2a016b9accb18f4ee2ed2a738585c >>>breaks the tc script by Paweł. Please find below for details. >> >> Did you do the bisection? >> The commit just uses block struct instead of q, but since they >> are in 1:1 relation, that should be equvivalent. So basically you still >> have per-qdisc hashtables for u32. > >Well, at least the following fixes the problem here. But I am not sure >if it is expected too for shared block among multiple qdiscs. For shared block, block->q is null. > > >@@ -338,7 +330,7 @@ static struct hlist_head *tc_u_common_hash; > > static unsigned int tc_u_hash(const struct tcf_proto *tp) > { >- return hash_ptr(tp->chain->block, U32_HASH_SHIFT); >+ return hash_ptr(tp->chain->block->q, U32_HASH_SHIFT); > } > > static struct tc_u_common *tc_u_common_find(const struct tcf_proto *tp) >@@ -348,7 +340,7 @@ static struct tc_u_common *tc_u_common_find(const >struct tcf_proto *tp) > > h = tc_u_hash(tp); > hlist_for_each_entry(tc, &tc_u_common_hash[h], hnode) { >- if (tc->block == tp->chain->block) >+ if (tc->block->q == tp->chain->block->q) :O I don't get it. tc->block is pointer, tc->block->q is pointer. And they are different at the same time for non-shared block. > return tc; > } > return NULL; ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Fwd: u32 ht filters 2018-02-08 7:38 ` Jiri Pirko @ 2018-02-10 20:41 ` Cong Wang 2018-02-12 13:23 ` Jiri Pirko 2018-02-12 15:32 ` Jiri Pirko 0 siblings, 2 replies; 8+ messages in thread From: Cong Wang @ 2018-02-10 20:41 UTC (permalink / raw) To: Jiri Pirko Cc: Jiri Pirko, Linux Kernel Network Developers, Paweł Staszewski On Wed, Feb 7, 2018 at 11:38 PM, Jiri Pirko <jiri@resnulli.us> wrote: > Thu, Feb 08, 2018 at 12:08:36AM CET, xiyou.wangcong@gmail.com wrote: >>On Tue, Feb 6, 2018 at 11:01 PM, Jiri Pirko <jiri@resnulli.us> wrote: >>> Wed, Feb 07, 2018 at 06:09:15AM CET, xiyou.wangcong@gmail.com wrote: >>>>Hi, Jiri >>>> >>>>Your commit 7fa9d974f3c2a016b9accb18f4ee2ed2a738585c >>>>breaks the tc script by Paweł. Please find below for details. >>> >>> Did you do the bisection? >>> The commit just uses block struct instead of q, but since they >>> are in 1:1 relation, that should be equvivalent. So basically you still >>> have per-qdisc hashtables for u32. >> >>Well, at least the following fixes the problem here. But I am not sure >>if it is expected too for shared block among multiple qdiscs. > > For shared block, block->q is null. According to this comment: /* block_index not 0 means the shared block is requested */ and the code, if (!block) { block = tcf_block_create(net, q, extack); block->q is set to q, and q is always non-NULL AFAIU. Also, I don't know if it is intended, but block->q always points to the parent qdisc rather than the qdisc attached to a class. >> >> >>@@ -338,7 +330,7 @@ static struct hlist_head *tc_u_common_hash; >> >> static unsigned int tc_u_hash(const struct tcf_proto *tp) >> { >>- return hash_ptr(tp->chain->block, U32_HASH_SHIFT); >>+ return hash_ptr(tp->chain->block->q, U32_HASH_SHIFT); >> } >> >> static struct tc_u_common *tc_u_common_find(const struct tcf_proto *tp) >>@@ -348,7 +340,7 @@ static struct tc_u_common *tc_u_common_find(const >>struct tcf_proto *tp) >> >> h = tc_u_hash(tp); >> hlist_for_each_entry(tc, &tc_u_common_hash[h], hnode) { >>- if (tc->block == tp->chain->block) >>+ if (tc->block->q == tp->chain->block->q) > > :O I don't get it. tc->block is pointer, tc->block->q is pointer. And > they are different at the same time for non-shared block. If you look into Pawel's script, a new block is created for each class therefore a different tc_u_common is created which causes the ht 9:22 can't be found. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Fwd: u32 ht filters 2018-02-10 20:41 ` Cong Wang @ 2018-02-12 13:23 ` Jiri Pirko 2018-02-12 15:32 ` Jiri Pirko 1 sibling, 0 replies; 8+ messages in thread From: Jiri Pirko @ 2018-02-12 13:23 UTC (permalink / raw) To: Cong Wang Cc: Jiri Pirko, Linux Kernel Network Developers, Paweł Staszewski Sat, Feb 10, 2018 at 09:41:57PM CET, xiyou.wangcong@gmail.com wrote: >On Wed, Feb 7, 2018 at 11:38 PM, Jiri Pirko <jiri@resnulli.us> wrote: >> Thu, Feb 08, 2018 at 12:08:36AM CET, xiyou.wangcong@gmail.com wrote: >>>On Tue, Feb 6, 2018 at 11:01 PM, Jiri Pirko <jiri@resnulli.us> wrote: >>>> Wed, Feb 07, 2018 at 06:09:15AM CET, xiyou.wangcong@gmail.com wrote: >>>>>Hi, Jiri >>>>> >>>>>Your commit 7fa9d974f3c2a016b9accb18f4ee2ed2a738585c >>>>>breaks the tc script by Paweł. Please find below for details. >>>> >>>> Did you do the bisection? >>>> The commit just uses block struct instead of q, but since they >>>> are in 1:1 relation, that should be equvivalent. So basically you still >>>> have per-qdisc hashtables for u32. >>> >>>Well, at least the following fixes the problem here. But I am not sure >>>if it is expected too for shared block among multiple qdiscs. >> >> For shared block, block->q is null. > >According to this comment: >/* block_index not 0 means the shared block is requested */ > >and the code, > > if (!block) { > block = tcf_block_create(net, q, extack); > >block->q is set to q, and q is always non-NULL AFAIU. Yep, that is a bug. Fixing it now. > >Also, I don't know if it is intended, but block->q always points to >the parent qdisc rather than the qdisc attached to a class. You are right. That is incorrect. Fixing now. > > >>> >>> >>>@@ -338,7 +330,7 @@ static struct hlist_head *tc_u_common_hash; >>> >>> static unsigned int tc_u_hash(const struct tcf_proto *tp) >>> { >>>- return hash_ptr(tp->chain->block, U32_HASH_SHIFT); >>>+ return hash_ptr(tp->chain->block->q, U32_HASH_SHIFT); >>> } >>> >>> static struct tc_u_common *tc_u_common_find(const struct tcf_proto *tp) >>>@@ -348,7 +340,7 @@ static struct tc_u_common *tc_u_common_find(const >>>struct tcf_proto *tp) >>> >>> h = tc_u_hash(tp); >>> hlist_for_each_entry(tc, &tc_u_common_hash[h], hnode) { >>>- if (tc->block == tp->chain->block) >>>+ if (tc->block->q == tp->chain->block->q) >> >> :O I don't get it. tc->block is pointer, tc->block->q is pointer. And >> they are different at the same time for non-shared block. > >If you look into Pawel's script, a new block is created for each class >therefore a different tc_u_common is created which causes the >ht 9:22 can't be found. Yeah :/ this tc_u_common thing is going to haunt me. This is resolvable now by using block->q. There is no support for block sharing for other qdiscs than ingress and clsact so we don't have to take care of the multi-class&block sharing now. Will fix. Thanks! ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Fwd: u32 ht filters 2018-02-10 20:41 ` Cong Wang 2018-02-12 13:23 ` Jiri Pirko @ 2018-02-12 15:32 ` Jiri Pirko 2018-02-12 15:51 ` Jiri Pirko 1 sibling, 1 reply; 8+ messages in thread From: Jiri Pirko @ 2018-02-12 15:32 UTC (permalink / raw) To: Cong Wang Cc: Jiri Pirko, Linux Kernel Network Developers, Paweł Staszewski Sat, Feb 10, 2018 at 09:41:57PM CET, xiyou.wangcong@gmail.com wrote: [...] >>>@@ -348,7 +340,7 @@ static struct tc_u_common *tc_u_common_find(const >>>struct tcf_proto *tp) >>> >>> h = tc_u_hash(tp); >>> hlist_for_each_entry(tc, &tc_u_common_hash[h], hnode) { >>>- if (tc->block == tp->chain->block) >>>+ if (tc->block->q == tp->chain->block->q) >> >> :O I don't get it. tc->block is pointer, tc->block->q is pointer. And >> they are different at the same time for non-shared block. > >If you look into Pawel's script, a new block is created for each class >therefore a different tc_u_common is created which causes the >ht 9:22 can't be found. But wait. Originally this code looked like this: static unsigned int tc_u_hash(const struct tcf_proto *tp) { struct net_device *dev = tp->q->dev_queue->dev; u32 qhandle = tp->q->handle; int ifindex = dev->ifindex; return hash_64((u64)ifindex << 32 | qhandle, U32_HASH_SHIFT); } static struct tc_u_common *tc_u_common_find(const struct tcf_proto *tp) { struct tc_u_common *tc; unsigned int h; h = tc_u_hash(tp); hlist_for_each_entry(tc, &tc_u_common_hash[h], hnode) { if (tc->q == tp->q) return tc; } return NULL; } That means that tc_u_common is identified according to tp->q. And that is different for every class. How that could work? I'm properly confused. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Fwd: u32 ht filters 2018-02-12 15:32 ` Jiri Pirko @ 2018-02-12 15:51 ` Jiri Pirko 0 siblings, 0 replies; 8+ messages in thread From: Jiri Pirko @ 2018-02-12 15:51 UTC (permalink / raw) To: Cong Wang Cc: Jiri Pirko, Linux Kernel Network Developers, Paweł Staszewski Mon, Feb 12, 2018 at 04:32:16PM CET, jiri@resnulli.us wrote: >Sat, Feb 10, 2018 at 09:41:57PM CET, xiyou.wangcong@gmail.com wrote: > >[...] > >>>>@@ -348,7 +340,7 @@ static struct tc_u_common *tc_u_common_find(const >>>>struct tcf_proto *tp) >>>> >>>> h = tc_u_hash(tp); >>>> hlist_for_each_entry(tc, &tc_u_common_hash[h], hnode) { >>>>- if (tc->block == tp->chain->block) >>>>+ if (tc->block->q == tp->chain->block->q) >>> >>> :O I don't get it. tc->block is pointer, tc->block->q is pointer. And >>> they are different at the same time for non-shared block. >> >>If you look into Pawel's script, a new block is created for each class >>therefore a different tc_u_common is created which causes the >>ht 9:22 can't be found. > >But wait. Originally this code looked like this: >static unsigned int tc_u_hash(const struct tcf_proto *tp) >{ > struct net_device *dev = tp->q->dev_queue->dev; > u32 qhandle = tp->q->handle; > int ifindex = dev->ifindex; > > return hash_64((u64)ifindex << 32 | qhandle, U32_HASH_SHIFT); >} > >static struct tc_u_common *tc_u_common_find(const struct tcf_proto *tp) >{ > struct tc_u_common *tc; > unsigned int h; > > h = tc_u_hash(tp); > hlist_for_each_entry(tc, &tc_u_common_hash[h], hnode) { > if (tc->q == tp->q) > return tc; > } > return NULL; >} > >That means that tc_u_common is identified according to tp->q. And that >is different for every class. How that could work? I'm properly confused. Okay, now I see what is wrong. I wrongly assumed 1:1 block:q. Will fix. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-02-12 15:51 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <9c8f997d-339c-5088-0bb5-124e9f55f02d@itcare.pl> 2018-02-07 5:09 ` Fwd: u32 ht filters Cong Wang 2018-02-07 7:01 ` Jiri Pirko 2018-02-07 23:08 ` Cong Wang 2018-02-08 7:38 ` Jiri Pirko 2018-02-10 20:41 ` Cong Wang 2018-02-12 13:23 ` Jiri Pirko 2018-02-12 15:32 ` Jiri Pirko 2018-02-12 15:51 ` Jiri Pirko
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.