Message-ID: <CAGXJAmwyp6tSO4KT_NSHKHSnUn-GSzSN=ucfjnBuXbg8uiw2pg@mail.gmail.com>
Date: Sun, 26 Jan 2025 21:22:38 -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 05/12] net: homa: create homa_rpc.h and homa_rpc.c

On Thu, Jan 23, 2025 at 6:30 AM Paolo Abeni <pabeni@...hat.com> wrote:
> ...
> How many RPCs should concurrently exist in a real server? with 1024
> buckets there could be a lot of them on each/some list and linear search
> could be very expansive. And this happens with BH disabled.

Server RPCs tend to be short-lived, so my best guess is that the
number of concurrent server RPCs will be relatively small (maybe a few
hundred?). But this is just a guess: I won't know for sure until I can
measure Homa in production use. If the number of concurrent RPCs turns
out to be huge then we'll have to find a different solution.

> > +
> > +     /* Initialize fields that don't require the socket lock. */
> > +     srpc = kmalloc(sizeof(*srpc), GFP_ATOMIC);
>
> You could do the allocation outside the bucket lock, too and avoid the
> ATOMIC flag.

In many cases this function will return an existing RPC so there won't
be any need to allocate; I wouldn't want to pay the allocation
overhead in that case. I could conceivably check the offset in the
packet and pre-allocate if the offset is zero (in this case it's
highly unlikely that there will be an existing RPC). But this is
starting to feel complicated so I'm not sure it's worth doing (and
there are many other places where GFP_ATOMIC is unavoidable, so fixing
just one place may not make much difference). homa_rpc objects are
about 500 bytes, so not super huge. I'm inclined to leave this as is
and consider a more complex approach only if problems arise in
practice.

> > + * homa_rpc_free() - Destructor for homa_rpc; will arrange for all resources
> > + * associated with the RPC to be released (eventually).
> > + * @rpc:  Structure to clean up, or NULL. Must be locked. Its socket must
> > + *        not be locked.
> > + */
> > +void homa_rpc_free(struct homa_rpc *rpc)
> > +     __acquires(&rpc->hsk->lock)
> > +     __releases(&rpc->hsk->lock)
>
> The function name is IMHO misleading. I expect homa_rpc_free() to
> actually free the memory allocated for the rpc argument, including the
> rpc struct itself.

That's a fair point. I have bitten the bullet and renamed it to homa_rpc_end.

> > +                     if (rpc->msgin.length >= 0) {
> > +                             while (1) {
> > +                                     struct sk_buff *skb;
> > +
> > +                                     skb = skb_dequeue(&rpc->msgin.packets);
> > +                                     if (!skb)
> > +                                             break;
> > +                                     kfree_skb(skb);
>
> You can use:
>                                         rx_free+= skb_queue_len(&rpc->msgin.packets);
>                                         skb_queue_purge(&rpc->msgin.packets);

Done.

> > +/**
> > + * homa_find_client_rpc() - Locate client-side information about the RPC that
> > + * a packet belongs to, if there is any. Thread-safe without socket lock.
> > + * @hsk:      Socket via which packet was received.
> > + * @id:       Unique identifier for the RPC.
> > + *
> > + * Return:    A pointer to the homa_rpc for this id, or NULL if none.
> > + *            The RPC will be locked; the caller must eventually unlock it
> > + *            by invoking homa_rpc_unlock.
>
> Why are using this lock schema? It looks like it adds quite a bit of
> complexity. The usual way of handling this kind of hash lookup is do the
> lookup locklessly, under RCU, and eventually add a refcnt to the
> looked-up entity - homa_rpc - to ensure it will not change under the
> hood after the lookup.

I considered using RCU for this, but the time period for RCU
reclamation is too long (10's - 100's of ms, if I recall correctly).
Homa needs to handle a very high rate of RPCs, so this would result in
too much accumulated memory (in particular, skbs don't get reclaimed
until the RPC is reclaimed).

The caller must have a lock on the homa_rpc anyway, so RCU wouldn't
save the overhead of acquiring a lock. The reason for putting the lock
in the hash table instead of the homa_rpc is that this makes RPC
creation/deletion atomic with respect to lookups. The lock was
initially in the homa_rpc, but that led to complex races with hash
table insertion/deletion. This is explained in sync.txt, but of course
you don't have that (yet).

This approach is unusual, but it has worked out really well. Before
implementing this approach I had what seemed like a never-ending
stream of synchronization problems over the socket hash tables; each
"fix" introduced new problems. Once I implemented this, all the
problems went away and the code has been very stable ever since
(several years now).

> > + */
> > +struct homa_rpc *homa_find_client_rpc(struct homa_sock *hsk, __u64 id)
> > +     __acquires(&crpc->bucket->lock)
> > +{
> > +     struct homa_rpc_bucket *bucket = homa_client_rpc_bucket(hsk, id);
> > +     struct homa_rpc *crpc;
> > +
> > +     homa_bucket_lock(bucket, id, __func__);
> > +     hlist_for_each_entry_rcu(crpc, &bucket->rpcs, hash_links) {
>
> Why are you using the RCU variant? I don't see RCU access for rpcs.

I have no idea why that uses RCU (maybe leftover from a long-ago
version that did actually use RCU?). I'll take it out. After seeing
this, I decided to review all of the RCU usage in Homa and I have
found and fixed several other problems and/or unnecessary uses of RCU.

-John-

Powered by blists - more mailing lists