Re: [RFC PATCH net-next 2/3] flow_offload: restore ability to collect separate stats per action

From: Jakub Kicinski <jakub.kicinski@netronome.com>
To: Edward Cree <ecree@solarflare.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>,
	Jiri Pirko <jiri@resnulli.us>,
	Pablo Neira Ayuso <pablo@netfilter.org>,
	David Miller <davem@davemloft.net>,
	netdev <netdev@vger.kernel.org>,
	"Cong Wang" <xiyou.wangcong@gmail.com>,
	Andy Gospodarek <andy@greyhouse.net>,
	Anjali Singhai Jain <anjali.singhai@intel.com>,
	Or Gerlitz <gerlitz.or@gmail.com>
Subject: Re: [RFC PATCH net-next 2/3] flow_offload: restore ability to collect separate stats per action
Date: Tue, 7 May 2019 10:22:53 -0700	[thread overview]
Message-ID: <20190507102253.37b6681d@cakuba.hsd1.ca.comcast.net> (raw)
In-Reply-To: <1b37d659-5a2b-6130-e8d6-c15d6f57b55e@solarflare.com>

On Tue, 7 May 2019 13:27:15 +0100, Edward Cree wrote:
> On 06/05/2019 13:41, Jamal Hadi Salim wrote:
> > On 2019-05-04 2:27 a.m., Jakub Kicinski wrote:  
> >> On Fri, 3 May 2019 16:06:55 +0100, Edward Cree wrote:  
> >>> Introduce a new offload command TC_CLSFLOWER_STATS_BYINDEX, similar to
> >>>   the existing TC_CLSFLOWER_STATS but specifying an action_index (the
> >>>   tcfa_index of the action), which is called for each stats-having action
> >>>   on the rule.  Drivers should implement either, but not both, of these
> >>>   commands.  
> >>
> >> It feels a little strange to me to call the new stats updates from
> >> cls_flower, if we really want to support action sharing correctly.
> >>
> >> Can RTM_GETACTION not be used to dump actions without dumping the
> >> classifiers?  If we dump from the classifiers wouldn't that lead to
> >> stale stats being returned?  
> >
> > Not sure about the staleness factor, but:
> > For efficiency reasons we certainly need the RTM_GETACTION approach
> > (as you stated above we dont need to dump all that classifier info if
> > all we want are stats). This becomes a big deal if you have a lot
> > of stats/rules.  
> 
> I don't know much of anything about RTM_GETACTION, but it doesn't appear
>  to be part of the current "tc offload" world, which AIUI is very much
>  centred around cls_flower.  I'm just trying to make counters in
>  cls_flower offload do 'the right thing' (whatever that may be), anything
>  else is out of scope.

I understand, and you are correct that the action sharing is not part
of the current "tc offload" world.  My point is that you are making it
part of the offload world, so it'd be best if it could be done well
from the start.

People trying to make the code do the right thing just in their narrow
use case brings us the periodic rewrites of the TC offload
infrastructure, and every time we do it some use cases get broken :/