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

[Marcelo Ricardo Leitner]

On Mon, Jan 08, 2018 at 04:42:03PM +0100, Jiri Pirko wrote:
> Mon, Jan 08, 2018 at 04:23:06PM CET, marcelo.leit...@gmail.com wrote:
> >On Sat, Jan 06, 2018 at 12:09:18AM +0100, Jiri Pirko wrote:
> >...
> >> 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
> >> ...
> >
> >If changing a rule on an interface and reflecting it on the other
> >is considered confusing, what about getting the stats including the
> >stats from the other interface? AFAICT that's what would happen in the
> >3 show commands above, they would show the same values.
> 
> Yes. Same block, same values.
> 
> 
> >
> >Seems it can get confusing to the user: to check an interface, see
> >some hits on it, but they actually happened on the other interface.
> 
> Okay, what do you suggest?

Only one idea so far: highlight somehow that 'block X' is shared in
there.

> Note that "filter show" uses dumpit.

Yes.

> Also note that each filter listed under qdisc ens7 ingress and
> dev ens8 ingress is very clearly marked with "block 22".

Yes but one listing the rules on ens7 doesn't know that "block 22"
is shared with other interface(s).

> 
> Why is it confusing?

Pretty much the same reasoning as changing rules on it IMO, but:
- they may show hits that didn't happen on this interface
- they won't ever amount to the stats reported by the interface
  itself, even if the user has some catch-all rule.

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

[Jiri Pirko]

Mon, Jan 08, 2018 at 04:23:06PM CET, marcelo.leit...@gmail.com wrote:
>On Sat, Jan 06, 2018 at 12:09:18AM +0100, Jiri Pirko wrote:
>...
>> 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
>> ...
>
>If changing a rule on an interface and reflecting it on the other
>is considered confusing, what about getting the stats including the
>stats from the other interface? AFAICT that's what would happen in the
>3 show commands above, they would show the same values.

Yes. Same block, same values.


>
>Seems it can get confusing to the user: to check an interface, see
>some hits on it, but they actually happened on the other interface.

Okay, what do you suggest?
Note that "filter show" uses dumpit.
Also note that each filter listed under qdisc ens7 ingress and
dev ens8 ingress is very clearly marked with "block 22".

Why is it confusing?

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

[Marcelo Ricardo Leitner]

On Sat, Jan 06, 2018 at 12:09:18AM +0100, Jiri Pirko wrote:
...
> 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
> ...

If changing a rule on an interface and reflecting it on the other
is considered confusing, what about getting the stats including the
stats from the other interface? AFAICT that's what would happen in the
3 show commands above, they would show the same values.

Seems it can get confusing to the user: to check an interface, see
some hits on it, but they actually happened on the other interface.

  Marcelo

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

[Jiri Pirko]

Sat, Jan 06, 2018 at 07:16:10PM CET, j...@mojatatu.com wrote:
>On 18-01-06 12:41 PM, David Ahern wrote:
>> On 1/6/18 1:07 AM, Jiri Pirko wrote:
>> > Sat, Jan 06, 2018 at 04:57:21AM CET, dsah...@gmail.com wrote:
>> > > On 1/5/18 4:09 PM, Jiri Pirko wrote:
>> > > > From: Jiri Pirko 
>> > > > 
>
>> > > 
>> > > $ tc filter show block 22
>> > > $ echo $?
>> > > 0
>> > > $  tc qdisc show | grep block
>> > > qdisc ingress : dev eth2 parent :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
>
>
>BTW, above syntax looks redundant because of the ingress parent
>specification. It is possible iproute2/tc is just ignoring it right now

I'm sure that is just a typo on DaveA's side.


>and the parent is not being used.
>i.e Once the block is bound to ingress (aka parent :) via:
>qdisc ingress : dev eth2 parent :fff1 block 42
>then
>it doesnt make sense to specify a parent again because
>it should be possible to bind that block to many parent locations
>eg clsact ingress of dev x and clsact egress of dev y.
>
>what am i missing?
>
>cheers,
>jamal

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

[Jiri Pirko]

Sat, Jan 06, 2018 at 06:41:18PM CET, dsah...@gmail.com wrote:
>On 1/6/18 1:07 AM, Jiri Pirko wrote:
>> Sat, Jan 06, 2018 at 04:57:21AM CET, dsah...@gmail.com wrote:
>>> On 1/5/18 4:09 PM, Jiri Pirko wrote:
 From: Jiri Pirko 

 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 : dev ens7 parent :fff1 block 22
 qdisc ingress : dev ens8 parent :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 : parent :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."

