From mboxrd@z Thu Jan 1 00:00:00 1970 From: Amir Vadai Subject: Re: [PATCH net-next 6/8] net/mlx5e: Introduce tc offload support Date: Wed, 2 Mar 2016 17:53:37 +0200 Message-ID: <56d70b87.49f9c20a.c7eb3.ffff9f05@mx.google.com> References: <1456842290-7844-1-git-send-email-amir@vadai.me> <1456842290-7844-7-git-send-email-amir@vadai.me> <20160301145208.GB2098@nanopsycho.orion> <56d5c9bd.c13fc20a.046a.ffffe0ce@mx.google.com> <56D5CDB5.4090906@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Jiri Pirko , "David S. Miller" , netdev@vger.kernel.org, Or Gerlitz , John Fastabend , Saeed Mahameed , Hadar Har-Zion , Jiri Pirko To: John Fastabend Return-path: Received: from mail-wm0-f67.google.com ([74.125.82.67]:33295 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932113AbcCBPta (ORCPT ); Wed, 2 Mar 2016 10:49:30 -0500 Received: by mail-wm0-f67.google.com with SMTP id n186so10374908wmn.0 for ; Wed, 02 Mar 2016 07:49:29 -0800 (PST) Content-Disposition: inline In-Reply-To: <56D5CDB5.4090906@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: On Tue, Mar 01, 2016 at 09:13:25AM -0800, John Fastabend wrote: > On 16-03-01 09:00 AM, Amir Vadai wrote: > > On Tue, Mar 01, 2016 at 03:52:08PM +0100, Jiri Pirko wrote: > >> Tue, Mar 01, 2016 at 03:24:48PM CET, amir@vadai.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. > >> > >> > >> > >>> @@ -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. > > 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?