netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jakub Kicinski <kuba@kernel.org>
To: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: Edward Cree <ecree@solarflare.com>, Jiri Pirko <jiri@resnulli.us>,
	netdev@vger.kernel.org, davem@davemloft.net, saeedm@mellanox.com,
	leon@kernel.org, michael.chan@broadcom.com, vishal@chelsio.com,
	jeffrey.t.kirsher@intel.com, idosch@mellanox.com,
	aelior@marvell.com, peppe.cavallaro@st.com,
	alexandre.torgue@st.com, jhs@mojatatu.com,
	xiyou.wangcong@gmail.com, mlxsw@mellanox.com,
	netfilter-devel@vger.kernel.org
Subject: Re: [patch net-next v2 01/12] flow_offload: Introduce offload of HW stats type
Date: Mon, 2 Mar 2020 12:18:52 -0800	[thread overview]
Message-ID: <20200302121852.50a4fccc@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> (raw)
In-Reply-To: <20200302192437.wtge3ze775thigzp@salvia>

On Mon, 2 Mar 2020 20:24:37 +0100 Pablo Neira Ayuso wrote:
> On Mon, Mar 02, 2020 at 04:29:32PM +0000, Edward Cree wrote:
> > On 02/03/2020 13:20, Pablo Neira Ayuso wrote:  
> > > 2) explicit counter action, in this case the user specifies explicitly
> > >    that it needs a counter in a given position of the rule. This
> > >    counter might come before or after the actual action.  
> >
> > But the existing API can already do this, with a gact pipe.  Plus, Jiri's
> >  new API will allow specifying a counter on any action (rather than only,
> >  implicitly, those which have .stats_update()) should that prove to be
> >  necessary.
> > 
> > I really think the 'explicit counter action' is a solution in search of a
> >  problem, let's not add random orthogonality violations.  (Equally if the
> >  counter action had been there first, I'd be against adding counters to
> >  the other actions.)  
> 
> It looks to me that you want to restrict the API to tc for no good
> _technical_ reason.

Undeniably part of the reason is that given how complex flow offloads
got there may be some resistance to large re-factoring. IMHO well
thought out refactoring of stats is needed.. but I'm not convinced 
this is the direction.

Could you give us clearer understanding of what the use cases for the
counter action is?

AFAIK right now actions do the accounting on input. That seems like the
only logical option. Either action takes the packet out of the action
pipeline, in which case even the counter action after will not see it,
or it doesn't and the input counter of the next action can be used.

Given counters must be next to real actions and not other counter
to have value, having them as a separate action seems to make no
difference at all (if users are silly, we can use the pipe/no-op).