Ok. Sounds reasonable.


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

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

[David Ahern]

On 1/6/18 11:02 AM, Jamal Hadi Salim wrote:
> BTW: From your output, DavidA, i noticed something strange:
> two flower filters with the same handle id 0x1 (different prios)
> and also two filters with the same prio (but different handles).
> I see one was added using :dev .." - how were the other 2 added?
> Consequence of patch, maybe?

Not clear from the command history. I was running commands (add by
device and add by block) in various orders to see what happens.


> 
> Note:
> Expected behavior is two filters of same kind on the same chain
> should be distinguished by priority. There are filters like u32
> (which hide hash tables under the same priority) which may
> allow the same prio for multiple handles - just dont see that
> fit with flower, but maybe missing something.
> 
> cheers,
> jamal

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

[Jamal Hadi Salim]

On 18-01-06 01:02 PM, Jamal Hadi Salim wrote:

On 18-01-06 04:48 AM, Jiri Pirko wrote:




BTW: From your output, DavidA, i noticed something strange:
two flower filters with the same handle id 0x1 (different prios)


At least on the kernel i am using this is the exhibited default
behavior. I can also create filters with different priorities
and with different handle ids.


and also two filters with the same prio (but different handles).


This does not seem possible (as i was expecting) - so could be
the result of the block code...

cheers,
jamal

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

[Jamal Hadi Salim]

On 18-01-06 12:41 PM, David Ahern wrote:

On 1/6/18 1:07 AM, Jiri Pirko wrote:

Sat, Jan 06, 2018 at 04:57:21AM CET, dsah...@gmail.com wrote:

On 1/5/18 4:09 PM, Jiri Pirko wrote:

From: Jiri Pirko 





$ tc filter show block 22
$ echo $?
0
$  tc qdisc show | grep block
qdisc ingress : dev eth2 parent :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



BTW, above syntax looks redundant because of the ingress parent
specification. It is possible iproute2/tc is just ignoring it right now
and the parent is not being used.
i.e Once the block is bound to ingress (aka parent :) via:
qdisc ingress : dev eth2 parent :fff1 block 42
then
it doesnt make sense to specify a parent again because
it should be possible to bind that block to many parent locations
eg clsact ingress of dev x and clsact egress of dev y.

what am i missing?

cheers,
jamal

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

[Jamal Hadi Salim]

On 18-01-06 04:48 AM, Jiri Pirko wrote:

[..]


Or, do you think it should work like:

$ tc qdisc add dev ens8 ingress
$ tc qdisc
qdisc ingress : dev ens8 parent :fff1


>

$ tc qdisc add dev ens7 ingress block 22

>

$ tc qdisc
qdisc ingress : dev ens7 parent :fff1 block 22
qdisc ingress : dev ens8 parent :fff1

And the only shareable block is the one which I spefify block index for?
And for that, I have to always use block index handle for filter
add/del/get?



This is more explicit and better IMO (assuming syntax for sharing is
as shown earlier - adding via ens7 is going to be rejected).

Another thing to consider:
ens8 to join in the sharing i.e something like:
tc qdisc change/replace dev ens8 ingress block 22
And this to replace the local block created earlier
(with any old filters destroyed in the process).

BTW: From your output, DavidA, i noticed something strange:
two flower filters with the same handle id 0x1 (different prios)
and also two filters with the same prio (but different handles).
I see one was added using :dev .." - how were the other 2 added?
Consequence of patch, maybe?

Note:
Expected behavior is two filters of same kind on the same chain
should be distinguished by priority. There are filters like u32
(which hide hash tables under the same priority) which may
allow the same prio for multiple handles - just dont see that
fit with flower, but maybe missing something.

cheers,
jamal

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

[David Ahern]

On 1/6/18 1:07 AM, Jiri Pirko wrote:
> Sat, Jan 06, 2018 at 04:57:21AM CET, dsah...@gmail.com wrote:
>> On 1/5/18 4:09 PM, Jiri Pirko wrote:
>>> From: Jiri Pirko 
>>>
>>> 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 : dev ens7 parent :fff1 block 22
>>> qdisc ingress : dev ens8 parent :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 : parent :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 

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

[Jiri Pirko]

