Message-ID: <20241219174109.198f7094@kernel.org>
Date: Thu, 19 Dec 2024 17:41:09 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: John Ousterhout <ouster@...stanford.edu>
Cc: netdev@...r.kernel.org, pabeni@...hat.com, edumazet@...gle.com,
 horms@...nel.org
Subject: Re: [PATCH net-next v4 01/12] inet: homa: define user-visible API
 for Homa

On Thu, 19 Dec 2024 10:57:22 -0800 John Ousterhout wrote:
> On Wed, Dec 18, 2024 at 5:43 PM Jakub Kicinski <kuba@...nel.org> wrote:
> >
> > On Mon, 16 Dec 2024 16:06:14 -0800 John Ousterhout wrote:  
> > > +#ifdef __cplusplus
> > > +extern "C"
> > > +{
> > > +#endif  
> >
> > I'm not aware of any networking header wrapped in extern "C"
> > Let's not make this precedent?  
> 
> Without this I don't seem to be able to use this header in C++ files:
> I end up getting linker errors such as 'undefined reference to
> `homa_replyv(int, iovec const*, int, sockaddr const*, unsigned int,
> unsigned long)'.

No idea TBH, I don't see homa_reply anywhere in the submission

> Any suggestions on how to make the header file work with C++ files
> without the #ifdef __cplusplus?

With the little C++ understanding I have, I _think_ the include site
can wrap:

extern "C" {
#include "<linux/homa.h>"
}

> > > +/**
> > > + * define HOMA_MIN_DEFAULT_PORT - The 16-bit port space is divided into
> > > + * two nonoverlapping regions. Ports 1-32767 are reserved exclusively
> > > + * for well-defined server ports. The remaining ports are used for client
> > > + * ports; these are allocated automatically by Homa. Port 0 is reserved.
> > > + */
> > > +#define HOMA_MIN_DEFAULT_PORT 0x8000  
> >
> > Not sure why but ./scripts/kernel-doc does not like this:
> >
> > include/uapi/linux/homa.h:51: warning: expecting prototype for HOMA_MIN_DEFAULT_PORT - The 16(). Prototype was for HOMA_MIN_DEFAULT_PORT() instead  
> 
> I saw this warning from kernel-doc before I posted the patch, but I
> couldn't figure out why it is happening. After staring at the error
> message some more I figured it out: kernel-doc is getting confused by
> the "-" in "16-bit" (it seems to use the last "-" on the line rather
> than the first). I've modified the comment to replace "16-bit" with
> "16 bit" and filed a bug report for kernel-doc.

Unless you're planning to render the docs on docs.kernel.org you can
just switch from /** to /* comments. IMVHO kdoc is overused, unless
there's full documentation with API rendered like
https://www.kernel.org/doc/html/latest/networking/net_dim.html
kdoc is more trouble than gain.

> > > +/** define SO_HOMA_RCVBUF - setsockopt option for specifying buffer region. */
> > > +#define SO_HOMA_RCVBUF 10
> > > +
> > > +/** struct homa_rcvbuf_args - setsockopt argument for SO_HOMA_RCVBUF. */
> > > +struct homa_rcvbuf_args {
> > > +     /** @start: First byte of buffer region. */
> > > +     void *start;  
> >
> > I'm not sure if pointers are legal in uAPI.
> > I *think* we are supposed to use __aligned_u64, because pointers
> > will be different size for 32b binaries running in compat mode
> > on 64b kernels, or some such.  
> 
> I see that "void *" is used in the declaration for struct msghdr
> (along with some other pointer types as well) and struct msghdr is
> part of several uAPI interfaces, no?

Off the top off my head this use is a source of major pain, grep around
for compat_msghdr.

> > > +/**
> > > + * define HOMA_FLAG_DONT_THROTTLE - disable the output throttling mechanism:
> > > + * always send all packets immediately.
> > > + */  
> >
> > Also makes kernel-doc unhappy:
> >
> > include/uapi/linux/homa.h:159: warning: expecting prototype for HOMA_FLAG_DONT_THROTTLE - disable the output throttling mechanism(). Prototype was for HOMA_FLAG_DONT_THROTTLE() instead  
> 
> It seems that the ":" also confuses kernel-doc. I've worked around this as well.
> 
> > Note that next patch adds more kernel-doc warnings, you probably want
> > to TAL at those as well. Use
> >
> >   ./scripts/kernel-doc -none -Wall $file  
> 
> Hmm, I did run kernel-doc before posting the patch, but maybe I missed
> some stuff. I'll take another look. There are a few things kernel-doc
> complained about where the requested documentation would add no useful
> information; it would just end up repeating what is already obvious
> from the code. Is any discretion allowed for cases like this? If the
> expectation is that there will be zero kernel-doc complaints, then
> I'll go ahead and add the useless documentation.

My recommendation is to use normal comments where kdoc just repeats
obvious stuff. All these warnings sooner or later will result in some
semi-automated and often poor quality patch submissions to "fix" it.
Which is just work for maintainers to deal with :(

Powered by blists - more mailing lists