On 19 Jan 2023, at 11:38, Simon Horman wrote:
> On Tue, Jan 17, 2023 at 10:19:23AM +0100, 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]> > > Thanks Eelco, > > this looks very nice. > > A few minor nits from my side, but that not withstanding. > > Acked-by: Simon Horman <[email protected]> > Thanks for looking into this Simon! I will send out a v8 with some of your changes, and one addition to auto-generate the ENUMS so the are synced between eBPF and python. See inline comments below… //Eelco >> +# >> +# event_to_dict() >> +# >> +def event_to_dict(event): >> + event_dict = {} >> + >> + for field, _ in event._fields_: >> + if isinstance(getattr(event, field), (int, bytes)): >> + event_dict[field] = getattr(event, field) >> + >> + return event_dict > > I thought that a list comprehension might be nicer here, > but now I'm not so sure. > > *completely untested* > > return dict([(field, getattr(event, field)) > for (field, _) in fields > if isinstance(getattr(event, field), (int, bytes))]) It’s more python, so I’ll take your suggestion (with a small fix on the ‘fields’ variable. > ... > >> +def _process_event(event): >> + global graph >> + >> + if export_file is not None: >> + export_file.write("event = {}\n".format(event_to_dict(event))) >> + >> + if event.id == 0 and not state['running']: >> + start = state["last_start"] >> + done = state["last_done"] >> + if done and start: >> + actual_wait = (event.ts - done.ts) / 1000000 >> + csv = "{}, {}, {}, {}, {}, {}, {}, {}, {}, {:.2f}".format( >> + start.ts, done.ts, done.n_flows, graph.ukey_count, >> + done.avg_n_flows, done.max_n_flows, done.flow_limit, >> + done.dump_duration, done.poll_wait, actual_wait) >> + >> + if graph.base_time == 0: >> + graph = graph._replace(base_time=done.ts) >> + >> + graph.time.append((done.ts - graph.base_time) / 1000000000) >> + graph.n_flows.append(done.n_flows) >> + graph.n_reval_flows.append(graph.ukey_count) >> + graph.avg_n_flows.append(done.avg_n_flows) >> + graph.max_n_flows.append(done.max_n_flows) >> + graph.flow_limit.append(done.flow_limit) >> + graph.dump_duration.append(done.dump_duration) >> + graph.poll_wait.append(done.poll_wait) >> + graph.actual_wait.append(actual_wait) >> + >> + if not options.no_gui and not options.no_realtime_plots: >> + updated_graph = dynamic_plot_update( >> + graph, refresh=options.update_interval) >> + if updated_graph is None: >> + raise KeyboardInterrupt >> + graph = updated_graph >> + >> + if options.compress_output: >> + last_csv = state["last_csv"] >> + if not last_csv or \ >> + csv.split(",")[2:-1] != last_csv.split(",")[2:-1] or \ >> + abs((event.ts - done.ts) / 1000000 - done.poll_wait) > >> 100: >> + print(csv) >> + else: >> + state["last_not_printed_csv"] = csv >> + >> + state["last_csv"] = csv >> + else: >> + print(csv) > > nit: maybe it is nicer to store csv as a tuple and provide > a helper to print it? (Maybe not :) I’ve decided to keep it as is for now, as else we need to define the tuple instead of the string so there will not be much difference. >> + >> + state["last_start"] = event >> + state['running'] = True >> + graph = graph._replace(ukey_count=0) >> + elif event.id == 1 and state['running']: >> + state["last_done"] = event >> + state['running'] = False >> + elif event.id == 2 and state['running']: >> + graph = graph._replace(ukey_count=graph.ukey_count + 1) >> + >> + >> +# >> +# run_program() >> +# >> +def run_program(command, need_result=True): > > nit: It appears that the need_result is passed by callers. > So perhaps the parameter can be dropped and the function simplified? need_results is always true in this script, so let me remove it. >> + try: >> + process = subprocess.run(command, >> + stdout=subprocess.PIPE, >> + stderr=subprocess.STDOUT, >> + encoding='utf8', >> + check=True) >> + >> + except subprocess.CalledProcessError as perror: >> + if need_result: >> + return perror.returncode, perror.stdout >> + >> + return perror.returncode >> + >> + if need_result: >> + return 0, process.stdout >> + >> + return 0 >> + >> + >> +# >> +# get_ovs_definitions() >> +# >> +def get_ovs_definitions(objects, pahole="pahole", pid=None): >> + if pid is None: >> + raise ValueError("A valid pid value should be supplied!") >> + >> + if not isinstance(objects, list): >> + objects = [objects] >> + >> + if len(objects) == 0: >> + raise ValueError("Must supply at least one object!") >> + >> + vswitchd = Path("/proc/{}/exe".format(str(pid))).resolve() >> + >> + object_str = "" >> + for obj in objects: >> + object_str += obj + ',' >> + >> + object_str = object_str.rstrip(',') > > nit: Maybe something like this is a bit nicer? > > object_str = ','.join(objects) > > *completely untested* ACK will change _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
