Message-ID: <CAGXJAmzCx6NJGeHrW+CB6+Uc0_DDBMJRMzfCbCs3FNGcdBtX3w@mail.gmail.com>
Date: Mon, 13 Jan 2025 09:11:51 -0800
From: John Ousterhout <ouster@...stanford.edu>
To: Jason Xing <kerneljasonxing@...il.com>
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 12:31 AM Jason Xing <kerneljasonxing@...il.com> wrote:
>
> 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.

I'm not quite sure what you are recommending in your comments above
(i.e. should Homa try to use SOCK_DEAD or stick with the current
approach of having a separate Homa-specific flag indicating that the
socket has been shutdown). Based on what I've heard so far I still
prefer having a separate flag because it eliminates any possibility of
confusion between Homa's use of SOCK_DEAD and the rest of Linux's use
of it.

I see now that SOCK_DEAD gets set (indirectly) by Homa when homa_close
calls sk_common_release. However, this only happens when the fd is
closed; hsk->shutdown gets set by homa_sock_shutdown, which can happen
before the fd is closed. This seems to argue for a separate state bit
for Homa's shutdown.

-John-

Powered by blists - more mailing lists