On Tue, Mar 23, 2021 at 5:23 PM Ben Pfaff <[email protected]> wrote: > > On Tue, Mar 23, 2021 at 03:50:32PM +0100, Frode Nordahl wrote: > > The devlink interface was introduced [0] in the Linux 4.6 time > > frame and has since gained traction among multiple hardware > > vendors. > > > > The devlink-port [1] and devlink-info[1] interfaces are > > particularly useful for managing NICs connected to multiple > > distinct CPUs such as SmartNICs. > > > > In such a topology it would be useful to be able to offload > > Open vSwitch and OVN onto the NIC SoC operating system and this > > library will help with discovering and managing ports representing > > resources made available on the host from the NIC SoC side. > > > > The library will be consumed by upcoming proposed changes to OVN, > > and I think it makes sense to maintain it together with the Open > > vSwitch netlink library code. > > > > 0: > > https://lore.kernel.org/netdev/[email protected]/ > > 1: > > https://github.com/torvalds/linux/blob/master/Documentation/networking/devlink/devlink-port.rst > > 2: > > https://github.com/torvalds/linux/blob/master/Documentation/networking/devlink/devlink-info.rst > > > > TODO: > > - Polish a small (non-install) utility that can be used for testing > > dump and monitoring of devlink changes. > > - Write tests > > > > Signed-off-by: Frode Nordahl <[email protected]> > > I took a quick look at this, which should not count as a full review. > It looks to me like it fits well with the existing style and structure > of the OVS code (great!).
Thank you for taking the time to comment, much appreciated! > In my skim, I only noticed a couple of things that might be done > better. One is that there is a pretty repetitive collection of > AC_COMPILE_IFELSE calls in OVS_CHECK_LINUX_DEVLINK. I think that a > straightforward m4_define would avoid repetition and make the whole > thing easier to read. > > (Since the only point of these tests is to check whether enums exist so > they can be #define'd if they don't, there's an alternate approach: just > #define all of them unconditionally if !__KERNEL__. They'd override > existing enums in some cases, but that is harmless. But I could > understand if you thought this was a nasty approach and preferred to > test for each of them.) > > It might make sense to supply a full devlink.h if it's missing. Then > what OVS builds won't depend on what kernel headers are installed on the > build machine. I may just pick you up on the offer to make this bit simpler, I did indeed aim at incremental replacements of non-present bits to stay compatible, but as you point out this is kernel ABI, and we ought to trust it to stay stable for a relatively long time. > I also noticed that NL_UINT_ATTR_ASSIGN and NL_STR_ATTR_ASSIGN could be > written as functions. The former could just return a uint64_t. Right you are, I got caught up in thinking polymorphism in C would translate to macros but had forgotten about the wonders of downcasting. I'll follow up with a new patch set incorporating improvements based on your comments and other findings re-re reviewing my own code + tests soon. Cheers! -- Frode Nordahl _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
