netdev - Re: [PATCH net-next v6 03/12] net: homa: create shared Homa header files

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGXJAmzcifEeNthmE2J0epFYUhJYH=XxoJUSxQEqPCjkbhHdBw@mail.gmail.com>
Date: Fri, 24 Jan 2025 13:21:44 -0800
From: John Ousterhout <ouster@...stanford.edu>
To: Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, edumazet@...gle.com, horms@...nel.org, 
	kuba@...nel.org
Subject: Re: [PATCH net-next v6 03/12] net: homa: create shared Homa header files

On Thu, Jan 23, 2025 at 3:01 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> On 1/15/25 7:59 PM, John Ousterhout wrote:
> [...]
> > +/**
> > + * union sockaddr_in_union - Holds either an IPv4 or IPv6 address (smaller
> > + * and easier to use than sockaddr_storage).
> > + */
> > +union sockaddr_in_union {
> > +     /** @sa: Used to access as a generic sockaddr. */
> > +     struct sockaddr sa;
> > +
> > +     /** @in4: Used to access as IPv4 socket. */
> > +     struct sockaddr_in in4;
> > +
> > +     /** @in6: Used to access as IPv6 socket.  */
> > +     struct sockaddr_in6 in6;
> > +};
>
> There are other protocol using the same struct with a different name
> (sctp) or  a very similar struct (mptcp). It would be nice to move this
> in a shared header and allow re-use.

I would be happy to do this, but I suspect it should be done
separately from this patch series. It's not obvious to me where such a
definition should go; can you suggest an appropriate place for it?

> [...]
> > +     /**
> > +      * @core: Core on which @thread was executing when it registered
> > +      * its interest.  Used for load balancing (see balance.txt).
> > +      */
> > +     int core;
>
> I don't see a 'balance.txt' file in this submission, possibly stray
> reference?

This is a file in the GitHub repo that I hadn't (yet) been including
with the code being upstreamed. I've now added this file (and a couple
of other explanatory .txt files) to the manifest for upstreaming.

> [...]
> > +     /**
> > +      * @pacer_wake_time: time (in sched_clock units) when the pacer last
> > +      * woke up (if the pacer is running) or 0 if the pacer is sleeping.
> > +      */
> > +     __u64 pacer_wake_time;
>
> why do you use the '__' variant here? this is not uapi, you should use
> the plain u64/u32 (more occurrences below).

Sorry, newbie mistake (I wasn't aware of the difference). I will fix everywhere.

> [...]
> > +     /**
> > +      * @prev_default_port: The most recent port number assigned from
> > +      * the range of default ports.
> > +      */
> > +     __u16 prev_default_port __aligned(L1_CACHE_BYTES);
>
> I think the idiomatic way to express the above is to use:
>
>         u16 prev_default_port ____cacheline_aligned;
>
> or
>
>         u16 prev_default_port ____cacheline_aligned_in_smp;
>
> more similar occourrences below.

I will fix everywhere.

Thanks for the comments.


-John-