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 } > 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. The TC match is only made on the masked bits but ofproto, all none mask bits are not matched. > 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. My above patch will take care of this, as it will set the TRACKED key and mask. > 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
