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

[Ben Pfaff]

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!).

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 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.