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, a...@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.
> >>
> >> <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.
>
> 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?
