On 06/06/2018 00:27, Jakub Kicinski 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. 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.
Maybe we can apply my patch logic of still trying the egress dev if the
block has a single device, and not shared. Is that ok with you?
You're patch seems good as an add on, but the egress hw device (sw1np0)
would go over the tc actions and see if it can offload such rule (output
to software lo device) and would fail anyway.
