On Tue, Mar 29, 2022 at 01:06:36PM +0200, 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. > > 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. > > 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? > > > > >> > >>> > >>> 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)
I can. Just not sure whether it's really useful or not. > > >> > >> 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
