On Mon, Jan 16, 2023 at 7:36 AM Eelco Chaudron <echau...@redhat.com> wrote: > > > > On 18 Oct 2022, at 18:11, Adrian Moreno wrote: > > > On 10/14/22 20:07, Han Zhou wrote: > >> > >> > >> On Thu, Oct 13, 2022 at 11:51 PM Adrian Moreno <amore...@redhat.com <mailto:amore...@redhat.com>> wrote: > >>> > >>> > >>> > >>> On 8/9/22 03:41, Han Zhou wrote: > >>>> Signed-off-by: Han Zhou <hz...@ovn.org <mailto:hz...@ovn.org>> > >>>> --- > >>>> ofproto/ofproto-dpif-upcall.c | 13 ++++++++++++- > >>>> 1 file changed, 12 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > >>>> index 23b52ef40..f26a035f4 100644 > >>>> --- a/ofproto/ofproto-dpif-upcall.c > >>>> +++ b/ofproto/ofproto-dpif-upcall.c > >>>> @@ -58,6 +58,10 @@ COVERAGE_DEFINE(upcall_ukey_replace); > >>>> COVERAGE_DEFINE(revalidate_missed_dp_flow); > >>>> COVERAGE_DEFINE(upcall_flow_limit_hit); > >>>> COVERAGE_DEFINE(upcall_flow_limit_kill); > >>>> +COVERAGE_DEFINE(upcall_flow_del_rev); > >>>> +COVERAGE_DEFINE(upcall_flow_del_no_rev); > >>>> +COVERAGE_DEFINE(upcall_flow_del_idle_or_limit); > >>>> +COVERAGE_DEFINE(upcall_flow_del_purge); > >>>> > >>>> /* A thread that reads upcalls from dpif, forwards each upcall's packet, > >>>> * and possibly sets up a kernel flow as a cache. */ > >>>> @@ -2334,7 +2338,12 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey, > >>>> } > >>>> result = revalidate_ukey__(udpif, ukey, push.tcp_flags, > >>>> odp_actions, recircs, ukey->xcache); > >>>> - } /* else delete; too expensive to revalidate */ > >>>> + if (result == UKEY_DELETE) { > >>>> + COVERAGE_INC(upcall_flow_del_rev); > >>>> + } > >>>> + } else { /* else delete; too expensive to revalidate */ > >>>> + COVERAGE_INC(upcall_flow_del_no_rev); > >>>> + } > >>>> } else if (!push.n_packets || ukey->xcache > >>>> || !populate_xcache(udpif, ukey, push.tcp_flags)) { > >>>> result = UKEY_KEEP; > >>>> @@ -2771,6 +2780,7 @@ revalidate(struct revalidator *revalidator) > >>>> } > >>>> if (kill_them_all || (used && used < now - max_idle)) { > >>>> result = UKEY_DELETE; > >>>> + COVERAGE_INC(upcall_flow_del_idle_or_limit); > >>>> } else { > >>>> result = revalidate_ukey(udpif, ukey, &stats, &odp_actions, > >>>> reval_seq, &recircs, > >>>> @@ -2852,6 +2862,7 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge) > >>>> > >>>> if (purge) { > >>>> result = UKEY_DELETE; > >>>> + COVERAGE_INC(upcall_flow_del_purge); > >>>> } else if (!seq_mismatch) { > >>>> result = UKEY_KEEP; > >>>> } else { > >>> > >>> > >>> This has some overlap with the work Kevin has been doing [1]. > >>> > >>> https://patchwork.ozlabs.org/project/openvswitch/patch/20221003200742.78047-1-ksprague0...@gmail.com/ < https://patchwork.ozlabs.org/project/openvswitch/patch/20221003200742.78047-1-ksprague0...@gmail.com/ > > >>> > >>> Maybe this patch can be combined with Kevin's to provide unified visibility > >>> (coverage counters + USDT probes) to the revalidation results? > >>> > >> Thanks Adrian for reviewing! This has been there for 2 months and I was about to send a reminder. > >> And thanks for the pointer to the related patch. These two patches are for similar purposes, while they provide different ways to access the information, and I think both are useful. The coverage counters added here try to provide stats of basic DP flow deletion categories, while USDT probes may provide more details, so I am not sure whether these two need to match to each other exactly. But I agree that at least for the same del reasons these two patches should use the same names to avoid confusion. > >> Do you have any detailed comments regarding the counters in this patch? > >> > > > > I think the counters added by this patch are useful. I just think (as you mention above) that the reason names should be unified with Kevin's work so we have both USDT and counters reporting revalidator decisions. > > I was adding a lot of changes above on naming, but I think it would be better to maybe merge his patch with yours?! > > However, as his patch is waiting for a v7. Maybe you can take his code changes without the script, and add some counters @ the probe location. As I’m fine with his patch, except for the python script. > > Latest Kevin patch, https://patchwork.ozlabs.org/project/openvswitch/patch/20221021163543.508906-1-ksprague0...@gmail.com/ > > Thanks Eelco. To make it simpler, I'd wait for Kevin's patch to be merged and I can rebase this one on top of it. I have provided some minor comments on Kevin's patch, too. Please take a look.
Regards, Han _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
