On Wed, Aug 7, 2019 at 11:51 AM Darrell Ball <[email protected]> wrote:
> > > On Wed, Aug 7, 2019 at 11:40 AM Darrell Ball <[email protected]> wrote: > >> >> >> On Tue, Aug 6, 2019 at 9:57 PM Justin Pettit <[email protected]> wrote: >> >>> >>> > On Aug 5, 2019, at 8:07 PM, Darrell Ball <[email protected]> wrote: >>> > >>> > On Thu, Aug 1, 2019 at 3:10 PM Yi-Hung Wei <[email protected]> >>> 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 >>> >>> >>> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
