netdev - Re: [PATCH net-next 6/8] net/mlx5e: Introduce tc offload support

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]

Message-ID: <56d5c9bd.c13fc20a.046a.ffffe0ce@mx.google.com>
Date:	Tue, 1 Mar 2016 19:00:37 +0200
From:	Amir Vadai <amir@...ai.me>
To:	Jiri Pirko <jiri@...nulli.us>
Cc:	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
	Or Gerlitz <ogerlitz@...lanox.com>,
	John Fastabend <john.r.fastabend@...el.com>,
	Saeed Mahameed <saeedm@...lanox.com>,
	Hadar Har-Zion <hadarh@...lanox.com>,
	Jiri Pirko <jiri@...lanox.com>
Subject: Re: [PATCH net-next 6/8] net/mlx5e: Introduce tc offload support

On Tue, Mar 01, 2016 at 03:52:08PM +0100, Jiri Pirko wrote:
> Tue, Mar 01, 2016 at 03:24:48PM CET, amir@...ai.me wrote:
> >Extend ndo_setup_tc() to support ingress tc offloading. Will be used by
> >later patches to offload tc flower filter.
> >
> >Feature is off by default and could be enabled by issuing:
> > # ethtool  -K eth0 hw-tc-offload on
> >
> >Offloads flow table is dynamically created when first filter is
> >added.
> >Rules are saved in a hash table that is maintained by the consumer (for
> >example - the flower offload in the next patch).
> >When last filter is removed and no filters exist in the hash table, the
> >offload flow table is destroyed.
> 
> <snip>	
> 	
> >@@ -1880,6 +1883,17 @@ static int mlx5e_setup_tc(struct net_device *netdev, u8 tc)
> > static int mlx5e_ndo_setup_tc(struct net_device *dev, u32 handle,
> > 			      __be16 proto, struct tc_to_netdev *tc)
> > {
> >+	struct mlx5e_priv *priv = netdev_priv(dev);
> >+
> >+	if (TC_H_MAJ(handle) != TC_H_MAJ(TC_H_INGRESS))
> >+		goto mqprio;
> >+
> >+	switch (tc->type) {
> >+	default:
> >+		return -EINVAL;
> 
> -EOPNOTSUPP would be better here perhaps?
> 
> 
> >+	}
> >+
> >+mqprio:
> > 	if (handle != TC_H_ROOT || tc->type != TC_SETUP_MQPRIO)
> > 		return -EINVAL;
> > 
> >@@ -1963,6 +1977,13 @@ static int mlx5e_set_features(struct net_device *netdev,
> > 			mlx5e_disable_vlan_filter(priv);
> > 	}
> > 
> >+	if ((changes & NETIF_F_HW_TC) && !(features & NETIF_F_HW_TC) &&
> >+	    mlx5e_tc_num_filters(priv)) {
> >+		netdev_err(netdev,
> >+			   "Active offloaded tc filters, can't turn hw_tc_offload off\n");
> >+		return -EINVAL;
> 
> This should not fail I believe. Just disable it in hw. I would even toss
> away the rules if necessary.
It depends on the answer regarding your comment on the previous patch.
If we have the rule in both SW and HW, and remove it from the HW it is
ok (although, currently I don't understand why would anyone want in both
places).
If the rule is processed by HW only - turning off this flag, will
disable the offloaded rules - it might be misleading, so I prefered not
to allow it and print a message.


>