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

On Tue, Mar 01, 2016 at 09:13:25AM -0800, John Fastabend wrote:

On 16-03-01 09:00 AM, Amir Vadai wrote:

quoted

On Tue, Mar 01, 2016 at 03:52:08PM +0100, Jiri Pirko wrote:

quoted

Tue, Mar 01, 2016 at 03:24:48PM CET, amir@vadai.me wrote:

quoted

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>	
	

quoted

@@ -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?

quoted

+	}
+
+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.

When we get the HW only mode we will need to also flush the hardware
representation in software as well as the hardware state.

Yep, I do think that just failing the operation is the best appraoch.
Will make the design simpler, and from the user point of view, less
surprises.

Jiri?