Re: [PATCH] SUNRPC: Don't allow compiler optimisation of svc_xprt_release_slot()

[Trond Myklebust <trondmy@hammerspace.com>]

On Mon, 2019-01-07 at 16:32 -0500, bfields@fieldses.org wrote:
> On Fri, Jan 04, 2019 at 12:39:12PM -0500, bfields@fieldses.org 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@hammerspace.com