Tue, Sep 26, 2017 at 04:50:10PM CEST, pab...@redhat.com wrote: >On Tue, 2017-09-26 at 17:17 +0300, Or Gerlitz wrote: >> On Tue, Sep 26, 2017 at 3:51 PM, Jiri Benc <jb...@redhat.com> wrote: >> > On Tue, 26 Sep 2017 15:41:37 +0300, Or Gerlitz wrote: >> > > Please note that the way the rule is being set to the HW driver is by >> > > delegation >> > > done in flower, see these commits (specifically "Add offload support >> > > using egress Hardware device") >> > >> > It's very well possible the bug is somewhere in net/sched. >> >> maybe before/instead you call it a bug, take a look on the design >> there and maybe >> tell us how to possibly do that otherwise? > >The problem, AFAICT, is in the API between flower and NIC implementing >the offload, because in the above example the kernel will call the >offload hook with exactly the same arguments with the 'bad' rule and >the 'good' one - but the 'bad' rule should never match any packets. > >I think that can be fixed changing the flower code to invoke the >offload hook for filters with tunnel-based match only if the device >specified in such match has the appropriate type, e.g. given that >currently only vxlan is supported with something like the code below >(very rough and untested, just to give the idea): > >Cheers, > >Paolo > >--- >diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c >index d230cb4c8094..ff8476e56d4e 100644 >--- a/net/sched/cls_flower.c >+++ b/net/sched/cls_flower.c >@@ -243,10 +243,11 @@ static int fl_hw_replace_filter(struct tcf_proto *tp, > struct fl_flow_key *mask, > struct cls_fl_filter *f) > { >- struct net_device *dev = tp->q->dev_queue->dev; >+ struct net_device *ingress_dev, *dev = tp->q->dev_queue->dev; > struct tc_cls_flower_offload cls_flower = {}; > int err; > >+ ingress_dev = dev; > 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))) { >@@ -259,6 +260,12 @@ static int fl_hw_replace_filter(struct tcf_proto *tp, > f->hw_dev = dev; > } > >+ if ((dissector_uses_key(f->dissector, FLOW_DISSECTOR_KEY_ENC_KEYID) || >+ dissector_uses_key(f->dissector, FLOW_DISSECTOR_KEY_ENC_PORTS) || >+ // ... list all the others tunnel based keys ... >+ ) && strcmp(ingress_dev->rtnl_link_ops->kind, "vxlan")) >+ return tc_skip_sw(f->flags) ? -EINVAL : 0;
This kind of hooks are giving me nightmares. The code is screwed up as it is already. I'm currently working on conversion to callbacks. This part is handled in: https://github.com/jpirko/linux_mlxsw/commits/jiri_devel_egdevcb