On Mon, Feb 21, 2022 at 12:36:22PM +0100, Ilya Maximets wrote:
> On 2/21/22 10:57, Eelco Chaudron wrote:
> >
> >
> > On 17 Feb 2022, at 14:00, Ilya Maximets wrote:
> >
> >> On 1/31/22 11:53, Eelco Chaudron wrote:
> >>> This patch checks for none offloadable ct_state match flag combinations.
> >>> If they exist tell the user about it once, and ignore the offload.
> >>>
> >>> Signed-off-by: Eelco Chaudron <[email protected]>
> >>> ---
> >>> lib/netdev-offload-tc.c | 18 +++++++++++++++---
> >>> 1 file changed, 15 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> >>> index 8135441c6..f3d1d3e61 100644
> >>> --- a/lib/netdev-offload-tc.c
> >>> +++ b/lib/netdev-offload-tc.c
> >>> @@ -1478,14 +1478,14 @@ flower_match_to_tun_opt(struct tc_flower *flower,
> >>> const struct flow_tnl *tnl,
> >>> flower->mask.tunnel.metadata.present.len = tnl->metadata.present.len;
> >>> }
> >>>
> >>> -static void
> >>> +static int
> >>> parse_match_ct_state_to_flower(struct tc_flower *flower, struct match
> >>> *match)
> >>> {
> >>> const struct flow *key = &match->flow;
> >>> struct flow *mask = &match->wc.masks;
> >>>
> >>> if (!ct_state_support) {
> >>> - return;
> >>> + return 0;
> >>> }
> >>>
> >>> if ((ct_state_support & mask->ct_state) == mask->ct_state) {
> >>> @@ -1541,6 +1541,13 @@ parse_match_ct_state_to_flower(struct tc_flower
> >>> *flower, struct match *match)
> >>> flower->key.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
> >>> flower->mask.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
> >>> }
> >>> +
> >>> + if (flower->key.ct_state &&
> >>> + !(flower->key.ct_state & TCA_FLOWER_KEY_CT_FLAGS_TRACKED)) {
> >>> + VLOG_INFO_ONCE("For ct_state match offload to work, you must
> >>> "
> >>> + "include +trk with any other flags set!");
> >>
> >> Why should OVS care about that? Is it a workaround for a kernel bug?
> >
> > This is not a kernel TC bug, but it just does not make sense to match on ct
> > flags without the +trk flag.
> > So TC marks these as invalid arguments and refuses to take the filter.
> >
> >> I mean, kernel should reject offload attempts for unsupported flag
> >> combinations.
> >
> > Yes, the kernel reports unsupported offload attempts by returning an
> > EINVAL. However, if this is returned as-is, the infrastructure sees this as
> > an error, not an incompatibility. For unsupported it only accepts
> > EOPNOTSUPP.
>
> Should we fix the return value in the kernel then?
> I mean, -trk is a valid match criteria, and if tc doesn't support it,
> it should say that it doesn't support it, instead of complaining that
> arguments are invalid. This is misleading, unless that is documented
> somewhere as invalid tc configuration. Is it?
Wait wait. Tc does support matching on "-trk". What it doesn't
support, similarly to ovs, is a combination like "-trk+est", and this
is what the check above is checking for.
Now on whether that should be EINVAL or EOPNOTSUPP, hm. I don't see a
reason for any future usage of combinations like this one. So at least
on tc level, EINVAL seems to fit as well. Yet, I don't have a strong
argument for A or B here. Well, other than that's probably more common
on UAPI to reject bogus parameters with EINVAL than anything else.
>
> My original fix had this:
> >
> > @@ -1975,6 +1975,11 @@ netdev_tc_flow_put(struct netdev *netdev, struct
> > match *match,
> > add_ufid_tc_mapping(netdev, ufid, &id);
> > }
> >
> > + if (err == EINVAL) {
> > + err = EOPNOTSUPP;
> > + }
> > +
> > return err;
> > }
> >
> > But to make it easier to debug, I decided to catch this earlier and log it.
> > This way, hopefully, we do not get people asking why the offload did not
> > work.
> >
> > I would prefer to keep the error message (with the check), what do you
> > think?
>
> Code snippet above is a bit overboard as it converts every error into
> EOPNOTSUPP, and that doesn't make much sense. For the change in the
> current patch, I think it will also reject plain +est match or something
> similar, because it checks the key, but not the mask.
>
> All in all, all combinations of flags that OVS tries to offload should
> be valid (unless there is an OVS bug in the flow translation layer).
> And we can't make assumptions on why user created this kind of OF pipeline.
>
> If we want to forbid some combination of matches, we should do that at
> the point of OpenFlow flow insertion.
We had issues at tc that it accepted bogus/any combination of
ct_flags. That got fixed in both kernel and ovs and ovs can now probe
for which flags are supported by the running kernel. That's why the
check on ct_state_support in this function.
This particular combination, with "-trk+something", should not be a
valid combination for ovs either (per the man page). I wonder what was
failing and if perhaps we could improve the tc code too/instead.
>
> '-trk' seems to be a very popular match that will be used in almost every
> pipeline, so the INFO message above will be printed always on every
> setup with ct and offload enabled, so I'm not sure how much value it has.
> 'extack' message should, probably, be the right way to receive the
> explanation why the certain flow is not offloaded.
>
> The message also seems to be a bit aggressive, especially since it will
> almost always be printed.
>
> As a temporary solution we may convert the error to EOPNOTSUPP, but for
> this case only, i.e.
> err == EINVAL
> && !(flower->key.ct_state & TCA_FLOWER_KEY_CT_FLAGS_TRACKED)
> && flower->mask.ct_state & TCA_FLOWER_KEY_CT_FLAGS_TRACKED
>
> And maybe print a debug level message.
>
> What do you think?
>
> And while we're here, I'm not sure why we need to check the +est+new
> combination in that code either.
If you mean about:
if (flower->key.ct_state & TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED) {
flower->key.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
flower->mask.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
}
I also don't see a reason for that manipulation.
Best,
Marcelo
>
> Best regards, Ilya Maximets.
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
