linux-kernel - Re: Udpated sys_membarrier() speedup patch, FYI

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20170731182736.GN3730@linux.vnet.ibm.com>
Date:   Mon, 31 Jul 2017 11:27:36 -0700
From:   "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:     Dave Watson <davejwatson@...com>
Cc:     Avi Kivity <avi@...lladb.com>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        maged michael <maged.michael@...il.com>,
        Andrew Hunter <ahh@...gle.com>, gromer <gromer@...gle.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: Udpated sys_membarrier() speedup patch, FYI

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;
>