On 12/7/21 16:36, Mohammad Heib wrote: > Hi Ilya, > > Thank you for reviewing the patches. > > i fully agree that is a code duplication but I wasn't sure if I can > include those files in nlmon code since those are kernel-headers files. > so I sent an email to OVS-DISCUSS mailing list but didn't get a helpful > answer: > email link: > https://www.mail-archive.com/[email protected]/msg08484.html > <https://www.mail-archive.com/[email protected]/msg08484.html> > > so i decided to submit an initial version of my implementation anyway just to > get feedback:) > and if you think it's a good approach to include one of those files into > nlmon? i will definitely remove all duplication and use the existing code.
include/linux/pkt_cls.h is a file in the OVS's source tree, so there is no reason to not use it. If the actual kernel header available and it is new enough, actual kernel header will be included instead by the include_next directive. You may also try to export and re-use some structures/functions from the lib/tc.c. > > Thanks, > > > On Tue, Dec 7, 2021 at 4:07 PM Ilya Maximets <[email protected] > <mailto:[email protected]>> wrote: > > On 11/22/21 00:39, Mohammad Heib wrote: > > These two patches aim to make nlmon tool more generic > > and extend its functionality to support TC flowers/actions > > monitoring and parsing. > > > > This change will improve the visibility of the communication > > between the OVS and the TC subsystem and can be used for debugging > > and testing OVS HW offload communication with TC. > > > > The patches added basic support for capturing and parsing TC flower > > create/replace/changes Netlink messages and print those messages after > > parsing them, example: > > > > $ nlmon -l info -t tc > > filter ifindex 10 nsid local protocol 0x806 pref 49148 flower chain 0 > handle 0x1 > > eth_type:arp > > filter-flags:[not-in-hw] > > > > filter ifindex 10 nsid local protocol 0x806 pref 49147 flower chain 0 > handle 0x1 > > dst_mac:10:11:12:13:ff:ff > > src_mac:12:13:14:15:16:17 > > eth_type:arp > > filter-flags:[not-in-hw] > > > > Also, > > The first patch adds support for setting the nlmon log level from the > CLI. > > > > This change is backward compatible and apps that use nlmon > > can still use it without any change required. > > > > Mohammad Heib (2): > > utilities/nlmon: extend nlmon design to handle more groups > > utilities/nlmon: Add TC flower monitoring support > > > > utilities/nlmon.c | 575 +++++++++++++++++++++++++++++++++++++++++++++- > > utilities/nlmon.h | 162 +++++++++++++ > > 2 files changed, 727 insertions(+), 10 deletions(-) > > create mode 100644 utilities/nlmon.h > > > > Hi, Mohammad. > > CI fails to build this patch set. Please, re-check. > > On a brief look through the patches I see that you're adding a lot of > things into > nlmon.{c,h}, which are already defined in include/linux/pkt_cls.h or > lib/tc.{c,h}. > It should be possible to re-use most of them instead of duplicating. > > Best regards, Ilya Maximets. > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
