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 <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.
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
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.
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.
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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