Sat, Jan 06, 2018 at 09:07:28AM CET, j...@resnulli.us wrote:
>Sat, Jan 06, 2018 at 04:57:21AM CET, dsah...@gmail.com wrote:
>>On 1/5/18 4:09 PM, Jiri Pirko wrote:
>>> From: Jiri Pirko 
>>> 
>>> 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 : dev ens7 parent :fff1 block 22
>>> qdisc ingress : dev ens8 parent :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 : parent :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.

Or, do you think it should work like:

$ tc qdisc add dev ens8 ingress
$ tc qdisc
qdisc ingress : dev ens8 parent :fff1

$ tc qdisc add dev ens7 ingress block 22
$ tc qdisc
qdisc ingress : dev ens7 parent :fff1 block 22
qdisc ingress : dev ens8 parent :fff1

And the only shareable block is the one which I spefify block index for?
And for that, I have to always use block index handle for filter
add/del/get?

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

[Jiri Pirko]

Sat, Jan 06, 2018 at 04:57:21AM CET, dsah...@gmail.com wrote:
>On 1/5/18 4:09 PM, Jiri Pirko wrote:
>> From: Jiri Pirko 
>> 
>> 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 : dev ens7 parent :fff1 block 22
>> qdisc ingress : dev ens8 parent :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 : parent :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.


>
>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 : dev eth2 parent :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 

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

[David Ahern]

On 1/5/18 4:09 PM, Jiri Pirko wrote:
> From: Jiri Pirko 
> 
> 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 : dev ens7 parent :fff1 block 22
> qdisc ingress : dev ens8 parent :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 : parent :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

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

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 : dev eth2 parent :fff1 block 42

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

[Jiri Pirko]

From: Jiri Pirko 

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 : dev ens7 parent :fff1 block 22
qdisc ingress : dev ens8 parent :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
...

---
v5->v6:
- added patch 6 that introduces block handle

v4->v5:
- patch 5:
 - add tracking of binding of devs that are unable to offload and check
   that before block cbs call.

v3->v4:
- patch 1:
 - rebased on top of the current net-next
 - added some extack strings
- patch 3:
 - rebased on top of the current net-next
- patch 5:
 - propagate netdev_ops->ndo_setup_tc error up to tcf_block_offload_bind
   caller
- patch 7:
 - rebased on top of the current net-next

v2->v3:
- removed original patch 1, removing tp->q cls_bpf dependency. Fixed by
  Jakub in the meantime.
- patch 1:
 - rebased on top of the current net-next
- patch 5:
 - new patch
- patch 8:
 - removed "p_" prefix from block index function args
- patch 10:
 - add tc offload feature handling

Jiri Pirko (11):
  net: sched: introduce support for multiple filter chain pointers
registration
  net: sched: avoid usage of tp->q in tcf_classify
  net: sched: introduce block mechanism to handle netif_keep_dst calls
  net: sched: remove classid and q fields from tcf_proto
  net: sched: keep track of offloaded filters and check tc offload
feature
  net: sched: use block index as a handle instead of qdisc when block is
shared
  net: sched: allow ingress and clsact qdiscs to share filter blocks
  mlxsw: spectrum_acl: Reshuffle code around
mlxsw_sp_acl_ruleset_create/destroy
  mlxsw: spectrum_acl: Don't store netdev and ingress for ruleset unbind
  mlxsw: spectrum_acl: Implement TC block sharing
  mlxsw: spectrum_acl: Pass mlxsw_sp_port down to ruleset bind/unbind
ops

 drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 182 ++-
 drivers/net/ethernet/mellanox/mlxsw/spectrum.h |  44 +-
 drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c | 302 ---
 .../ethernet/mellanox/mlxsw/spectrum_acl_tcam.c|  44 +-
 .../net/ethernet/mellanox/mlxsw/spectrum_flower.c  |  41 +-
 include/net/pkt_cls.h  |   9 +
 include/net/sch_generic.h  |  27 +-
 include/uapi/linux/pkt_sched.h |  11 +
 net/sched/cls_api.c| 604 -
 net/sched/cls_bpf.c|   9 +-
 net/sched/cls_flow.c   |   2 +-
 net/sched/cls_flower.c |   3 +-
 net/sched/cls_matchall.c   |   3 +-
 net/sched/cls_route.c  |   2 +-
 net/sched/cls_u32.c|  13 +-
 net/sched/sch_ingress.c|  89 ++-
 16 files changed, 1079 insertions(+), 306 deletions(-)

-- 
2.9.5