netdev - Re: [RFC net-next 1/8] pkt_cls: make tc_can_offload

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 24 Jan 2018 00:53:52 -0800
From:   Jakub Kicinski <kubakici@...pl>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, oss-drivers@...ronome.com,
        dsahern@...il.com, Alexander Duyck <alexander.h.duyck@...el.com>,
        "amritha.nambiar@...el.com" <amritha.nambiar@...el.com>
Subject: Re: [RFC net-next 1/8] pkt_cls: make tc_can_offload_extack() check
 chain index

On Wed, 24 Jan 2018 09:28:44 +0100, Jiri Pirko wrote:
> Tue, Jan 23, 2018 at 10:33:33PM CET, jakub.kicinski@...ronome.com wrote:
> >No upstream drivers seem to allow offload of chains other than 0.  
> 
> mlxsw does. And I know that Intel was talking about adding the support
> to i40e iirc.

An, I wrote that commit message before looking deeper into mlxsw.
My understanding is that you only use the simple tc_can_offload() 
check for matchall egress redirect.  For flower which actually makes
use of multiple chains, you also allow block sharing and have the
driver count the number of ports with tc offloads disabled.  Therefore
you don't use tc_can_offload() there.

Is that correct?

> >Save driver developers typing and make tc_can_offload_extack()
> >check for that condition as well.  Rename the function to
> >tc_can_offload_cls() to better represent its application.
> >
> >Signed-off-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
> >---  
> 
> [...]
> 
> >--- a/include/net/pkt_cls.h
> >+++ b/include/net/pkt_cls.h
> >@@ -645,15 +645,21 @@ static inline bool tc_can_offload(const struct net_device *dev)
> > 	return dev->features & NETIF_F_HW_TC;
> > }
> > 
> >-static inline bool tc_can_offload_extack(const struct net_device *dev,
> >-					 struct netlink_ext_ack *extack)
> >+static inline bool tc_can_offload_cls(const struct net_device *dev,
> >+				      struct tc_cls_common_offload *common)
> > {
> >-	bool can = tc_can_offload(dev);
> >-
> >-	if (!can)
> >-		NL_SET_ERR_MSG(extack, "TC offload is disabled on net device");
> >+	if (common->chain_index) {  
> 
> This check here is wrong. It is up to the driver if it supports
> multichain offload or not, should not be checked in a generic code along
> with tc_can_offload.

Mm.. maybe the name is a bit unfortunate now..  How about keeping
tc_can_offload_extack() as is and adding:

static inline bool tc_can_offload_simple(const struct net_device *dev,
				         struct tc_cls_common_offload
*common) {
	if (common->chain_index) {
		NL_SET_ERR_MSG(common->extack,
			       "Driver supports only offload of chain
0"); return false;
	}
	return tc_can_offload_extack(dev, common->extack);
}

"simple" standing for simple driver.

My gut feeling is that the drivers will fall into two categories:
simple drivers which don't share blocks and chains and advanced drivers
which do what mlxsw does and therefore doesn't use those helpers.  IOW
there will be no tc_can_offload_extack() callers.

CCing Intel folks.