Message-ID: <CAGXJAmxdRVm7jY7FZCNsvd8Kvd_p5FPUSHq8gbZvzn0GSK6=2w@mail.gmail.com>
Date: Fri, 8 Nov 2024 09:55:24 -0800
From: John Ousterhout <ouster@...stanford.edu>
To: Edward Cree <ecree.xilinx@...il.com>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH net-next 01/12] net: homa: define user-visible API for Homa

On Thu, Nov 7, 2024 at 1:58 PM Edward Cree <ecree.xilinx@...il.com> wrote:
>
> On 28/10/2024 21:35, John Ousterhout wrote:
> > Note: for man pages, see the Homa Wiki at:
> > https://homa-transport.atlassian.net/wiki/spaces/HOMA/overview
> >
> > Signed-off-by: John Ousterhout <ouster@...stanford.edu>
> ...
> > +/**
> > + * Holds either an IPv4 or IPv6 address (smaller and easier to use than
> > + * sockaddr_storage).
> > + */
> > +union sockaddr_in_union {
> > +     struct sockaddr sa;
> > +     struct sockaddr_in in4;
> > +     struct sockaddr_in6 in6;
> > +};
>
> Are there fundamental reasons why Homa can only run over IP and not
>  other L3 networks?  Or performance measurements showing that the
>  cost of using sockaddr_storage is excessive?
> Otherwise, baking this into the uAPI seems unwise.

This structure made it easier to write code that runs over both IPv4
and IPv6. But, I see your point about the limitations it creates
(there is no fundamental reason Homa couldn't run over other datagram
protocols). In looking over the code, I don't think this structure is
used anymore in the kernel code or the kernel-user interface (it
appears in one structure, but I believe that field is now obsolete and
can be eliminated); its remaining uses are in user-level code. I will
remove sockaddr_in_union from this file.

> > +     /**
> > +      * @error_addr: the address of the peer is stored here when available.
> > +      * This field is different from the msg_name field in struct msghdr
> > +      * in that the msg_name field isn't set after errors. This field will
> > +      * always be set when peer information is available, which includes
> > +      * some error cases.
> > +      */
> > +     union sockaddr_in_union peer_addr;
>
> Member name (peer_addr) doesn't match the kerneldoc (@error_addr).

I will fix.

> > +int     homa_send(int sockfd, const void *message_buf,
> > +               size_t length, const union sockaddr_in_union *dest_addr,
> > +               uint64_t *id, uint64_t completion_cookie);
> > +int     homa_sendv(int sockfd, const struct iovec *iov,
> > +                int iovcnt, const union sockaddr_in_union *dest_addr,
> > +                uint64_t *id, uint64_t completion_cookie);
> > +ssize_t homa_reply(int sockfd, const void *message_buf,
> > +                size_t length, const union sockaddr_in_union *dest_addr,
> > +                uint64_t id);
> > +ssize_t homa_replyv(int sockfd, const struct iovec *iov,
> > +                 int iovcnt, const union sockaddr_in_union *dest_addr,
> > +                 uint64_t id);
>
> I don't think these belong in here.  They seem to be userland
>  library functions which wrap the sendmsg syscall, and as far as
>  I can tell the definitions corresponding to these prototypes do
>  not appear in the patch series.

I'll remove for now. This leaves open the question of where these
declarations should go once the userland library is upstreamed. Those
library methods are low-level wrappers that make it easier to use the
sendmsg kernel call for Homa; users will probably think of them as if
they were system calls. It feels awkward to require people to #include
2 different header files in order to use Homa kernel calls; is it
considered bad form to mix declarations for very low-level methods
like these ("not much more than kernel calls") with those for "real"
kernel calls? Do you know of other low-level kernel-call wrappers in
Linux that are analogous to these? If so, how are they handled?

Thanks for your comments.

-John-

Powered by blists - more mailing lists