Subject: Re: [patch net-next v4 00/10] net: sched: allow qdiscs to share filter block instances

From: Jakub Kicinski <[email-masked]>
Date: Wed, 3 Jan 2018 23:06:58 -0800

On Thu, 4 Jan 2018 07:57:02 +0100, Jiri Pirko wrote:
> Thu, Jan 04, 2018 at 12:51:52AM CET, [email-masked] wrote:
> >On Wed, 3 Jan 2018 18:22:09 +0100, Jiri Pirko wrote:  
> >> 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.  
> >
> >FWIW I would agree with David that allowing add on a shared block
> >modify filters on another interface can break existing users.  (No
> >opinion on dump and lifetime).  
> 
> I don't think that David is saying that, but why do you think it would
> break existing users?

Perhaps I worded is too strongly as "breaking existing users", but it
certainly introduces surprising side effects.  David put it into words
very well:

On Tue, 2 Jan 2018 19:07:36 -0700, David Ahern wrote:
> 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.
> 
> 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.
> 
> > 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.