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. > 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
