Message-ID: <CAL+tcoAOwxqQVrj3P7AcaB0FOtJZNwSM9iwwpFtic_RGrU8B2A@mail.gmail.com>
Date: Tue, 14 Jan 2025 08:20:14 +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 Tue, Jan 14, 2025 at 1:12 AM John Ousterhout <ouster@...stanford.edu> wrote:
>
> 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

I didn't suggest specifically as I still need some time to take a deep
look into the implementation, only having offered some information
that you may be interested in. I will get back to you on this point
later.

> 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