Re: [net-next,v5,00/12] add flow_rule infrastructure

From: Michal Kubecek <mkubecek@suse.cz>
To: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: Jakub Kicinski <jakub.kicinski@netronome.com>,
	Or Gerlitz <gerlitz.or@gmail.com>,
	David Miller <davem@davemloft.net>,
	Florian Westphal <fw@strlen.de>,
	Linux Netdev List <netdev@vger.kernel.org>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Jiri Pirko <jiri@mellanox.com>,
	Jamal Hadi Salim <jhs@mojatatu.com>
Subject: Re: [net-next,v5,00/12] add flow_rule infrastructure
Date: Thu, 13 Dec 2018 21:17:38 +0100	[thread overview]
Message-ID: <20181213201738.GI21324@unicorn.suse.cz> (raw)
In-Reply-To: <20181213195439.dn7wt2mlfoo5dkpw@salvia>

On Thu, Dec 13, 2018 at 08:54:39PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Dec 13, 2018 at 11:23:15AM -0800, Jakub Kicinski wrote:
> > I think having the drivers call the IR translation could be a good
> > compromise instead of having flower always pass down converted flows.
> > 
> > tc flower ->  flower offload object -> setup_tc   -> driver -> 
> >   -> flow_offload_from_flower() -> driver -> driver's common handling
> > 
> > This patchset already does that for ethtool:
> > 
> > ethtool   ->  ethtool flow          -> ethtool_op -> driver -> 
> >   -> ethtool_rx_flow_rule_create() -> driver -> driver's common handling
> > 
> > It feels like a bit of a waste to let the driver patches go, but
> > perhaps it's a good way to move forward?
> 
> I think Michal Kubecek mentioned that it would be good if, moving
> forward, core/ethtool.c calls ethtool_rx_flow_rule_create(), so this is:
> 
> ethtool   ->  ethtool flow -> ethtool_rx_flow_rule_create() ->
>  -> ethtool_op -> driver -> driver's common handling
> 
> In that case, ethtool would match what flower is doing.
> 
> We can probably offer the flow_rule representation as parameter via
> ethtool_op indirection, so there is a single ethtool_rx_flow_rule_create()
> call. Still, this new infrastructure is used by two ethtool drivers,
> so probably it is too early for this consolidation work.

The main reason why I would prefer the translation in the generic
ethtool code is that once we have netlink interface for ethtool, current
model would require us to translate netlink messages to the data
structures for current ethtool_ops handlers (which are essentially the
same as those used by ethtool ioctl interface) only to translate them
once more in the NIC handler to the common representation.

So in the long term it seems better to let ethtool_ops use your
representation of Rx flow rules and translate both the ioctl structures
and netlink messages into it in the generic code so that NIC drivers
would only work with flow_rule.

But at the moment, this would mean a lot of work for not so much gain as
it would mean adjusting all NIC drivers supporting Rx flow rule handling
via ethtool. Also, it would look strange if the new ethtool_ops handlers
passed extack which the only caller wouldn't use.

Michal Kubecek