On Fri, Jan 26, 2024 at 10:29:27AM -0800, Han Zhou wrote: > On Fri, Jan 26, 2024 at 10:26 AM Han Zhou <hz...@ovn.org> wrote: > > > > > > > > On Fri, Jan 26, 2024 at 7:53 AM Aaron Conole <acon...@redhat.com> wrote: > > > > > > Han Zhou <hz...@ovn.org> writes: > > > > > > > On Thu, Jan 25, 2024 at 12:55 PM Aaron Conole <acon...@redhat.com> > wrote: > > > >> > > > >> From: Kevin Sprague <ksprague0...@gmail.com> > > > >> > > > >> During normal operations, it is useful to understand when a > particular flow > > > >> gets removed from the system. This can be useful when debugging > performance > > > >> issues tied to ofproto flow changes, trying to determine deployed > traffic > > > >> patterns, or while debugging dynamic systems where ports come and go. > > > >> > > > >> Prior to this change, there was a lack of visibility around flow > expiration. > > > >> The existing debugging infrastructure could tell us when a flow was > added to > > > >> the datapath, but not when it was removed or why. > > > >> > > > >> This change introduces a USDT probe at the point where the > revalidator > > > >> determines that the flow should be removed. Additionally, we track > the > > > >> reason for the flow eviction and provide that information as well. > With > > > >> this change, we can track the complete flow lifecycle for the netlink > > > >> datapath by hooking the upcall tracepoint in kernel, the flow put > USDT, and > > > >> the revaldiator USDT, letting us watch as flows are added and > removed from > > > >> the kernel datapath. > > > >> > > > >> This change only enables this information via USDT probe, so it > won't be > > > >> possible to access this information any other way (see: > > > >> Documentation/topics/usdt-probes.rst). > > > >> > > > >> Also included is a script > (utilities/usdt-scripts/flow_reval_monitor.py) > > > >> which serves as a demonstration of how the new USDT probe might be > used > > > >> going forward. > > > >> > > > >> Signed-off-by: Kevin Sprague <ksprague0...@gmail.com> > > > >> Co-authored-by: Aaron Conole <acon...@redhat.com> > > > >> Signed-off-by: Aaron Conole <acon...@redhat.com> > > > > > > > > Thanks Aaron for taking care of this patch. I saw you resolved most > of my comments for the v6 of the original patch: > > > > > https://mail.openvswitch.org/pipermail/ovs-dev/2023-January/401220.html > > > > > > > > Butit seems my last comment was missed: > > > > === > > > > > > > > I do notice a counter in my patch doesn't have a > > > > counterpart in this patch. In revalidator_sweep__(), I have: > > > > if (purge) { > > > > result = UKEY_DELETE; > > > > + COVERAGE_INC(upcall_flow_del_purge); > > > > > > > > Would it be good to add one (e.g. FDR_PURGE) here, too? > > > > > > > > === > > > > Could you check if this can be added? > > > > If this is merged I can rebase my patch on top of this. > > > > > > Sorry I didn't reply to this. > > > > > > I'm not sure it makes sense to add the probe for purge, specifically as > > > the purge is only done in two cases: > > > > > > 1. The threads are being stopped (which should never occur after > > > initialization unless the vswitchd is being stopped / killed) > > > > > > 2. An admin runs a command to purge the revalidators (which isn't a > > > recommended procedure as it can cause lots of really weird side > > > effects and we only use it as a debug tool). > > > > > > Did I understand the case enough? I didn't reread the patch you're > > > proposing, so I might be misunderstanding something. > > > > I believe your understanding is correct. However, I think it would be > good to cover all the reasons of DP flow deletion, and "purge" seems to be > the only case missing now. > > Although purge is less likely to happen in production, it is still > possible, probably in some weird scenarios. Would it be good to add for > completeness, if there is no harm? > > > > Thanks, > > Han > > > That being said, I don't have a strong opinion on this. So I give my ack in > either case: > Acked-by: Han Zhou <hz...@ovn.org>
Perhaps we can treat this as a follow-up item? Acked-by: Simon Horman <ho...@ovn.org> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
