On Mon, 2019-01-07 at 16:32 -0500, bfields@xxxxxxxxxxxx wrote:
> On Fri, Jan 04, 2019 at 12:39:12PM -0500, bfields@xxxxxxxxxxxx wrote:
> > I wonder if there's a race here independent of that change:
> >
> > svc_xprt_enqueue() callers all do something like:
> >
> > 1. change some condition
> > 2. call svc_xprt_enqueue() to check whether the xprt should
> > now be enqueued.
> >
> > where the conditions are settings of the xpt_flags, or socket
> > wspace, or
> > xpt_nr_rqsts.
> >
> > In theory if we miss some concurrent change we're OK because
> > whoever's
> > making that change will then also call svc_xprt_enqueue. But
> > that's not
> > enough; e.g.:
> >
> > task 1 task 2
> > ------ ------
> > set XPT_DATA
> > atomic_dec(xpt_nr_rqsts)
> >
> > check XPT_DATA && check xpt_nr_rqsts
> >
> > check XPT_DATA && check
> > xpt_nr_rqsts
> >
> > If the tasks only see their local changes, then neither see both
> > conditions true, so the socket doesn't get enqueued. (And a
> > request
> > that was ready to be processed will sit around until someone else
> > comes
> > calls svc_xprt_enqueue() on that xprt.)
>
> So maybe we actually need
>
> static bool svc_xprt_has_something_to_do(struct svc_xprt *xprt)
> {
> + mb();
You would at best need a 'smp_rmb()'. There is nothing to gain from
adding a write barrier here, and you don't even need a read barrier in
the non-smp case.
> if (xprt->xpt_flags & ((1<<XPT_CONN)|(1<<XPT_CLOSE)))
> return true;
> if (xprt->xpt_flags & ((1<<XPT_DATA)|(1<<XPT_DEFERRED))) {
>
> Then whichever memory barrier executes second guarantees that the
> following check sees the result of both the XPT_DATA and xpt_nr_rqsts
> changes. I think....
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
