Message-ID: <CAL+tcoCOSk2ezZ+OnsKBZc_JcO_U01X1q3KmTd6WhObuzbuzsA@mail.gmail.com>
Date: Sat, 11 Jan 2025 16:30:32 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: John Ousterhout <ouster@...stanford.edu>
Cc: "D. Wythe" <alibuda@...ux.alibaba.com>, netdev@...r.kernel.org, pabeni@...hat.com, 
	edumazet@...gle.com, horms@...nel.org, kuba@...nel.org
Subject: Re: [PATCH net-next v5 07/12] net: homa: create homa_sock.h and homa_sock.c

On Sat, Jan 11, 2025 at 8:20 AM John Ousterhout <ouster@...stanford.edu> wrote:
>
> On Fri, Jan 10, 2025 at 1:25 AM D. Wythe <alibuda@...ux.alibaba.com> wrote:
> >
> > > +void homa_sock_unlink(struct homa_sock *hsk)
> > > +{
> > > +     struct homa_socktab *socktab = hsk->homa->port_map;
> > > +     struct homa_socktab_scan *scan;
> > > +
> > > +     /* If any scans refer to this socket, advance them to refer to
> > > +      * the next socket instead.
> > > +      */
> > > +     spin_lock_bh(&socktab->write_lock);
> > > +     list_for_each_entry(scan, &socktab->active_scans, scan_links) {
> > > +             if (!scan->next || scan->next->sock != hsk)
> > > +                     continue;
> > > +             scan->next = (struct homa_socktab_links *)
> > > +                             rcu_dereference(hlist_next_rcu(&scan->next->hash_links));
> > > +     }
> >
> > I can't get it.. Why not just mark this sock as unavailable and skip it
> > when the iterator accesses it ?
> >
> > The iterator was used under rcu and given that your sock has the
> > SOCK_RCU_FREE flag set, it appears that there should be no concerns
> > regarding dangling pointers.
>
> The RCU lock needn't be held for the entire lifetime of an iterator,
> but rather only when certain functions are invoked, such as
> homa_socktab_next. Thus it's possible for a socket to be reclaimed and
> freed while a scan is in progress. This is described in the comments
> for homa_socktab_start_scan. This behavior is necessary because of
> homa_timer, which needs to call schedule in the middle of a scan and
> that can't be done without releasing the RCU lock. I don't like this
> complexity but I haven't been able to find a better alternative.
>
> > > +     hsk->shutdown = true;
> >
> > From the actual usage of the shutdown member, I think you should use
> > sock_set_flag(SOCK_DEAD), and to check it with sock_flag(SOCK_DEAD).
>
> I wasn't aware of SOCK_DEAD until your email. After poking around a
> bit to learn more about SOCK_DEAD, I am nervous about following your
> advice. I'm still not certain exactly when SOCK_DEAD is set or who is
> allowed to set it. The best information I could find was from ChatGPT
> which says this:
>
> "The SOCK_DEAD flag indicates that the socket is no longer referenced
> by any user-space file descriptors or kernel entities. Essentially,
> the socket is considered "dead" and ready to be cleaned up."

Well, I'm surprised that the GPT is becoming more and more intelligent...

The above is correct as you can see from this call trace
(__tcp_close()->sk_orphan()). Let me set TCP as an example, when the
user decides to close a socket or accidently kill/exit the process,
the socket would enter into __tcp_close(), which indicates that this
socket has no longer relationship with its owner (application).

>
> If ChatGPT isn't hallucinating, this would suggest that Homa shouldn't
> set SOCK_DEAD, since the conditions above might not yet be true when
> homa_sock_shutdown is invoked.

Introducing a common usage about SOCK_DEAD might be a good choice. But
if it's not that easy to implement, I think we can use the internal
destruction mechanism instead like you did.

>
> Moreover, I'm concerned that some other entity might set SOCK_DEAD
> before homa_sock_shutdown is invoked, in which case homa_sock_shutdown
> would not cleanup the socket properly.

No need to worry about that. If it happens, it usually means there is
a bug somewhere and then we will fix it.

Thanks,
Jason

>
> Thus, it seems safest to me for Homa to have its own shutdown flag.
>
> Let me know if you still think Homa should use SOCK_DEAD.
>
> > > +
> > > +     while (!list_empty(&hsk->dead_rpcs))
> > > +             homa_rpc_reap(hsk, 1000);
> >
> > I take a quick look at homa_rpc_reap, although there is no possibility
> > of an infinite loop founded currently, it still raises concerns.
> >
> > It might be better to let homa_rpc_reap() handle this kind of actions by itself.
> > For example, code like that:
> >
> > homa_rpc_reap(hsk, 0, flags=RPC_FORCE_REAP|RPC_REAP_ALL);
> >
> > In this way, anyone making modifications to homa_rpc_reap() in the future will
> > at least be aware that there is such a case that needs to be handled well.
>
> I have changed the API for homa_rpc_reap to this:
>
> int homa_rpc_reap(struct homa_sock *hsk, bool reap_all)
>
> The caller can no longer specify a count. When reap_all isn't
> specified, homa_rpc_reap determines for itself what represents a
> "small amount of work" to perform; no need for the caller to figure
> this out.
>
> -John-
>

Powered by blists - more mailing lists