Message-ID: <CAGXJAmyGqMC=RC-X7T9U4DZ89K=VMpLc0=9MVX6ohs5doViZjg@mail.gmail.com>
Date: Thu, 19 Dec 2024 10:57:22 -0800
From: John Ousterhout <ouster@...stanford.edu>
To: Jakub Kicinski <kuba@...nel.org>
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 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)'.

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

> > +/**
> > + * 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.

> > +/**
> > + * struct homa_sendmsg_args - Provides information needed by Homa's
> > + * sendmsg; passed to sendmsg using the msg_control field.
> > + */
> > +struct homa_sendmsg_args {
> > +     /**
> > +      * @id: (in/out) An initial value of 0 means a new request is
> > +      * being sent; nonzero means the message is a reply to the given
> > +      * id. If the message is a request, then the value is modified to
> > +      * hold the id of the new RPC.
> > +      */
> > +     uint64_t id;
>
> Please use Linux uapi types, __u64

Done. In the process I discovered that the underlying type for __u64
is not the same as that for uint64_t; this results in awkwardness in
programs that use both...

> > +/** 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?

> > +/**
> > + * 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.

> > +#ifdef __cplusplus
> > +}
> > +#endif
> --
> pw-bot: cr

I'm not sure what "pw-bot: cr" means; I assume this is related to the
"#ifdef __cplusplus" discussion above?

Thanks for the comments.

-John-

Powered by blists - more mailing lists