Date:   Wed, 3 Jan 2018 08:57:23 -0700
From:   David Ahern <dsahern@...il.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, jhs@...atatu.com,
        xiyou.wangcong@...il.com, mlxsw@...lanox.com, andrew@...n.ch,
        vivien.didelot@...oirfairelinux.com, f.fainelli@...il.com,
        michael.chan@...adcom.com, ganeshgr@...lsio.com,
        saeedm@...lanox.com, matanb@...lanox.com, leonro@...lanox.com,
        idosch@...lanox.com, jakub.kicinski@...ronome.com,
        simon.horman@...ronome.com, pieter.jansenvanvuuren@...ronome.com,
        john.hurley@...ronome.com, alexander.h.duyck@...el.com,
        ogerlitz@...lanox.com, john.fastabend@...il.com,
        daniel@...earbox.net
Subject: Re: [patch net-next v4 00/10] net: sched: allow qdiscs to share
 filter block instances

On 1/3/18 2:40 AM, Jiri Pirko wrote:
> Wed, Jan 03, 2018 at 03:07:36AM CET, dsahern@...il.com wrote:
>> On 1/2/18 12:49 PM, Jiri Pirko wrote:
>>> DaveA, please consider following example:
>>>
>>> $ tc qdisc add dev ens7 ingress
>>> $ tc qdisc
>>> qdisc ingress ffff: dev ens7 parent ffff:fff1 block 1
>>>
>>> Now I have one device with one qdisc attached.
>>>
>>> I will add some filters, for example:
>>> $ tc filter add dev ens7 ingress protocol ip pref 25 flower dst_ip 192.168.0.0/16 action drop
>>>
>>> No sharing is happening. The user is doing what he is used to do.
>>>
>>> Now user decides to share this filters with another device. As you can
>>> see above, the block created for ens7 qdisc instance has id "1".
>>> User can simply do:
>>>
>>> tc qdisc add dev ens8 ingress block 1
>>>
>>> And the block gets shared among ens7 ingress qdisc instance and ens8
>>> ingress qdisc instance.
>>>
>>> What is wrong with this? The approach you suggest would disallow this
>>
>> Conceptually, absolutely nothing. We all agree that a shared block
>> feature is needed. So no argument on sharing the filters across devices.
>>
>> The disagreement is in how they should be managed. I think my last
>> response concisely captures my concerns -- the principle of least surprise.
>>
>> So with the initial commands above, all is fine. Then someone is
>> debugging a problem or wants to add another filter to ens8, so they run:
>>
>> $ tc filter add dev ens8 ingress protocol ip pref 25 flower dst_ip
>> 192.168.1.0/16 action drop
>>
>> Then traffic flows through ens7 break and some other user is struggling
>> to understand what just happened. That the new filter magically appears
>> on ens7 when the user operated on ens8 is a surprise. Nothing about that
>> last command acknowledges that it is changing a shared resource.
> 
> Given the fact that user configured sharing between ens7 and ens8 and he
> can easily see that by "$ tc qdisc show" I don't see anything wrong
> about it, no surprise. Either the user knows what is he doing or not.

tc is one of the most difficult commands for users to understand and get
right. The API behind the command even more so. There seems to be a
general agreement on this.

To someone like you who is well versed in tc semantics this may seem
obvious, but I contend that even you would slip up here at some point.
There is too much distance between the filter management and the qdisc
listing a part of which shows a block id - not that it is shared or
anything else, just of the many words in the output there is 'block N'.

