On 3/30/22 11:35, Eelco Chaudron wrote: > > > On 29 Mar 2022, at 13:06, Ilya Maximets wrote: > >> On 2/25/22 13:29, Marcelo Ricardo Leitner wrote: >>> On Thu, Feb 24, 2022 at 03:20:23PM +0100, Eelco Chaudron wrote: >>>> >>>> >>>> On 23 Feb 2022, at 17:23, Marcelo Ricardo Leitner wrote: >>>> >>>>> On Tue, Feb 22, 2022 at 04:26:10PM +0100, Eelco Chaudron wrote: >>>>>> This patch checks for none offloadable ct_state match flag combinations. >>>>>> If they exist force the +trk flag down to TC Flower >>>>>> >>>>>> Signed-off-by: Eelco Chaudron <echau...@redhat.com> >>>>>> --- >>>>>> v3: >>>>>> - Instead of warning about an invalid flag combination fix it. >>>>>> >>>>>> lib/netdev-offload-tc.c | 6 ++++++ >>>>>> 1 file changed, 6 insertions(+) >>>>>> >>>>>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c >>>>>> index 0105d883f..3d2c1d844 100644 >>>>>> --- a/lib/netdev-offload-tc.c >>>>>> +++ b/lib/netdev-offload-tc.c >>>>>> @@ -1541,6 +1541,12 @@ 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)) { >>>>>> + flower->key.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_TRACKED; >>>>>> + flower->mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_TRACKED; >>>>>> + } >>>>> >>>>> Instead, what about: >>>> >>>> I guess we still need the patch in OVS, as older kernels will exist ;) >>> >>> Hah, indeed! >> >> My main concern around such changes is that revlaidators might >> cycle that flow back a forth if the match is not right, so we >> should be very careful. >> >> This one seems safe as it only makes the match more strict and >> revalidators will not remove such flow. > > I did test for this scenario, and as you already concluded this is not an > issue for this specific change. > > >> However, I'm not sure this workaround covers all the cases. >> The kernel part inside the fl_validate_ct_state tests masked key, >> not the raw one. So, in case +trk set in the key, but not set >> in the mask, the code above will not add the mask bit and the >> kernel will reject the flow. > > I guess you mean the following situations in fl_validate_ct_state, where the > mask/flag are set: > > > - TCA_FLOWER_KEY_CT_FLAGS_NEW && TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED -> > mutual exclusive > OVS handles this since day one, 576126a9, and does this by removing the > TCA_FLOWER_KEY_CT_FLAGS_NEW flag before programming. > Not sure why they added this in the first place, maybe there is a corner > case? > > > - TCA_FLOWER_KEY_CT_FLAGS_NEW && TCA_FLOWER_KEY_CT_FLAGS_REPLY -> mutual > exlusive > - TCA_FLOWER_KEY_CT_FLAGS_INVALID only TCA_FLOWER_KEY_CT_FLAGS_TRACKED can be > set > > These two are currently not handled by OVS. > > I guess as the documentation already explains these three combinations would > never happen, and thus in a real-life scenario, this will never result in a > match. > > > I can do a follow-up patch once I update the system test cases to work with > TC, to handle these three (two) cases.
What I meant was: flower->key.ct_state = TCA_FLOWER_KEY_CT_FLAGS_TRACKED | TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED flower->mask.ct_state = TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED So, ofproto layer requests to match only on the TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED, while TCA_FLOWER_KEY_CT_FLAGS_TRACKED is set only in the key. Kernel will take (key.ct_state & mask.ct_state) and will fail the validation, because TCA_FLOWER_KEY_CT_FLAGS_TRACKED will not be set. Does that make sense? > > >> Another thing is that we do have a way to probe support of >> kernel flags, so we may check if kernel accept flows without >> match on +trk and only add an extra match for such kernels. >> Might be useful in combination with the kernel patch below. >> >> What do you think? > > Does it make sense to do all this in OVS as we know that +trk is always set > anyway? > >>> >>>> >>>>> >>>>> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c >>>>> index 1a9b1f140f9e..b8bfb5733f9e 100644 >>>>> --- a/net/sched/cls_flower.c >>>>> +++ b/net/sched/cls_flower.c >>>>> @@ -1407,12 +1407,6 @@ static int fl_set_enc_opt(struct nlattr **tb, >>>>> struct fl_flow_key *key, >>>>> static int fl_validate_ct_state(u16 state, struct nlattr *tb, >>>>> struct netlink_ext_ack *extack) >>>>> { >>>>> - if (state && !(state & TCA_FLOWER_KEY_CT_FLAGS_TRACKED)) { >>>>> - NL_SET_ERR_MSG_ATTR(extack, tb, >>>>> - "no trk, so no other flag can be set"); >>>>> - return -EINVAL; >>>>> - } >>>>> - >>>>> if (state & TCA_FLOWER_KEY_CT_FLAGS_NEW && >>>>> state & TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED) { >>>>> NL_SET_ERR_MSG_ATTR(extack, tb, >>>>> >>>>> I really don't see an issue in having flower to match only on +trk >>>>> without having +trk together in there. >> >> Marcelo, do you plan to send this out once the net-next is open? >> (I'm not sure if it's open right now) >> >>>> >>>> The kernel change looks fine to me, but I guess in your description the >>>> first +trk was supposed to be something like “any flag but +trk”? >>>> >>> >>> Oh oh, yes. On my previous email, >>> s/match only on +trk/match only on +est/ >>> sorry about that. >>> >>> Marcelo >>> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev