netdev - Re: [PATCH net-next v15 12/15] net: homa: create homa

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: <CAGXJAmzngowSPJOx1Dg9=on++HX7E_bYVNv_Fy6GLQUSb-_BRw@mail.gmail.com>
Date: Mon, 1 Sep 2025 15:12:31 -0700
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 v15 12/15] net: homa: create homa_incoming.c

On Tue, Aug 26, 2025 at 5:05 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> On 8/18/25 10:55 PM, John Ousterhout wrote:
> > +/**
> > + * homa_dispatch_pkts() - Top-level function that processes a batch of packets,
> > + * all related to the same RPC.
> > + * @skb:       First packet in the batch, linked through skb->next.
> > + */
> > +void homa_dispatch_pkts(struct sk_buff *skb)
> > +{
> > +#define MAX_ACKS 10
> > +     const struct in6_addr saddr = skb_canonical_ipv6_saddr(skb);
> > +     struct homa_data_hdr *h = (struct homa_data_hdr *)skb->data;
> > +     u64 id = homa_local_id(h->common.sender_id);
> > +     int dport = ntohs(h->common.dport);
> > +
> > +     /* Used to collect acks from data packets so we can process them
> > +      * all at the end (can't process them inline because that may
> > +      * require locking conflicting RPCs). If we run out of space just
> > +      * ignore the extra acks; they'll be regenerated later through the
> > +      * explicit mechanism.
> > +      */
> > +     struct homa_ack acks[MAX_ACKS];
> > +     struct homa_rpc *rpc = NULL;
> > +     struct homa_sock *hsk;
> > +     struct homa_net *hnet;
> > +     struct sk_buff *next;
> > +     int num_acks = 0;
>
> No black lines in the variable declaration section, and the stack usage
> feel a bit too high.

I have eliminated "acks" and "num_acks" (there's a cleaner way to
handle acks now that RPCs have real reference counts).

> > +     /* Each iteration through the following loop processes one packet. */
> > +     for (; skb; skb = next) {
> > +             h = (struct homa_data_hdr *)skb->data;
> > +             next = skb->next;
> > +
> > +             /* Relinquish the RPC lock temporarily if it's needed
> > +              * elsewhere.
> > +              */
> > +             if (rpc) {
> > +                     int flags = atomic_read(&rpc->flags);
> > +
> > +                     if (flags & APP_NEEDS_LOCK) {
> > +                             homa_rpc_unlock(rpc);
> > +
> > +                             /* This short spin is needed to ensure that the
> > +                              * other thread gets the lock before this thread
> > +                              * grabs it again below (the need for this
> > +                              * was confirmed experimentally in 2/2025;
> > +                              * without it, the handoff fails 20-25% of the
> > +                              * time). Furthermore, the call to homa_spin
> > +                              * seems to allow the other thread to acquire
> > +                              * the lock more quickly.
> > +                              */
> > +                             homa_spin(100);
> > +                             homa_rpc_lock(rpc);
>
> This can still fail due to a number of reasons, e.g. if multiple threads
> are spinning on the rpc lock, or in fully preemptable kernels.

Yes, but that's not a problem; working most of the time gets most of
the benefit.

> You need to either ensure that:
> - the loop works just fine even if the handover fails with high

I've already done this: earlier versions of Homa had no handover at
all and the system worked fine except that tail latency was higher.

-John-