Re: Udpated sys_membarrier() speedup patch, FYI

[Dave Watson <davejwatson@fb.com>]

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. 

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.

> +	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?

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