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