Message-ID: <20180103094025.GA2067@nanopsycho.orion>
Date:   Wed, 3 Jan 2018 10:40:25 +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 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.


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

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

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

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