Date: Mon, 31 Jul 2017 11:27:36 -0700
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
To: Dave Watson <davejwatson@fb.com>
Cc: Avi Kivity <avi@scylladb.com>,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        maged michael <maged.michael@gmail.com>,
        Andrew Hunter <ahh@google.com>, gromer <gromer@google.com>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Peter Zijlstra <peterz@infradead.org>
Subject: Re: Udpated sys_membarrier() speedup patch, FYI
Reply-To: paulmck@linux.vnet.ibm.com
References: <20170727181250.GA20183@linux.vnet.ibm.com>
 <20170731180019.GA74975@dhcp-172-20-173-153.dhcp.thefacebook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170731180019.GA74975@dhcp-172-20-173-153.dhcp.thefacebook.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Message-Id: <20170731182736.GN3730@linux.vnet.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2451
Lines: 66

On Mon, Jul 31, 2017 at 11:00:19AM -0700, Dave Watson wrote:
> Hi Paul, 
> 
> Thanks for looking at this again!
> 
> On 07/27/17 11:12 AM, Paul E. McKenney wrote:
> > Hello!
> > 
> > But my main question is whether the throttling shown below is acceptable
> > for your use cases, namely only one expedited sys_membarrier() permitted
> > per scheduling-clock period (1 millisecond on many platforms), with any
> > excess being silently converted to non-expedited form.  The reason for
> > the throttling is concerns about DoS attacks based on user code with a
> > tight loop invoking this system call.
> 
> We've been using sys_membarrier for the last year or so in a handful
> of places with no issues.  Recently we made it an option in our hazard
> pointers implementation, giving us something with performance between
> hazard pointers and RCU:
> 
> https://github.com/facebook/folly/blob/master/folly/experimental/hazptr/hazptr-impl.h#L555
> 
> Currently hazard pointers tries to free retired memory the same thread
> that did the retire(), so the latency spiked for workloads that were
> retire() heavy.   For the moment we dropped back to using mprotect
> hacks.
> 
> I've tested Mathieu's v4 patch, it works great.  We currently don't
> have any cases where we need SHARED. 

Very good!!!  May I have your Tested-by?  (Or the Tested-by of whoever
did the testing, as the case may be?)

> I also tested the rate-limited version, while better than the current
> non-EXPEDITED SHARED version, we still hit the slow path pretty often.
> I agree with other commenters that returning an error code instead of
> silently slowing down might be better.

If I need to fall back to the rate-limited version, I will add some sort
of error code capability.  For the moment, I am hoping that Mathieu's
patch proves acceptable, but will fall back to the rate-limited version
if some fatal problem arises.

> > +	case MEMBARRIER_CMD_SHARED_EXPEDITED:
> > +		if (num_online_cpus() > 1) {
> > +			static unsigned long lastexp;
> > +			unsigned long j;
> > +
> > +			j = jiffies;
> > +			if (READ_ONCE(lastexp) == j) {
> > +				synchronize_sched();
> > +				WRITE_ONCE(lastexp, j);
> 
> It looks like this update of lastexp should be in the other branch?

Good catch, fixed.  It is on branch paulmck.2017.08.01a, and will
hopefully not be needed.

							Thanx, Paul

> > +			} else {
> > +				synchronize_sched_expedited();
> > +			}
> > +		}
> > +		return 0;
>