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