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