Re: [patch net-next v6 00/11] net: sched: allow qdiscs to share filter block instances

[David Ahern <dsahern@gmail.com>]

On 1/6/18 1:07 AM, Jiri Pirko wrote:
> Sat, Jan 06, 2018 at 04:57:21AM CET, dsahern@gmail.com wrote:
>> On 1/5/18 4:09 PM, Jiri Pirko wrote:
>>> From: Jiri Pirko <jiri@mellanox.com>
>>>
>>> Currently the filters added to qdiscs are independent. So for example if you
>>> have 2 netdevices and you create ingress qdisc on both and you want to add
>>> identical filter rules both, you need to add them twice. This patchset
>>> makes this easier and mainly saves resources allowing to share all filters
>>> within a qdisc - I call it a "filter block". Also this helps to save
>>> resources when we do offload to hw for example to expensive TCAM.
>>>
>>> So back to the example. First, we create 2 qdiscs. Both will share
>>> block number 22. "22" is just an identification. If we don't pass any
>>> block number, a new one will be generated by kernel:
>>>
>>> $ tc qdisc add dev ens7 ingress block 22
>>>                                 ^^^^^^^^
>>> $ tc qdisc add dev ens8 ingress block 22
>>>                                 ^^^^^^^^
>>>
>>> Now if we list the qdiscs, we will see the block index in the output:
>>>
>>> $ tc qdisc
>>> qdisc ingress ffff: dev ens7 parent ffff:fff1 block 22
>>> qdisc ingress ffff: dev ens8 parent ffff:fff1 block 22
>>>
>>>
>>> To make is more visual, the situation looks like this:
>>>
>>>    ens7 ingress qdisc                 ens7 ingress qdisc
>>>           |                                  |
>>>           |                                  |
>>>           +---------->  block 22  <----------+
>>>
>>> Unlimited number of qdiscs may share the same block.
>>>
>>> Now we can add filter using the block index:
>>>
>>> $ tc filter add block 22 protocol ip pref 25 flower dst_ip 192.168.0.0/16 action drop
>>>
>>>
>>> Note we cannot use the qdisc for filter manipulations for shared blocks:
>>>
>>> $ tc filter add dev ens8 ingress protocol ip pref 1 flower dst_ip 192.168.100.2 action drop
>>> Error: Cannot work with shared block, please use block index.
>>>
>>>
>>> We will see the same output if we list filters for ingress qdisc of
>>> ens7 and ens8, also for the block 22:
>>>
>>> $ tc filter show block 22
>>> filter block 22 protocol ip pref 25 flower chain 0
>>> filter block 22 protocol ip pref 25 flower chain 0 handle 0x1
>>> ...
>>>
>>> $ tc filter show dev ens7 ingress
>>> filter block 22 protocol ip pref 25 flower chain 0
>>> filter block 22 protocol ip pref 25 flower chain 0 handle 0x1
>>> ...
>>>
>>> $ tc filter show dev ens8 ingress
>>> filter block 22 protocol ip pref 25 flower chain 0
>>> filter block 22 protocol ip pref 25 flower chain 0 handle 0x1
>>> ...
>>
>> I like the API and output shown here, but I am not getting that with the
>> patches.
>>
>> In this example, I am using 42 for the block id:
>>
>> $ tc qdisc show dev eth2
>> qdisc mq 0: root
>> qdisc pfifo_fast 0: parent :2 bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1
>> 1 1 1
>> qdisc pfifo_fast 0: parent :1 bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1
>> 1 1 1
>> qdisc ingress ffff: parent ffff:fff1 block 42
>>
>> It allows me to add a filter using the device:
>> $ tc filter add dev eth2 ingress protocol ip pref 1 flower dst_ip
>> 192.168.101.2 action drop
>> $  echo $?
>> 0
> 
> Yes, because the block is not shared yet. You have it only for one
> qdisc. As long as you have that, the "filter add dev" api still works.
> It stops working when you add another qdisc to that block.

Interesting.

Once I add the block to another qdisc I do get an error:

$ tc filter add dev eth2 ingress protocol ip pref 1 flower dst_ip
192.168.100.2 action drop
Error: Cannot work with shared block, please use block index.

Can you change that to something like: "This filter block is shared.
Please use the block index to make changes."


> 
> 
>>
>> And it modifies the shared block:
>> $  tc filter show block 42
>> filter pref 1 flower chain 0
>> filter pref 1 flower chain 0 handle 0x1
>>  eth_type ipv4
>>  dst_ip 192.168.100.2
>>  not_in_hw
>> 	action order 1: gact action drop
>> 	 random type none pass val 0
>> 	 index 2 ref 1 bind 1
>>
>> filter pref 1 flower chain 0 handle 0x2
>>  eth_type ipv4
>>  dst_ip 192.168.101.2
>>  not_in_hw
>> 	action order 1: gact action drop
>> 	 random type none pass val 0
>> 	 index 3 ref 1 bind 1
>>
>> filter pref 25 flower chain 0
>> filter pref 25 flower chain 0 handle 0x1
>>  eth_type ipv4
>>  dst_ip 192.168.0.0/16
>>  not_in_hw
>> 	action order 1: gact action drop
>> 	 random type none pass val 0
>> 	 index 1 ref 1 bind 1
>>
>> Notice the header does not give the 'filter block N protocol' part. I
>> don't get that using the device either (tc filter show dev eth2 ingress).
> 
> That is correct. Check the print_filter function in tc/tc_filter.c. It
> works with "filter_ifindex" and with my patch with "filter_block_index".
> That means that if the value for the filter dumped actually differs from
> what you passed on the command line, it prints it.
> 
> Once you actually share the block with another qdisc, you will see
> "block N"
> 
> 
>>
>> Something else I noticed is that I do not get an error message if I pass
>> an invalid block id:
>>
>> $ tc filter show block 22
>> $ echo $?
>> 0
>> $  tc qdisc show | grep block
>> qdisc ingress ffff: dev eth2 parent ffff:fff1 block 42
> 
> Yeah, I will try to fix this. The thing is, this is not error by kernel
> but by the userspace. Kernel is perfectly ok with invalid device or
> block index, it just does not dump anything and I would leave it like
> that. I have to somehow check the validity of block_index in userspace.
> Not sure how now.

Ok. I saw a response about idr_alloc_ext.

Here's another one: adding a filter to an unknown block id:

$ tc filter add block 66 ingress protocol ip pref 1 flower dst_ip
192.168.100.2 action drop
RTNETLINK answers: Invalid argument
We have an error talking to the kernel


Can you add a proper extack message for that case?

Thanks,