>>
>> Consider the commands being run by different people, and a time span
>> between. Allowing the shared block to be configured by any device using
>> the block is just setting up users for errors and confusion.
> 
> No confusion. Everything is visible, all info is in the manpage. The
> same story as always.
> 
> 
>>
>>> forcing user to explicitly create some block entity and then to attach
>>> it to qdisc instances. I don't really see good reason for it. Could you
>>> please clear this up for me?
>>
>> It forces the user to acknowledge it is changing a resource that may be
>> shared by more than one device.
>>
>> $ tc filter add dev ens8 ingress protocol ip pref 25 flower dst_ip
>> 192.168.1.0/16 action drop
>> Error: This qdisc is a shared block. Use the block API to configure.
>>
>> $ tc qdisc show dev ens8
>> qdisc ingress ffff: dev ens7 parent ffff:fff1 block 1
>>
>> $ tc filter add block 1 protocol ip pref 25 flower dst_ip 192.168.1.0/16
>> action drop
>>
>> Now there are no surprises. I have to know that ens8 is using block 1,
>> and I have to specify that block when adding a filter.
> 
> On contrary. This is surprising! Consider my original example extended
> by your approach and limitations:

Nope, I was not extending your approach; I was using your examples to
show why I disagree with the approach. As I mentioned in past responses,
I believe the block lifecycle should be independent of any device.

$ tc qdisc add block 1
$ tc filter add block 1 ....

$ tc qdisc add dev ens7 ingress block 1
$ tc qdisc add dev ens8 ingress block 1

$ tc filter show block 1
(filters listed)

$ tc filter show dev ens7 ingress
Info: This qdisc is shared. Use the block api to list filters.

(This is very similar to how I am handling nexthop objects for routes. I
can show some examples if desired, but don't want this tangent to go too
far.)

> 
> $ tc qdisc add dev ens7 ingress
> $ tc qdisc
> qdisc ingress ffff: dev ens7 parent ffff:fff1 block 1
> $ tc filter add dev ens7 ingress protocol ip pref 25 flower dst_ip 192.168.0.0/16 action drop
> 
> So far, everything is good. Now I add qdisc with block 1 to ens8:
> $ tc qdisc add dev ens8 ingress block 1
> 
> And I do:
> $ tc filter add dev ens7 ingress protocol ip pref 25 flower dst_ip 192.168.0.0/16 action drop
> Should it Error out or pass by your limitations?
> 
> Assume it should pass.
> I do:
> $ tc filter add dev ens8 ingress protocol ip pref 25 flower dst_ip 192.168.1.0/16 action drop
> Error: This qdisc is a shared block. Use the block API to configure.
> 
> This will error out as you wrote. Now I do show:
> 
> $ tc qdisc show dev ens8                                                
> qdisc ingress ffff: dev ens7 parent ffff:fff1 block 1
> 
> As you wrote, there is "ens7" in output of ens8 qdisc. That is
> surprising.

that's a typo on my part with copy-paste-modify of commands while
writing an email; that was not intentional to show ens7 on an ens8 device.


> 
> What would following commands show with your limitations:
> $ tc filter show dev ens7 ingress
> $ tc filter show dev ens8 ingress

see above

> 
> All filters should be listed under ens7 and ens8 should be blank? I
> cannot add filters to ens8 with your limitations so I guess the show for
> it should be blank. But there are actually rules there! That is another
> surprise and breakage!
> 
> Now I continue and remove the qdisc from ens7:
> $ tc qdisc add dev ens7 ingress
> 
> The block 1 is still there for ens8. So what happens now? What is the
> output of "filter show dev ens8 ingress" and "qdisc show dev ens8"?
> Will "add dev ens8 ingress" magically start to work now? This is another
> set of surprises and breakages.
> 
> So as I see it with your limitations, there is a lot of surprises
> introduced.
> 
> Note that I gave a lot of thoughts to all this. The approach I suggest
> is the cleanest and does not break anything. Also, it is easily
> extendable by adding the block handle to add/del/list the filters.
> But the current commands should not be broken. Please.
> 
> If you want, I can implement the block handle extension as a part of this
> patchset. I wanted to do it as a follow-up to limit the number of
> patches in the set so DaveM would not have reason to hate me :)
> 
> 
>>
>>
>> BTW, is there an option to list all devices using the same shared block
>> - short of listing all and grepping?
> 
> $ tc qdisc show
> 
> 

Powered by blists - more mailing lists