Re: [tip:perf/core] tools/perf: Add required memory barriers

[Peter Zijlstra]

On Wed, Nov 06, 2013 at 12:31:53PM -0500, Vince Weaver wrote:
> On Wed, 6 Nov 2013, Peter Zijlstra wrote:
> 
> > On Wed, Nov 06, 2013 at 03:44:56PM +0100, Peter Zijlstra wrote:
> > > long head = ((__atomic long)pc->data_head).load(memory_order_acquire);
> > > 
> > > coupled with:
> > > 
> > > ((__atomic long)pc->data_tail).store(tail, memory_order_release);
> > > 
> > > might be the 'right' and proper C11 incantations to avoid having to
> > > touch kernel macros; but would obviously require a recent compiler.
> > > 
> > > Barring that, I think we're stuck with:
> > > 
> > > long head = ACCESS_ONCE(pc->data_head);
> > > smp_rmb();
> > > 
> > > ...
> > > 
> > > smp_mb();
> > > pc->data_tail = tail;
> > > 
> > > And using the right asm goo for the barriers. That said, all these asm
> > > barriers should include a compiler barriers (memory clobber) which
> > > _should_ avoid the worst compiler trickery -- although I don't think it
> > > completely obviates the need for ACCESS_ONCE() -- uncertain there.
> > 
> > http://software.intel.com/en-us/articles/single-producer-single-consumer-queue/
> > 
> > There's one for icc on x86.
> > 
> 
> I think the problem here is this really isn't a good interface.

Its _so_ common Intel put it on a website ;-) This is a fairly well
documented 'problem'.

> Most users just want the most recent batch of samples.  Something like
> 
>     char buffer[4096];
>     int count;
> 
>     do {
>        count=perf_read_sample_buffer(buffer,4096);
>        process_samples(buffer);
>     } while(count);
> 
> where perf_read_sample_buffer() is a syscall that just copies the current 
> valid samples to userspace.
> 
> Yes, this is inefficient (requires an extra copy of the values) but the 
> kernel then could handle all the SMP/multithread/barrier/locking issues.
> 
> How much overhead is really introduced by making a copy?

It would make the current perf-record like thing do 2 copies; one into
userspace, and one back into the kernel for write().

Also, we've (unfortunately) already used the read() implementation of
the perf-fd and I'm fairly sure people will not like adding a special
purpose read-like syscall just for this.

That said, I've no idea how expensive it is, not having actually done
it. I do know people were trying to get rid of the one copy we currently
already do.

> Requiring the user of a kernel interface to have a deep knowledge of 
> optimizing compilers, barriers, and CPU memory models is just asking for 
> trouble.

It shouldn't be all that hard to put this in a (lgpl) library others can
link to -- that way you can build it once (using GCC).

We'd basically need to lift the proposed smp_load_acquire() and
smp_store_release() into userspace for all relevant architectures and
then have something like:

unsigned long perf_read_sample_buffer(void *mmap, long mmap_size, void *dst, long len)
{
	struct perf_event_mmap_page *pc = mmap;
	void *data = mmap + page_size;
	unsigned long data_size = mmap_size - page_size; /* should be 2^n */
	unsigned long tail, head, size, copied = 0;

	tail = pc->data_tail;
	head = smp_load_acquire(&pc->data_head);

	size = (head - tail) & (data_size - 1);

	while (len && size) {
		unsigned long offset = tail & (data_size - 1);
		unsigned long bytes = min(len, data_size - offset);

		memcpy(data + offset, dst, bytes);

		dst += bytes;
		tail += bytes;
		copied += bytes;
		size -= bytes;
		len -= bytes;
	}

	smp_store_release(&pc->data_tail, tail);

	return copied;
}

And presto!

> Especially as this all needs to get documented in the manpage and I'm not 
> sure that's possible in a sane fashion.

Given that this is a fairly well documented problem that shouldn't be
too hard.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/