From: Aaron Conole <[email protected]>
Date: 2022-01-17 23:51:55
To: [email protected]
Cc: [email protected],[email protected],[email protected]
Subject: Re: [ovs-dev] [PATCH v6] conntrack: support default timeout policy
get/set cmd for netdev datapath>Aaron Conole <[email protected]> writes:
>
>> [email protected] writes:
>>
>>> From: wenxu <[email protected]>
>>>
>>> Now, the default timeout policy for netdev datapath is hard codeing. In
>>> some case show or modify is needed.
>>> Add command for get/set default timeout policy. Using like this:
>>>
>>> ovs-appctl dpctl/ct-get-default-tp [dp]
>>> ovs-appctl dpctl/ct-set-default-tp [dp] policies
>>>
>>> Signed-off-by: wenxu <[email protected]>
>>> ---
>>
>> So far looks good. Just one minor comment (and one conflict) that might be
>> able to be addressed
>
>After over an hour of loops, I did manage to produce one failure with
>your test case:
>
>--- - 2022-01-17 10:31:12.740151293 -0500
>+++
>/home/aconole/git/ovs2/ovs/tests/system-userspace-testsuite.dir/at-groups/94/stdout
> 2022-01-17 10:31:12.739245956 -0500
>@@ -1,3 +1,2 @@
>-icmp,orig=(src=10.1.1.1,dst=10.1.1.2,id=<cleared>,type=8,code=0),reply=(src=10.1.1.2,dst=10.1.1.1,id=<cleared>,type=0,code=0),zone=5
> udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),zone=5
>
>
>I think the test suite is "stable" but it might be better to use
>something like 'ovs-ofctl monitor' to watch for events. Maybe you have
>tried this already?
>
>The errant 'sleep's can result in failures like this.
This ‘sleep’ is just following the testcase [conntrack - zone-based timeout
policy]. Maybe it is enough?
>
>>> NEWS | 4 +++
>>> lib/conntrack-tp.c | 11 +++++++
>>> lib/conntrack-tp.h | 2 ++
>>> lib/ct-dpif.c | 56 ++++++++++++++++++++++++++++++++
>>> lib/ct-dpif.h | 9 ++++++
>>> lib/dpctl.c | 69
>>> ++++++++++++++++++++++++++++++++++++++++
>>> lib/dpif-netdev.c | 25 +++++++++++++++
>>> lib/dpif-netlink.c | 2 ++
>>> lib/dpif-provider.h | 8 +++++
>>> tests/system-kmod-macros.at | 10 ++++++
>>> tests/system-traffic.at | 67
>>> ++++++++++++++++++++++++++++++++++++++
>>> tests/system-userspace-macros.at | 7 ++++
>>> 12 files changed, 270 insertions(+)
>>>
>>> diff --git a/NEWS b/NEWS
>>> index 553a57d..f9fc04a 100644
>>> --- a/NEWS
>>> +++ b/NEWS
>>> @@ -39,6 +39,10 @@ Post-v2.16.0
>>> is mainly for the benefit of OVN load balancing configurations.
>>> - Ingress policing on Linux now uses 'matchall' classifier instead of
>>> 'basic', if available.
>>> + - ovs-appctl dpctl/:
>>> + * New commands 'ct-set-default-tp' and
>>> + 'ct-set-default-tp' that allows to get or configure
>>> + netdev datapath ct default timeout policy.
>>
>> I had a conflict with this section on apply. It isn't a huge deal.
>>
>>> diff --git a/lib/conntrack-tp.h b/lib/conntrack-tp.h
>>> index 4d411d1..07dcb4e 100644
>>> --- a/lib/conntrack-tp.h
>>> +++ b/lib/conntrack-tp.h
>>> @@ -710,6 +729,43 @@ ct_dpif_free_zone_limits(struct ovs_list *zone_limits)
>>> }
>>> }
>>>
>>> +
>>> +/* Parses a specification of a timeout policy from 's' into '*tp'
>>> + * . Returns true on success. Otherwise, returns false and
>>> + * and puts the error message in 'ds'. */
>>
>> The '.' in the above is ending the sentence on the previous line, and
>> the world 'and' appears twice in a row.
>>
>> I think it should read:
>>
>> /* Parses a specification of a timeout policy from 's' into '*tp'.
>> * Returns true on success. Otherwise, returns false and puts the
>> * error message in 'ds'. */
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
