Date:   Thu, 16 Feb 2023 13:16:41 +0100
From:   Larysa Zaremba <larysa.zaremba@...el.com>
To:     Jesper Dangaard Brouer <jbrouer@...hat.com>
CC:     Alexander Lobakin <aleksander.lobakin@...el.com>,
        <brouer@...hat.com>, <bpf@...r.kernel.org>,
        <netdev@...r.kernel.org>, Stanislav Fomichev <sdf@...gle.com>,
        <martin.lau@...nel.org>, <ast@...nel.org>, <daniel@...earbox.net>,
        <alexandr.lobakin@...el.com>, <xdp-hints@...-project.net>
Subject: Re: [xdp-hints] Re: [PATCH bpf-next V1] xdp: bpf_xdp_metadata use
 NODEV for no device support

On Wed, Feb 15, 2023 at 06:50:10PM +0100, Jesper Dangaard Brouer wrote:
> 
> On 15/02/2023 18.11, Alexander Lobakin wrote:
> > From: Zaremba, Larysa <larysa.zaremba@...el.com>
> > Date: Wed, 15 Feb 2023 16:45:18 +0100
> > 
> > > On Wed, Feb 15, 2023 at 11:09:36AM +0100, Jesper Dangaard Brouer wrote:
> > > > With our XDP-hints kfunc approach, where individual drivers overload the
> > > > default implementation, it can be hard for API users to determine
> > > > whether or not the current device driver have this kfunc available.
> > > > 
> > > > Change the default implementations to use an errno (ENODEV), that
> > > > drivers shouldn't return, to make it possible for BPF runtime to
> > > > determine if bpf kfunc for xdp metadata isn't implemented by driver.
> > > 
> > > I think it diverts ENODEV usage from its original purpose too much.
> 
> Can you suggest a errno that is a better fit?

EOPNOTSUPP fits just fine.

> 
> > > Maybe providing information in dmesg would be a better solution?
> 
> IMHO we really don't want to print any information in this code path, as
> this is being executed as part of the BPF-prog. This will lead to
> unfortunate latency issues.  Also considering the packet rates this need
> to operate at.

I meant printing messages at bpf program load time...
When driver functions are patched-in, you have all the information you may need 
to inform user, if the default implementation for a particular function is used 
instead.

> 
> > 
> > +1, -%ENODEV shouldn't be used here. It stands for "no device", for
> > example the driver probing core doesn't treat it as an error or that
> > something is not supported (rather than there's no device installed
> > in a slot / on a bus etc.).
> > 
> 
> I wanted to choose something that isn't natural for a device driver
> developer to choose as a return code.  I choose the "no device", because
> the "device" driver doesn't implement this.
> 
> The important part is help ourselves (and support) to reliably determine
> if a device driver implements this kfunc or not. I'm not married to the
> specific errno.
> 
> I hit this issue myself, when developing these kfuncs for igc.  I was
> constantly loading and unloading the driver while developing this. And
> my kfunc would return -EOPNOTSUPP in some cases, and I couldn't
> understand why my code changes was not working, but in reality I was
> hitting the default kfunc implementation as it wasn't the correct
> version of the driver I had loaded.  It would in practice have save me
> time while developing...
> 
> Please suggest a better errno if the color is important to you.
> 
> > > 
> > > > 
> > > > This is intended to ease supporting and troubleshooting setups. E.g.
> > > > when users on mailing list report -19 (ENODEV) as an error, then we can
> > > > immediately tell them their kernel is too old.
> > > 
> > > Do you mean driver being too old, not kernel?
> 
> Sure I guess, I do mean the driver version.
> 
> I guess you are thinking in the lines of Intel customers compiling Intel
> out-of-tree kernel modules, this will also be practical and ease
> troubleshooting for Intel support teams.
> 
> > > > 
> > > > Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
> > > > ---
> > [...]
> > 
> > Thanks,
> > Olek
> > 
> 

Powered by blists - more mailing lists