[ovs-dev] [PATCH v3 9/9] ofproto-dpif-xlate: Translate timeout policy in ct action

Yi-Hung Wei yihung.wei at gmail.com
Thu Aug 15 00:28:52 UTC 2019


On Tue, Aug 13, 2019 at 8:03 PM Darrell Ball <dlu998 at gmail.com> wrote:
>> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
>> index e988626ea05b..d12b5a91c2eb 100644
>> --- a/lib/dpif-provider.h
>> +++ b/lib/dpif-provider.h
>> @@ -542,6 +542,16 @@ struct dpif_class {
>>                                         struct ct_dpif_timeout_policy *tp);
>>      int (*ct_timeout_policy_dump_done)(struct dpif *, void *state);
>>
>> +    /* Gets timeout policy name based on 'tp_id', 'dl_type' and 'nw_proto'.
>> +     * On success, returns 0, stores the timeout policy name in 'tp_name',
>> +     * and sets 'unwildcard'.  'unwildcard' is true if the timeout
>> +     * policy in 'dpif' is 'dl_type' and 'nw_proto' specific, .i.e. in
>> +     * kernel datapath.  Sets 'unwildcard' to false if the timeout policy
>> +     * is generic to all supported 'dl_type' and 'nw_proto'. */
>> +    int (*ct_get_timeout_policy_name)(struct dpif *, uint32_t tp_id,
>> +                                      uint16_t dl_type, uint8_t nw_proto,
>> +                                      struct ds *tp_name, bool *unwildcard);
>
> I think adding the 'unwildcard' parameter to this particular API is not intuitive;
> I would create a simple dedicated API for it.

As our offline discussion, we will keep this interface as is but
update comment to make the API easier to understand.  I also add some
comment in the caller (in ofproto-dpif.c) to make the 'unwildcard'
idea to be more clear.

>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index 3013d83e96a0..8bbc596e2ce0 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> +bool
>> +ofproto_dpif_ct_zone_timeout_policy_get_name(
>> +    const struct dpif_backer *backer, uint16_t zone, uint16_t dl_type,
>> +    uint8_t nw_proto, struct ds *tp_name, bool *unwildcard)
>> +{
>> +    struct ct_zone *ct_zone;
>> +
>> +    if (!ct_dpif_timeout_policy_support_ipproto(nw_proto)) {
>> +        return false;
>> +    }
>> +
>> +    ct_zone = ct_zone_lookup(&backer->ct_zones, zone);
>
>
> struct ct_zone *ct_zone = ct_zone_lookup(&backer->ct_zones, zone);

Done in v4.


>> --- a/tests/system-traffic.at
>> +++ b/tests/system-traffic.at
>> +dnl Wait until the timeout expire.
>> +dnl We intend to wait a bit longer, because conntrack does not recycle the entry right after it is expired.
>> +sleep 4
>
> Once the issue with sending additional packets after the first timeout expiry is fixed, we should enhance the test
> to resend and re-timeout to make sure it works.

Sure, will modify the test case once the kernel fix is upstream.


>> diff --git a/tests/system-userspace-macros.at b/tests/system-userspace-macros.at
>> index 9d5f3bf419d3..8950a4de7287 100644
>> --- a/tests/system-userspace-macros.at
>> +++ b/tests/system-userspace-macros.at
>> +# VSCTL_ADD_ZONE_TIMEOUT_POLICY([parameters])
>> +#
>> +# Add zone based timeout policy to userspace datapath
>> +m4_define([VSCTL_ADD_ZONE_TIMEOUT_POLICY],
>
>
> TBH, does not seems useful; just hides the what is happening

Thanks for the diff in the other e-mail.  I will fold in the proposed
diff in v4.

Thanks,

-Yi-Hung


More information about the dev mailing list