[ovs-dev] [PATCH 05/12] ct-dpif: Add conntrack timeout policy support in dpif layer

Yi-Hung Wei yihung.wei at gmail.com
Mon Jul 29 19:37:11 UTC 2019


On Fri, Jul 26, 2019 at 11:41 AM Darrell Ball <dlu998 at gmail.com> wrote:
>
> Thanks for the patch
>
> I found this patch hard to review since it does not contain implementations
> The same comment applies to Patch 6
> I think Patches 5-7 can be combined into one patch, which will make review easier.

Thanks Darrell for the review. Sure, I can squash patch 5-7 altogether in v2.


>> +struct ct_dpif_timeout_policy {
>> +    uint32_t    id;         /* id that uniquely identify a timeout policy. */
>> +    uint32_t    present;    /* If a timeout attribute is present set the
>> +                             * corresponding bit. */
>>
>> +    uint32_t    attrs[CT_DPIF_TP_ATTR_MAX];     /* An array that specifies
>> +                                                 * timeout attribute values */
>
>
> I think you can make attrs of type 'int32_t' and use '-1' timeout for 'not present' and then
> remove the 'present' field

The timeout value is uint32_t in the kernel, so I will keep it as
uint32_t.  I find the present flag to be handy when doing conversion
from ct-dpif layer to dpif-netlink layer, and as you mentioned in the
following e-mail, I will keep it as is.


>> --- a/lib/dpif-netlink.c
>> +++ b/lib/dpif-netlink.c
>> @@ -3434,6 +3434,12 @@ const struct dpif_class dpif_netlink_class = {
>>      dpif_netlink_ct_set_limits,
>>      dpif_netlink_ct_get_limits,
>>      dpif_netlink_ct_del_limits,
>> +    NULL,                       /* ct_set_timeout_policy */
>> +    NULL,                       /* ct_get_timeout_policy */
>> +    NULL,                       /* ct_del_timeout_policy */
>> +    NULL,                       /* ct_timeout_policy_dump_start */
>> +    NULL,                       /* ct_timeout_policy_dump_next */
>> +    NULL,                       /* ct_timeout_policy_dump_done */
>
>
> I found this patch hard to review since it does not contain implementations
> The same comment applies to Patch 6
> I think Patches 5-7 can be combined into one patch, which will make review easier.
>
>
>>
>>      NULL,                       /* ipf_set_enabled */
>>      NULL,                       /* ipf_set_min_frag */
>>      NULL,                       /* ipf_set_max_nfrags */
>> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
>> index 12898b9e3c6d..3460ef8aa98d 100644
>> --- a/lib/dpif-provider.h
>> +++ b/lib/dpif-provider.h
>> @@ -80,6 +80,7 @@ dpif_flow_dump_thread_init(struct dpif_flow_dump_thread *thread,
>>  struct ct_dpif_dump_state;
>>  struct ct_dpif_entry;
>>  struct ct_dpif_tuple;
>> +struct ct_dpif_timeout_policy;
>>
>>  /* 'dpif_ipf_proto_status' and 'dpif_ipf_status' are presently in
>>   * sync with 'ipf_proto_status' and 'ipf_status', but more
>> @@ -498,6 +499,48 @@ struct dpif_class {
>>       * list of 'struct ct_dpif_zone_limit' entries. */
>>      int (*ct_del_limits)(struct dpif *, const struct ovs_list *zone_limits);
>>
>> +    /* Connection tracking timeout policy */
>> +
>> +    /* A connection tracking timeout policy contains a list of timeout
>> +     * attributes that specifies timeout values on various connection states.
>> +     * In a datapath, the timeout policy is identified by a 4 bytes unsigned
>> +     * integer, and the unsupported timeout attributes are ignored.
>> +     * When a connection is committed it can be associated with a timeout
>> +     * policy, or it defaults to the default timeout policy. */
>> +
>> +    /* Add timeout policy '*tp' into the datapath.  If 'is_default' is true
>
>
> "is_default" - can you explain this one ?

This flag is used to configure the default timeout policy in the
datapath.  If 'is_default' is true, it will set the provided timeout
policy to be the default timeout policy. The default timeout policy is
documented in vswitchd/vswitch.xml

>>
>> +     * make the timeout policy to be the default timeout policy. */
>> +    int (*ct_add_timeout_policy)(struct dpif *, bool is_default,
>> +                                 const struct ct_dpif_timeout_policy *tp);
>> +    /* Gets a timeout policy and stores that into '*tp'.
>
>
>
>>
>>   If 'is_default' is
>> +     * true, sets '*tp' to the default timeout policy.  Otherwise, gets the
>
>
> The above text does not make sense:
>  "sets" ?
>
> "is_default" - can you explain this one ?

I re-wreite the comment as following.
    /* Gets a timeout policy and stores that into '*tp'.  If 'is_default' is
     * true, gets default timeout policy.  Otherwise, gets the timeout
     * policy identified by 'tp_id'. */
    int (*ct_get_timeout_policy)(struct dpif *, bool is_default,
                                 uint32_t tp_id,
                                 struct ct_dpif_timeout_policy *tp);

Thanks,

-Yi-Hung


More information about the dev mailing list