On 11 Sep 2023, at 14:00, Adrian Moreno wrote:
> On 8/21/23 17:41, Eelco Chaudron wrote: >> This patch adds an utility that can be used to determine if >> an issue is related to a lack of Linux kernel resources. >> >> This tool is also featured in a Red Hat developers blog article: >> >> https://developers.redhat.com/articles/2023/07/24/troubleshooting-open-vswitch-kernel-blame >> >> Signed-off-by: Eelco Chaudron <[email protected]> >> --- >> utilities/automake.mk | 4 >> utilities/usdt-scripts/kernel_delay.py | 1397 >> +++++++++++++++++++++++++++++++ >> utilities/usdt-scripts/kernel_delay.rst | 594 +++++++++++++ >> 3 files changed, 1995 insertions(+) >> create mode 100755 utilities/usdt-scripts/kernel_delay.py >> create mode 100644 utilities/usdt-scripts/kernel_delay.rst >> > > I have some comments below but overall an awesome script! Thanks Eelco. Thanks Adrian for the review! I’ve included your comments and will sent out a v4 soon. Below are some comments on stuff I kept as is. Cheers, Eelco <SNIP> >> +import pytz > > IIRC it's the first time we add this dependency. > I think usdt-script dependencies are starting to grow. Have you considered > adding a requirements.txt to document them all and make it easier for users > to consume these scripts? Alternatively, we should maybe add it to the p I’ve added the dependencies in the header of the script. I do not think a requerments.txt will work, as we need one for each script. Also, the BCC dependencies need to be installed manually or through the distribution package. <SNIP> >> +/* >> + * For measuring the hard irq time, we need the following. >> + */ > > nits: > - These comments seem to introduce fairly independent sections of ebpf code > (which makes reading it much easier), but in this case the above "section" is > incomplete without the below tracepoints. Yes this is true, but as they record different stats, it was even worse when I moved it to one section, so for now I think keeping it separate will be more visually appealing ;) <SNIP> >> + >> + def _get_kprobe_c_code(self, function_name, function_content): > > Argument function_name is unused. > Correct, wanted to keep all function definitions the same, see comment below. <SNIP> >> + if BPF.kernel_struct_has_field(b'task_struct', b'state') == 1: >> + source = source.replace('<STATE_FIELD>', 'state') >> + else: >> + source = source.replace('<STATE_FIELD>', '__state') >> + > > Not that it makes a huge difference but have you considered using > bpf_core_field_exists() directly on the ebpf side? With the bpf_core_field_exists it would execute code each time, so decided to just make the BPF code static. <SNIP> >> index 000000000..95e98db34 >> --- /dev/null >> +++ b/utilities/usdt-scripts/kernel_delay.rst > > What do you think about moving this file to (or linking it from) the > Documentation so it gets published? I looked at this, but for now, I decided to keep the debug script stuff together. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
