All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jakub Kicinski <kubakici@wp.pl>
To: Jiri Pirko <jiri@resnulli.us>
Cc: David Ahern <dsahern@gmail.com>,
	netdev@vger.kernel.org, davem@davemloft.net, jhs@mojatatu.com,
	xiyou.wangcong@gmail.com, mlxsw@mellanox.com, andrew@lunn.ch,
	vivien.didelot@savoirfairelinux.com, f.fainelli@gmail.com,
	michael.chan@broadcom.com, ganeshgr@chelsio.com,
	saeedm@mellanox.com, matanb@mellanox.com, leonro@mellanox.com,
	idosch@mellanox.com, simon.horman@netronome.com,
	pieter.jansenvanvuuren@netronome.com, john.hurley@netronome.com,
	alexander.h.duyck@intel.com, ogerlitz@mellanox.com,
	john.fastabend@gmail.com, daniel@iogearbox.net
Subject: Re: [patch net-next v4 00/10] net: sched: allow qdiscs to share filter block instances
Date: Wed, 3 Jan 2018 23:06:58 -0800	[thread overview]
Message-ID: <20180103230658.595eac7d@cakuba.netronome.com> (raw)
In-Reply-To: <20180104065702.GH2067@nanopsycho.orion>

On Thu, 4 Jan 2018 07:57:02 +0100, Jiri Pirko wrote:
> Thu, Jan 04, 2018 at 12:51:52AM CET, kubakici@wp.pl 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.

  reply	other threads:[~2018-01-04  7:07 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-23 15:54 [patch net-next v4 00/10] net: sched: allow qdiscs to share filter block instances Jiri Pirko
2017-12-23 15:54 ` [patch net-next v4 01/10] net: sched: introduce support for multiple filter chain pointers registration Jiri Pirko
2017-12-23 15:54 ` [patch net-next v4 02/10] net: sched: avoid usage of tp->q in tcf_classify Jiri Pirko
2017-12-23 15:54 ` [patch net-next v4 03/10] net: sched: introduce block mechanism to handle netif_keep_dst calls Jiri Pirko
2017-12-23 15:54 ` [patch net-next v4 04/10] net: sched: remove classid and q fields from tcf_proto Jiri Pirko
2017-12-23 15:54 ` [patch net-next v4 05/10] net: sched: keep track of offloaded filters and check tc offload feature Jiri Pirko
2017-12-24  2:20   ` Jakub Kicinski
2017-12-24  7:52     ` Jiri Pirko
2017-12-23 15:54 ` [patch net-next v4 06/10] net: sched: allow ingress and clsact qdiscs to share filter blocks Jiri Pirko
2017-12-23 15:54 ` [patch net-next v4 07/10] mlxsw: spectrum_acl: Reshuffle code around mlxsw_sp_acl_ruleset_create/destroy Jiri Pirko
2017-12-23 15:54 ` [patch net-next v4 08/10] mlxsw: spectrum_acl: Don't store netdev and ingress for ruleset unbind Jiri Pirko
2017-12-23 15:54 ` [patch net-next v4 09/10] mlxsw: spectrum_acl: Implement TC block sharing Jiri Pirko
2017-12-23 15:54 ` [patch net-next v4 10/10] mlxsw: spectrum_acl: Pass mlxsw_sp_port down to ruleset bind/unbind ops Jiri Pirko
2017-12-23 16:06 ` [patch iproute2] tc: implement filter block sharing to ingress and clsact qdiscs Jiri Pirko
2017-12-24  1:54 ` [patch net-next v4 00/10] net: sched: allow qdiscs to share filter block instances David Ahern
2017-12-24  7:19   ` Jiri Pirko
2017-12-24 16:25     ` David Ahern
2017-12-25 10:23       ` Jiri Pirko
2018-01-02 19:49         ` Jiri Pirko
2018-01-03  2:07           ` David Ahern
2018-01-03  9:40             ` Jiri Pirko
2018-01-03 15:57               ` David Ahern
2018-01-03 17:22                 ` Jiri Pirko
2018-01-03 23:51                   ` Jakub Kicinski
2018-01-04  6:57                     ` Jiri Pirko
2018-01-04  7:06                       ` Jakub Kicinski [this message]
2018-01-04 10:12                         ` Jiri Pirko
2018-01-04 12:41                           ` Jamal Hadi Salim
2018-01-04 13:00                             ` Jiri Pirko
2018-01-04 13:30                               ` Jamal Hadi Salim
2018-01-04 14:02                                 ` Jiri Pirko
2018-01-04 15:45                             ` David Miller
2018-01-04 12:55                           ` Jamal Hadi Salim
2018-01-04 13:05                             ` Jiri Pirko
2018-01-04 13:43                               ` Jamal Hadi Salim
2018-01-04 14:06                                 ` Jiri Pirko
2018-01-04 15:42                                   ` Jamal Hadi Salim
2018-01-04 15:33                           ` David Miller
2018-01-04 15:51                             ` Jiri Pirko
2018-01-05 10:38                               ` Jiri Pirko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180103230658.595eac7d@cakuba.netronome.com \
    --to=kubakici@wp.pl \
    --cc=alexander.h.duyck@intel.com \
    --cc=andrew@lunn.ch \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=dsahern@gmail.com \
    --cc=f.fainelli@gmail.com \
    --cc=ganeshgr@chelsio.com \
    --cc=idosch@mellanox.com \
    --cc=jhs@mojatatu.com \
    --cc=jiri@resnulli.us \
    --cc=john.fastabend@gmail.com \
    --cc=john.hurley@netronome.com \
    --cc=leonro@mellanox.com \
    --cc=matanb@mellanox.com \
    --cc=michael.chan@broadcom.com \
    --cc=mlxsw@mellanox.com \
    --cc=netdev@vger.kernel.org \
    --cc=ogerlitz@mellanox.com \
    --cc=pieter.jansenvanvuuren@netronome.com \
    --cc=saeedm@mellanox.com \
    --cc=simon.horman@netronome.com \
    --cc=vivien.didelot@savoirfairelinux.com \
    --cc=xiyou.wangcong@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.