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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Wed, 6 Nov 2013 19:24:37 +0100
From:	Peter Zijlstra <peterz@...radead.org>
To:	Vince Weaver <vince@...ter.net>
Cc:	mingo@...nel.org, hpa@...or.com, anton@...ba.org,
	mathieu.desnoyers@...ymtl.ca, linux-kernel@...r.kernel.org,
	michael@...erman.id.au, paulmck@...ux.vnet.ibm.com,
	benh@...nel.crashing.org, fweisbec@...il.com, VICTORK@...ibm.com,
	tglx@...utronix.de, oleg@...hat.com, mikey@...ling.org,
	linux-tip-commits@...r.kernel.org
Subject: Re: [tip:perf/core] tools/perf: Add required memory barriers

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists