[ovs-dev] [PATCH v2 5/9] ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy tables

Darrell Ball dlu998 at gmail.com
Wed Aug 7 18:51:46 UTC 2019


On Wed, Aug 7, 2019 at 11:40 AM Darrell Ball <dlu998 at gmail.com> wrote:

>
>
> On Tue, Aug 6, 2019 at 9:57 PM Justin Pettit <jpettit at ovn.org> wrote:
>
>>
>> > On Aug 5, 2019, at 8:07 PM, Darrell Ball <dlu998 at gmail.com> wrote:
>> >
>> > On Thu, Aug 1, 2019 at 3:10 PM Yi-Hung Wei <yihung.wei at gmail.com>
>> wrote:
>> >
>> >> +struct ct_timeout_policy {
>> >> +    struct uuid uuid;
>> >> +    unsigned int last_used_seqno;
>> >> +    struct ct_dpif_timeout_policy cdtp;
>> >> +    struct cmap_node node;          /* Element in struct dpif_backer's
>> >> +                                     * "ct_tps" cmap. */
>> >>
>> >
>> >
>> > This looks like a layering violation
>> > should be in dpif-netlink or netlink-conntrack for kernel side
>>
>> Hi, Darrell.  As I mentioned in my code review, I had my own concerns
>> about layering, but mine were from the top-down.  Yi-Hung and I didn't
>> understand your concern here, since these seem to be structures that would
>> be useful regardless of the implementation.  Can you explain a bit more
>> about your layering concerns?
>>
>
>
> I was off yesterday afternoon.
>
> There are 3 behaviors with the patchset that are datapath specific
>
> 1/ Unwildcarding of commit flows with timeout policies
>     As we discussed, the userspace conntrack does not need to do this and
> would not since it is suboptimal
>     since unnecessary flows are generated.
>     This is because userspace conntrack would use a single shared profile
> across all dl_types and ip_proto
>     rather than expanding to 6 profiles as in the case of kernel across
> dl_types and ip_protos.
>
> 2/ Userspace datepath/conntrack can easily manage cleanup of deleted
> profiles using a refcount approach.
>     For userspace conntrack, we don't need to read all the timeouts
> profiles continually and to continually try to
>     delete them from top down hoping to catch a window when the profile is
> not referenced by a flow.
>
> 3/ In terms of timeout profile naming in userspace conntrack, we don't
> need to manage a separate profile ID space for
>     userspace conntrack. We can simply use the uuid directly. This
> simplifies the management of profiles and always
>      keeps knowledge of the profile name in sync across layers.
>

Hence, the comments for this patch center around where the implementation
code is.
I think the code should live in datapath type specific code/files.

Of course, wrappers are needed at higher layers to call the datapath
specific implementations.


>
> Thanks Darrell
>
>
>
>
>>
>> Thanks,
>>
>> --Justin
>>
>>
>>


More information about the dev mailing list