Fwd: u32 ht filters

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

Re: Fwd: u32 ht filters

[Jiri Pirko]

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

Re: Fwd: u32 ht filters

[Cong Wang]

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;

Re: Fwd: u32 ht filters

[Jiri Pirko]

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;

Re: Fwd: u32 ht filters

[Cong Wang]

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.

Re: Fwd: u32 ht filters

[Jiri Pirko]

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!

Re: Fwd: u32 ht filters

[Jiri Pirko]

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.

Re: Fwd: u32 ht filters

[Jiri Pirko]

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.