IOW modeling the stats as attribute of other actions or a separate
action is entirely equivalent, and there's nothing to be gained from
moving from the existing scheme to explicit actions... other than it'd
make it look more like nft actions... :)

  reply	other threads:[~2020-03-02 20:18 UTC|newest]

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-28 17:24 [patch net-next v2 00/12] net: allow user specify TC action HW stats type Jiri Pirko
2020-02-28 17:24 ` [patch net-next v2 01/12] flow_offload: Introduce offload of " Jiri Pirko
2020-02-29 19:29   ` Pablo Neira Ayuso
2020-03-01  8:44     ` Jiri Pirko
2020-03-02 13:20       ` Pablo Neira Ayuso
2020-03-02 13:58         ` Jiri Pirko
2020-03-02 16:29         ` Edward Cree
2020-03-02 19:24           ` Pablo Neira Ayuso
2020-03-02 20:18             ` Jakub Kicinski [this message]
2020-03-02 21:46               ` Pablo Neira Ayuso
2020-03-02 22:49                 ` Jakub Kicinski
2020-03-03 17:25                   ` Pablo Neira Ayuso
2020-03-03 19:34                     ` Jakub Kicinski
2020-03-03 18:55                   ` Edward Cree
2020-03-03 19:26                     ` Jakub Kicinski
2020-03-03 20:27                     ` Pablo Neira Ayuso
2020-03-03 21:06                       ` Edward Cree
2020-03-03 21:25                         ` Pablo Neira Ayuso
2020-03-03 19:13   ` Edward Cree
2020-03-04 14:18     ` Jiri Pirko
2020-02-28 17:24 ` [patch net-next v2 02/12] ocelot_flower: use flow_offload_has_one_action() helper Jiri Pirko
2020-02-29  0:50   ` Vladimir Oltean
2020-02-28 17:24 ` [patch net-next v2 03/12] flow_offload: check for basic action hw stats type Jiri Pirko
2020-02-28 19:40   ` Jakub Kicinski
2020-02-29  7:40     ` Jiri Pirko
2020-02-29 19:18       ` Jakub Kicinski
2020-03-01  9:00         ` Jiri Pirko
2020-03-02 19:33           ` Jakub Kicinski
2020-03-03 11:46             ` Jiri Pirko
2020-02-28 17:24 ` [patch net-next v2 04/12] mlx5: en_tc: Do not allow mixing HW stats types for actions Jiri Pirko
2020-02-28 17:24 ` [patch net-next v2 05/12] mlxsw: spectrum_flower: " Jiri Pirko
2020-02-28 17:24 ` [patch net-next v2 06/12] mlx5: restrict supported HW stats type to "any" Jiri Pirko
2020-02-28 17:25 ` [patch net-next v2 07/12] mlxsw: " Jiri Pirko
2020-02-28 17:25 ` [patch net-next v2 08/12] flow_offload: introduce "immediate" HW stats type and allow it in mlxsw Jiri Pirko
2020-02-29 19:32   ` Pablo Neira Ayuso
2020-03-01  8:47     ` Jiri Pirko
2020-03-02 13:23       ` Pablo Neira Ayuso
2020-03-02 13:59         ` Jiri Pirko
2020-02-28 17:25 ` [patch net-next v2 09/12] flow_offload: introduce "delayed" HW stats type and allow it in mlx5 Jiri Pirko
2020-02-28 17:25 ` [patch net-next v2 10/12] mlxsw: spectrum_acl: Ask device for rule stats only if counter was created Jiri Pirko
2020-02-28 17:25 ` [patch net-next v2 11/12] flow_offload: introduce "disabled" HW stats type and allow it in mlxsw Jiri Pirko
2020-02-28 17:25 ` [patch net-next v2 12/12] sched: act: allow user to specify type of HW stats for a filter Jiri Pirko
2020-02-28 19:59   ` Jakub Kicinski
2020-02-29  7:52     ` Jiri Pirko
2020-02-29 20:14       ` Jakub Kicinski
2020-03-01  8:57         ` Jiri Pirko
2020-03-02 19:39           ` Jakub Kicinski
2020-03-03 13:20             ` Jiri Pirko
2020-03-03 19:48               ` Jakub Kicinski
2020-03-04  8:15                 ` Jiri Pirko
2020-03-03 19:44   ` Jakub Kicinski

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=20200302121852.50a4fccc@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com \
    --to=kuba@kernel.org \
    --cc=aelior@marvell.com \
    --cc=alexandre.torgue@st.com \
    --cc=davem@davemloft.net \
    --cc=ecree@solarflare.com \
    --cc=idosch@mellanox.com \
    --cc=jeffrey.t.kirsher@intel.com \
    --cc=jhs@mojatatu.com \
    --cc=jiri@resnulli.us \
    --cc=leon@kernel.org \
    --cc=michael.chan@broadcom.com \
    --cc=mlxsw@mellanox.com \
    --cc=netdev@vger.kernel.org \
    --cc=netfilter-devel@vger.kernel.org \
    --cc=pablo@netfilter.org \
    --cc=peppe.cavallaro@st.com \
    --cc=saeedm@mellanox.com \
    --cc=vishal@chelsio.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).