Message-ID: <20180103172209.GD2067@nanopsycho.orion>
Date:   Wed, 3 Jan 2018 18:22:09 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     David Ahern <dsahern@...il.com>
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

Wed, Jan 03, 2018 at 04:57:23PM CET, dsahern@...il.com wrote:
>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

qdisc add block seems odd as in this point, block 1 has nothing to do
with any qdisc


>$ tc filter add block 1 ....
>
>$ tc qdisc add dev ens7 ingress block 1
>$ tc qdisc add dev ens8 ingress block 1

So when I do just:
tc qdisc add dev ens7 ingress
there won't be a block id created. I would like to be able to have it
done on one device and share on the second when I please to. With your
limitation, I have remove all, add block, add all back again.


>
>$ tc filter show block 1
>(filters listed)
>
>$ tc filter show dev ens7 ingress

So the command will return 0 as everything is ok. List is empty, yet
there are still filters being processed on this qdisc. That is surprise
from where I stand. I as a user would expect a list of all filters being
used here. This what you propose is clearly a breakage.


I think that both of us have different expectations about how this
should work. I said already that "tc filter add block 1 ...." and
"tc filter show block 1" from your approach is fine with me. I even
think that the explicit creation and destruction of block ("tc qdisc add
block 1" and "tc qdisc del block 1") is fine.

However I don't agree about breaking the existing filter add and show
and also imposibility to make not-shared block shared in the runtime
before defining it first.

Now, I suggest I will implement the block api extensions you suggest
without the limitations and send it as a part of this patchset. Would
you be ok with that?

Thanks!


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

ok.


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