On Wed, 6 Jun 2018 08:15:27 +0300, Or Gerlitz wrote:
> On Wed, Jun 6, 2018 at 12:27 AM, Jakub Kicinski <kubak...@wp.pl> wrote:
> > On Tue, 05 Jun 2018 15:06:40 -0400 (EDT), David Miller wrote:
> >> From: Jakub Kicinski <kubak...@wp.pl>
> >> Date: Tue, 5 Jun 2018 11:57:47 -0700
> >>
> >> > Do we still care about correctness and not breaking backward
> >> > compatibility?
> >>
> >> Jakub let me know if you want me to revert this change.
> >
> > Yes, I think this patch introduces a regression when block is shared
> > between offload capable and in-capable device, therefore it should be
> > reverted. Shared blocks went through a number of review cycles to
> > ensure such cases are handled correctly.
> >
> >
> > Longer story about the actual issue which is never explained in the
> > commit message is this: in kernels 4.10 - 4.14 skip_sw flag was
> > supported on tunnels in cls_flower only:
> >
> > static int fl_hw_replace_filter(struct tcf_proto *tp,
> > [...]
> > if (!tc_can_offload(dev)) {
> > if (tcf_exts_get_dev(dev, &f->exts, &f->hw_dev) ||
> > (f->hw_dev && !tc_can_offload(f->hw_dev))) {
> > f->hw_dev = dev;
> > return tc_skip_sw(f->flags) ? -EINVAL : 0;
> > }
> > dev = f->hw_dev;
> > cls_flower.egress_dev = true;
> > } else {
> > f->hw_dev = dev;
> > }
> >
> >
> > In 4.15 - 4.17 with addition of shared blocks egdev mechanism was
> > promoted to a generic TC thing supported on all filters but it no
> > longer works with skip_sw.
> >
> > I'd argue skip_sw is incorrect for tunnels, because the rule will only
> > apply to traffic ingressing on ASIC ports, not all traffic which hits
> > the tunnel netdev.
>
> This argument makes sense, however, skip_sw for tunnel decap rules
> **is** allowed since 4.10 and we have some sort of regression here (turns
> out that before and after the patch..)
As I said it was allowed in 4 releases, which was a mistake, in last 3
it wasn't. I understand your use case, but the semantics of skip_sw
are not preserved here so we should find a different solution.
> > Therefore we should keep the 4.15 - 4.17 behaviour.
> > But that's a side note, I don't think we should be breaking offload on
> > shared blocks whether we decide to support skip_sw on tunnels or not.
>
> skip_sw on tunnels was there before shared-block, newer features should
> take care not to break existing ones.
Oh, I agree we shouldn't break existing use cases so please don't break
the use case I mentioned above. I want to set up shared block between
a LAG and its members. Now since the bond will not be offload-capable
TC will not even make an attempt to offload to members.
I'm gonna test that my reading of the code is correct and send a revert
later today, sorry.
