On 3 Jan 2023, at 9:52, Adrian Moreno wrote:
> On 1/2/23 14:12, Eelco Chaudron wrote: >> >> >> On 23 Dec 2022, at 18:05, Adrian Moreno wrote: >> >>> On 12/23/22 10:49, Eelco Chaudron wrote: >>>> >>>> >>>> On 22 Dec 2022, at 18:42, Adrian Moreno wrote: >>>> >>>>> Hi Eelco, >>>>> >>>>> I am not done reviewing this patch. I'm still tyring to figure why I >>>>> can't make the pahole thing work so I'll continue tomorrow. For now, see >>>>> some questions below. >>>> >>>> Are you sure you are on the next branch, not on main, that was my issue >>>> earlier :) >>>> >>>>> On 12/22/22 09:55, Eelco Chaudron wrote: >>>>>> This patch adds a Python script that can be used to analyze the >>>>>> revalidator runs by providing statistics (including some real time >>>>>> graphs). >>>>>> >>>>>> The USDT events can also be captured to a file and used for >>>>>> later offline analysis. >>>>>> >>>>>> The following blog explains the Open vSwitch revalidator >>>>>> implementation and how this tool can help you understand what is >>>>>> happening in your system. >>>>>> >>>>>> https://developers.redhat.com/articles/2022/10/19/open-vswitch-revalidator-process-explained >>>>>> >>>>>> Signed-off-by: Eelco Chaudron <[email protected]> >>>>>> --- >>>>>> >>>>>> v2: Added note that script only works a with single datapath configured. >>>>>> v3: Updated patch to use pahole to get OVS structures dynamically from >>>>>> debug data. >>>>>> v4: Added script to usdt_SCRIPTS >>>>>> >>>>>> Documentation/topics/usdt-probes.rst | 84 +++ >>>>>> ofproto/ofproto-dpif-upcall.c | 11 >>>>>> utilities/automake.mk | 4 >>>>>> utilities/usdt-scripts/reval_monitor.py | 858 >>>>>> +++++++++++++++++++++++++++++++ >>>>>> 4 files changed, 956 insertions(+), 1 deletion(-) >>>>>> create mode 100755 utilities/usdt-scripts/reval_monitor.py >>>>>> >>>>>> diff --git a/Documentation/topics/usdt-probes.rst >>>>>> b/Documentation/topics/usdt-probes.rst >>>>>> index 7ce19aaed..bc250e723 100644 >>>>>> --- a/Documentation/topics/usdt-probes.rst >>>>>> +++ b/Documentation/topics/usdt-probes.rst >>>>>> @@ -214,6 +214,10 @@ Available probes in ``ovs_vswitchd``: >>>>>> - dpif_recv:recv_upcall >>>>>> - main:poll_block >>>>>> - main:run_start >>>>>> +- revalidate_ukey\_\_:entry >>>>>> +- revalidate_ukey\_\_:exit >>>>>> +- udpif_revalidator:start_dump >>>>>> +- udpif_revalidator:sweep_done >>>>>> dpif_netlink_operate\_\_:op_flow_del >>>>>> @@ -327,6 +331,7 @@ probe main:run_start >>>>>> ~~~~~~~~~~~~~~~~~~~~ >>>>>> **Description**: >>>>>> + >>>>>> The ovs-vswitchd's main process contains a loop that runs every time >>>>>> some work >>>>>> needs to be done. This probe gets triggered every time the loop >>>>>> starts from the >>>>>> beginning. See also the ``main:poll_block`` probe below. >>>>>> @@ -344,6 +349,7 @@ probe main:poll_block >>>>>> ~~~~~~~~~~~~~~~~~~~~~ >>>>>> **Description**: >>>>>> + >>>>>> The ovs-vswitchd's main process contains a loop that runs every time >>>>>> some work >>>>>> needs to be done. This probe gets triggered every time the loop is >>>>>> done, and >>>>>> it's about to wait for being re-started by a poll_block() call >>>>>> returning. >>>>>> @@ -358,6 +364,84 @@ See also the ``main:run_start`` probe above. >>>>>> - ``utilities/usdt-scripts/bridge_loop.bt`` >>>>>> +revalidate_ukey\_\_:entry >>>>>> +~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>>> + >>>>>> +**Description**: >>>>>> + >>>>>> +This probe gets triggered on entry of the revalidate_ukey__() function. >>>>>> + >>>>> >>>>> Why not trace the begining and end of revalidate_ukey so we can also >>>>> detect when a flow is evicted because should_revalidate() returns false? >>>> >>>> Because the need for this trace event is to capture the actual revalidate >>>> of a ukey, not the potential one. If we need this we can add a more >>>> specific one, but I think Kevin’s probe will do this. >>>> >>> >>> So, the main goal is to perform timing measurements? I'm asking because if >>> you really want the result of the revalidation, moving the PROBE a fiew >>> lines up and adding the reval_context might help providing insights into >>> what actually happened (i.e: the computed odp_actions). >> >> Not sure if I get it… So you mean moving the probe to revalidate_ukey() >> entry and exit? This will require an additional flag to tell us if we did >> the revalidate, and it might also increase the number of actual calls to the >> probe. Every reval interval vs only the time we do the actual revalidate of >> the flow. >> >> With this in mind, I would rather have two separate probes, or do you have a >> different vision? >> > > No, I agree we should have two separate probes. My question was: do you see > this probe used only for timing/profiling purposes or also for tools to be > able to introspect the results of revalidations? If you see the latter useful > it might be worth considering moving the probe before we call > "netflow_flow_clear" and adding "(struct reval_context *) &ctx" as parameter > since the context seems to hold useful information such as the odp_actions > and the xlated flow. Thanks for clarifying. Looking at the code this will require two USDT probes one for the error case and one for the none error case. So for now I think we should keep this an entry probe, and we can add the additional one if we do need it in the future. What do you think? //Eelco _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
