On 4/5/22 12:33, Ilya Maximets wrote: > On 4/5/22 12:24, Eelco Chaudron wrote: >> >> >> On 4 Apr 2022, at 21:05, Ilya Maximets wrote: >> >>> 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 <[email protected]> >>>>>>>>> --- >>>>>>>>> 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 >> >> Maybe I’m still not understanding you correctly, but the above can not >> happen. The key.ct_state will only be set if the corresponding mask is set. >> >> 1504 if (mask->ct_state & OVS_CS_F_TRACKED) { >> 1505 if (key->ct_state & OVS_CS_F_TRACKED) { >> 1506 flower->key.ct_state |= >> TCA_FLOWER_KEY_CT_FLAGS_TRACKED; >> 1507 } >> 1508 flower->mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_TRACKED; >> 1509 mask->ct_state &= ~OVS_CS_F_TRACKED; >> 1510 } > > Oh. You're right. Sorry, I was thinking in terms of ofproto-generated > keys and masks, but forgot that flower keys/masks are already processed > versions of them. Thanks for pointing that out.
I applied the change now. Thanks! Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
