On Mon, Feb 21, 2022 at 01:51:37PM +0100, Ilya Maximets wrote:
> On 2/21/22 13:34, Eelco Chaudron wrote:
> >
> >
> > On 21 Feb 2022, at 12:36, 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 <echau...@redhat.com>
> >>>>> ---
> >>>>> 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?
> >
> > Well in practice we supply the wrong arguments, as this is not a valid
> > scenario, so EINVAL is ok.
> >
> >> 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 valid, as the bit value included with the mask, is 0, and this is
> > not creating the warning.
> > It will if we do -trk +new
> >
> >> This is misleading, unless that is documented
> >> somewhere as invalid tc configuration. Is it?
> >
> > Don’t think it is, it makes perfect sense, as we should never have -trk and
> > any other flag.
> > Because if you are not tracking you can not have this state.
> >
> > For fail-safe you normally do “if (flower->key.ct_state &
> > flower->mast.ct_state && ….” but we know that the above code will only set
> > the bit if the mask is also set.
> >
> >
> >> 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.
> >>
> >
> > Yes, that is why I did not follow through.
> >
> >> 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.
> >
> > Yes, I agree, but this is how it is now, so maybe blocking this might
> > render existing deployments no longer working.
> > I know it will fail a hand full of self-tests ;)
> >
> >> '-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.
> >
> > Don’t think this is true, it will only print if +trk and any other flags
> > are set.
> > Guess this is where the miscommunication is.>
> >> The message also seems to be a bit aggressive, especially since it will
> >> almost always be printed.
>
> Yeah. I missed the fact that you're checking for zero and
> flower->key.ct_state
> will actually mark existence of other flags. So, that is fine.
>
> However, I'm still not sure that the condition is fully correct.
>
> If we'll take a match on '+est' with all other flags wildcarded, that will
> trigger the condition, because 'flower->key.ct_state' will contain the 'est'
> bit,
> but 'trk' bit will not be set. The point is that even though -trk+est is not
Oh ow. tc flower will reject this combination today, btw. I don't know
about hw implications for changing that by now.
https://elixir.bootlin.com/linux/latest/C/ident/fl_validate_ct_state
'state' parameter in there is the value masked already.
We directly mapped openflow restrictions to the datapath.
> a valid combination and +trk+est is, OVS may in theory produce the match with
> 'est' bit set and 'trk' bit wildcarded. And that can be a correct
> configuration.
I guess that means that the only possible parameter validation on
ct_state at tc level is about its length. Thoughts?
>
> Or am I missing something else?
>
> >>
> >> 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?
> >
> > See above, I think the existing code is fine, so we do not try to offload
> > it to TC.
> >
> >> And while we're here, I'm not sure why we need to check the +est+new
> >> combination in that code either.
> >
> > Because according to the kernel these two flags are mutually exclusive, so
> > you can not set a filter for both. A connection can be new and established
> > at the same time, not sure why they choose to ignore the new state, guess
> > the kernel module ignores this also.
> >
> > Guess I could also do a similar fix, i.e. force the +trk flag when not set?
> > What do you think!? I did not test this, so it might bring new side effects
> > ;)
> >
> >
> >> Best regards, Ilya Maximets.
> >
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
