[ovs-dev] [RFC PATCH] lib: Introduce netlink-devlink library
frode.nordahl at canonical.com
Tue Mar 23 22:04:38 UTC 2021
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  in the Linux 4.6 time
> > frame and has since gained traction among multiple hardware
> > vendors.
> > The devlink-port  and devlink-info 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://firstname.lastname@example.org/
> > 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
I'll follow up with a new patch set incorporating improvements based
on your comments and other findings re-re reviewing my own code +
More information about the dev