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

[Darrell Ball]

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

>
>
> 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.
>>
>
I think '3' is not that important for the userspace datapath, since when
vswitchd restarts we loose everything in the datapath
and will need to reallocate u32 profile IDs and re-associate them, anyways.
So we never loose the association per se.
Also, we would need to increase the size of a datastructure field to use
the uuid directly.

Also, '1' and '2' can be implemented later if it delays things too much. I
can submit a followup patch(es) if needed.


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