linux-kernel - Re: [PATCH] trace: Check for waiting readers before calling wakeup()

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <BANLkTi=Y9zRnzouBKq9NnXZE5+FmXAWyfg@mail.gmail.com>
Date:	Wed, 4 May 2011 23:05:41 -0700
From:	Vaibhav Nagarnaik <vnagarnaik@...gle.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Frederic Weisbecker <fweisbec@...il.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Ingo Molnar <mingo@...hat.com>,
	Michael Rubin <mrubin@...gle.com>,
	David Sharp <dhsharp@...gle.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] trace: Check for waiting readers before calling wakeup()

On Wed, May 4, 2011 at 8:53 PM, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
> On Wed, May 4, 2011 at 2:26 PM, Frederic Weisbecker <fweisbec@...il.com> wrote:
>> On Wed, May 04, 2011 at 02:21:28PM -0700, Vaibhav Nagarnaik wrote:
>>> Steven, Frederic
>>>
>>> Does this patch look ok?
>>>
>>> Vaibhav Nagarnaik
>>>
>>
>> No problem for me. I'll let Steve give the last word.
>
> Almost every time somebody does this optimization, there's a race.
>
> The thing is, because waitqueue_active() implies no barriers, if there
> is somebody adding itself to the waitqueue just at the same time as
> somebody is waking things up, the waiter may lose wakeup events -
> there is no memory barriers to guarantee that the waker will see
> either the waiter, or the waiter will see the event that it waited
> for.
>
> I dunno. It's sometimes safe, but it's equally often not safe unless
> there is some other locking around the thing. So I'd be a bit nervous.
> The tracer people would need to do some serious thinking about it.
>
>                   Linus
>

Thanks for looking at the patch and weighing in.

The problem I was trying to solve here is the contention when trying
to wake up a process. The process calling wakeup() has to take a
spinlock. When concurrent processes are getting traced, they contend
on locking the spinlock and cause large tracing latency. Having safe
locking around this code would defeat the purpose of this patch.

One could argue that if the trace reader does not get a wakeup() event
due to race, it will get it when the tracepoint is hit the next time.

Also, it seems the current tracers cannot rely on polling on the trace
file. The wakeup() is called only when the syscalls get traced. If any
other events are traced, the trace logging function (in
include/linux/syscalls.h) does not call wakeup(). This was done due to
hard lockup in the kernel if sched events were being traced.

I am new to this area, so Frederic and/or Steven could address your
concerns better.

Vaibhav Nagarnaik
--
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/