On 17 Jan 2023, at 4:11, Han Zhou wrote:
> 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. That will work, I looked at your comments and they all make sense to me. Let’s wait for Kevin to reply and sent out a new version. //Eelco _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
