On 13 May 2024, at 14:50, Adrian Moreno wrote:
> On 5/13/24 2:48 PM, Ilya Maximets wrote: >> On 5/13/24 13:10, Adrian Moreno wrote: >>> >>> >>> On 5/13/24 12:44 PM, Eelco Chaudron wrote: >>>> >>>> >>>> On 13 May 2024, at 9:01, Adrian Moreno wrote: >>>> >>>>> On 5/10/24 12:06 PM, Eelco Chaudron wrote: >>>>>> On 24 Apr 2024, at 21:53, Adrian Moreno wrote: >>>>>> >>>>>>> This simple program reads from psample and prints the packets to stdout. >>>>>>> It's useful for quickly collecting sampled packets. >>>>>> >>>>>> See some comments below, did not review the actual sample application in >>>>>> detail. >>>>>> >>>>>> //Eelco >>>>>> >>>>>>> Signed-off-by: Adrian Moreno <[email protected]> >>>>>>> --- >>>>>>> Documentation/automake.mk | 1 + >>>>>>> Documentation/conf.py | 2 + >>>>>>> Documentation/ref/index.rst | 1 + >>>>>>> Documentation/ref/ovs-psample.8.rst | 47 ++++ >>>>>>> include/linux/automake.mk | 1 + >>>>>>> include/linux/psample.h | 64 ++++++ >>>>>>> rhel/openvswitch-fedora.spec.in | 2 + >>>>>>> rhel/openvswitch.spec.in | 1 + >>>>>>> utilities/automake.mk | 9 + >>>>>>> utilities/ovs-psample.c | 330 >>>>>>> ++++++++++++++++++++++++++++ >>>>>>> 10 files changed, 458 insertions(+) >>>>>>> create mode 100644 Documentation/ref/ovs-psample.8.rst >>>>>>> create mode 100644 include/linux/psample.h >>>>>>> create mode 100644 utilities/ovs-psample.c >>>>>>> >>>>>>> diff --git a/Documentation/automake.mk b/Documentation/automake.mk >>>>>>> index 47d2e336a..c22facfd6 100644 >>>>>>> --- a/Documentation/automake.mk >>>>>>> +++ b/Documentation/automake.mk >>>>>>> @@ -165,6 +165,7 @@ RST_MANPAGES = \ >>>>>>> ovs-l3ping.8.rst \ >>>>>>> ovs-parse-backtrace.8.rst \ >>>>>>> ovs-pki.8.rst \ >>>>>>> + ovs-psample.8.rst \ >>>>>>> ovs-tcpdump.8.rst \ >>>>>>> ovs-tcpundump.1.rst \ >>>>>>> ovs-test.8.rst \ >>>>>>> diff --git a/Documentation/conf.py b/Documentation/conf.py >>>>>>> index 15785605a..75efed2fc 100644 >>>>>>> --- a/Documentation/conf.py >>>>>>> +++ b/Documentation/conf.py >>>>>>> @@ -134,6 +134,8 @@ _man_pages = [ >>>>>>> u'convert "tcpdump -xx" output to hex strings'), >>>>>>> ('ovs-test.8', >>>>>>> u'Check Linux drivers for performance, vlan and L3 tunneling >>>>>>> problems'), >>>>>>> + ('ovs-psample.8', >>>>>>> + u'Print packets sampled by psample'), >>>>>>> ('ovs-vlan-test.8', >>>>>>> u'Check Linux drivers for problems with vlan traffic'), >>>>>>> ('ovsdb-server.7', >>>>>>> diff --git a/Documentation/ref/index.rst b/Documentation/ref/index.rst >>>>>>> index 03ada932f..d1076435a 100644 >>>>>>> --- a/Documentation/ref/index.rst >>>>>>> +++ b/Documentation/ref/index.rst >>>>>>> @@ -46,6 +46,7 @@ time: >>>>>>> ovs-pki.8 >>>>>>> ovs-sim.1 >>>>>>> ovs-parse-backtrace.8 >>>>>>> + ovs-psample.8 >>>>>>> ovs-tcpdump.8 >>>>>>> ovs-tcpundump.1 >>>>>>> ovs-test.8 >>>>>>> diff --git a/Documentation/ref/ovs-psample.8.rst >>>>>>> b/Documentation/ref/ovs-psample.8.rst >>>>>>> new file mode 100644 >>>>>>> index 000000000..c849c83d8 >>>>>>> --- /dev/null >>>>>>> +++ b/Documentation/ref/ovs-psample.8.rst >>>>>>> @@ -0,0 +1,47 @@ >>>>>>> +========== >>>>>>> +ovs-sample >>>>>> >>>>>> Interesting, here you call it all ovs-sample here, which is like ;) >>>>> >>>>> Yes, at the begining I called it ovs-sample as I thought to make some >>>>> generic tool, but since it ended up being very psample-specific I added >>>>> the "p" (and missed this one). >>>>> >>>>>> >>>>>> You could add options like —local-kernel --local-userspace (--ipfix, >>>>>> --sflow) and it will eventually work on each datapath. >>>> >>>>> >>>>> You mean implementing an IPFIX and sFlow collector? >>>>> >>>> ? >>>>>> If you want to keep this a separate psample utility, I would not have >>>>>> ovs in the name, as it's rather general and not OVS specific. Maybe just >>>>>> psample/psample_mon, as we also have nlmon. >>>>>> >>>>> >>>>> Well, the way the cookie is decoded into observation_domain_id and >>>>> observation_point_id is ovs-specific. >>>>> >>>>> In fact, for the next iteration of the series I will remove the filtering >>>>> API since its getting removed from the kernel series as well and add some >>>>> kind of BPF code that does the filtering. Also I was considering looking >>>>> into the OVSDB to ensure that we filter on groups configured in it or >>>>> else we could wrongly interpet a sampled packet that comes from some >>>>> other subsystem. >>>> >>>> I would prefer to have this as an ova-sample application, which can be >>>> extended with other sample methods as we see fit. So when we added >>>> userspace support we can add it here. If we find someone crazy enough to >>>> do a simple IPFIX and/or sFlow collector it can be added too. >>>> >>>> So my request would be to remove the (p) from ovs-psample, and have a >>>> switch to select --psample (the only supported (MANDATORY) option for now). >>>> >>> >>> Oh, ok. Now I undestand, thank you. We want to leave room for the userspace >>> implementation. Will do. >> >> FYI, we do have sflow and netflow basic collectors in tests/test-sflow.c and >> tests/test-netflow.c. >> >> And I'm not convinced we should maintain an actual collector for any of >> these. >> It's not OVS'es job as a project. Like we're not shipping nlmon, we probably >> shouldn't ship psample or any other collector. But we can and should have >> basic >> implementations for testing purposes in tests/test-psample.c for example. >> >> > > Fair enough, I could move it to tests directory. After all, the original > reason why I wrote this was testing the series. Is that OK with you as well > Eelco? Having this in tests sounds good to me. //Eelco _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
