Message-ID: <CAGXJAmwsxBdzgEa5hg6vvgdCU3yvCJhvG7xzAembMVTSeCXGag@mail.gmail.com>
Date: Thu, 19 Dec 2024 09:50:45 -0800
From: John Ousterhout <ouster@...stanford.edu>
To: Edward Cree <ecree.xilinx@...il.com>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH net-next v3 03/12] net: homa: create shared Homa header files

On Wed, Dec 18, 2024 at 4:25 AM Edward Cree <ecree.xilinx@...il.com> wrote:\
>
>
> > This is a lock-free mechanism to hand off a complete message to a
> > receiver thread (which may be polling, though the polling code has
> > been removed from this stripped down patch series). I couldn't find an
> > "atomic pointer" structure, which is why the code uses atomic_long_t
> > (which I agree is a bit ugly).
>
> As far as I can tell, ready_rpc only ever transitions from NULL to
>  populated; once non-NULL, the value is never overwritten by a
>  different pointer.  Is that correct?  If so, I believe you could use
>  a (typed) nonatomic pointer and an atomic flag to indicate "there is
>  an RPC in ready_rpc".

Yep, that's much better; don't know why I didn't think of that. I've
refactored to implement this approach.

> > I'm not
> > sure I understand your comment about not manually sleeping and waking
> > threads from within Homa; is there a particular mechanism for this
> > that you have in mind?
>
> It's not in this patch but homa_rpc_handoff() does a wake_up_process()
>  and home_wait_for_message() does set_current_state(), neither of
>  which ought to be necessary.
> I think wait-queues do what you want (see `struct wait_queue_head` and
>  `wait_event_interruptible` in include/linux/wait.h — it's basically a
>  condvar), or if you want to wake unconditionally then completions
>  (kernel/sched/completion.c, include/linux/completion.h).

It's been a long time since I wrote this part of Homa, but I believe
that I considered wait queues and found that they wouldn't work. The
problem is that different threads might be waiting for partially
overlapping groups of RPCs (e.g. thread A might be waiting for a
specific RPC X, while thread B might be waiting for any client
response, which happens to include X). Wait queues don't work because
they assume that everyone in the queue is waiting for the same stuff,
so any event satisfies any thread. That isn't the case with Homa RPCs.
Thus I had to introduce the homa_interest struct and handle the
wakeups with lower-level mechanisms (which, as I recall, I copied from
the wait queue implementation).
>
> >>> +     interest->request_links.next = LIST_POISON1;
> >>> +     interest->response_links.next = LIST_POISON1;
> >>
> >> Any particular reason why you're opencoding poisoning, rather than
> >>  using the list helpers (which distinguish between a list_head that
> >>  has been inited but never added, so list_empty() returns true, and
> >>  one which has been list_del()ed and thus poisoned)?
> >> It would likely be easier for others to debug any issues that arise
> >>  in Homa if when they see a list_head in an oops or crashdump they
> >>  can relate it to the standard lifecycle.
> >
> > I couldn't find any other way to do this: I want to initialize the
> > links to be the same state as if list_del had been called,
>
> If there's a reason why this is necessary, there should be a comment
>  here explaining why.  I *think*, from poking around the rest of the
>  Homa code, you're using 'next == LIST_POISON1' to signal some kind
>  of state, but if it's just "this interest is not on a list", then
>  list_empty() after INIT_LIST_HEAD() or list_del_init() should work
>  just as well.  (Use list_del_init(), rather than plain list_del(),
>  if the interest is still reachable by some code that may need to
>  tell whether the interest is on a list; otherwise, using list_del()
>  gets the poisoning behaviour that will give a recognisable error if
>  buggy code tries to walk the list anyway.)

Oops. I had somehow overlooked list_del_init. I've switched to using
that, and now I don't need the LIST_POISON stuff (the problem was that
I needed "initialized" and "inserted then removed" states to be the
same). That's much better.

Thanks for all of your comments; they've been very helpful. I look
forward to getting more next year.

-John-

Powered by blists - more mailing lists