Re: Fwd: u32 ht filters

[Jiri Pirko]

Mon, Feb 12, 2018 at 04:32:16PM CET, j...@resnulli.us wrote:
>Sat, Feb 10, 2018 at 09:41:57PM CET, xiyou.wangc...@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, _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, _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.

Re: Fwd: u32 ht filters

[Jiri Pirko]

Sat, Feb 10, 2018 at 09:41:57PM CET, xiyou.wangc...@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, _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, _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.

Re: Fwd: u32 ht filters

[Jiri Pirko]

Sat, Feb 10, 2018 at 09:41:57PM CET, xiyou.wangc...@gmail.com wrote:
>On Wed, Feb 7, 2018 at 11:38 PM, Jiri Pirko  wrote:
>> Thu, Feb 08, 2018 at 12:08:36AM CET, xiyou.wangc...@gmail.com wrote:
>>>On Tue, Feb 6, 2018 at 11:01 PM, Jiri Pirko  wrote:
 Wed, Feb 07, 2018 at 06:09:15AM CET, xiyou.wangc...@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, _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 sharing now. Will fix.

Thanks!

Re: Fwd: u32 ht filters

[Cong Wang]

On Wed, Feb 7, 2018 at 11:38 PM, Jiri Pirko  wrote:
> Thu, Feb 08, 2018 at 12:08:36AM CET, xiyou.wangc...@gmail.com wrote:
>>On Tue, Feb 6, 2018 at 11:01 PM, Jiri Pirko  wrote:
>>> Wed, Feb 07, 2018 at 06:09:15AM CET, xiyou.wangc...@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, _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.

Re: Fwd: u32 ht filters

[Jiri Pirko]

Thu, Feb 08, 2018 at 12:08:36AM CET, xiyou.wangc...@gmail.com wrote:
>On Tue, Feb 6, 2018 at 11:01 PM, Jiri Pirko  wrote:
>> Wed, Feb 07, 2018 at 06:09:15AM CET, xiyou.wangc...@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, _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;

Re: Fwd: u32 ht filters

[Cong Wang]

On Tue, Feb 6, 2018 at 11:01 PM, Jiri Pirko  wrote:
> Wed, Feb 07, 2018 at 06:09:15AM CET, xiyou.wangc...@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, _u_common_hash[h], hnode) {
-   if (tc->block == tp->chain->block)
+   if (tc->block->q == tp->chain->block->q)
return tc;
}
return NULL;

Re: Fwd: u32 ht filters

[Jiri Pirko]

Wed, Feb 07, 2018 at 06:09:15AM CET, xiyou.wangc...@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 
>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 
>Signed-off-by: David S. Miller 
>
>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 
>Date: Tue, Feb 6, 2018 at 8:05 AM
>Subject: u32 ht filters
>To: Cong Wang 
>
>
>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 1Mbit ul
>m2 1Mbit
>tc class add dev ifb1 parent 1:1 classid 1:2 hfsc ls m2 1Mbit ul
>m2 1Mbit
>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 1Mbit
>ul m2 1Mbit
>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 1
>
>
>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 0x00ff 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

Fwd: u32 ht filters

[Cong Wang]

Hi, Jiri

Your  commit 7fa9d974f3c2a016b9accb18f4ee2ed2a738585c
breaks the tc script by Paweł. Please find below for details.


commit 7fa9d974f3c2a016b9accb18f4ee2ed2a738585c
Author: Jiri Pirko 
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 
Signed-off-by: David S. Miller 

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 
Date: Tue, Feb 6, 2018 at 8:05 AM
Subject: u32 ht filters
To: Cong Wang 


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 1Mbit ul
m2 1Mbit
tc class add dev ifb1 parent 1:1 classid 1:2 hfsc ls m2 1Mbit ul
m2 1Mbit
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 1Mbit
ul m2 1Mbit
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 1


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