[ovs-dev] [RFC PATCH] lib: Introduce netlink-devlink library

[Frode Nordahl]

On Tue, Mar 23, 2021 at 5:23 PM Ben Pfaff <blp at ovn.org> 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/1456504351-18871-1-git-send-email-jiri@resnulli.us/
> > 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 <frode.nordahl at canonical.com>
>
> 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