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/


_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to