linux-kernel - Re: [PATCH RFC] x86: avoid atomic operation in test_and_set_bit

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]

Date:	Fri, 25 Mar 2011 16:47:19 +0000
From:	"Jan Beulich" <JBeulich@...ell.com>
To:	"Linus Torvalds" <torvalds@...ux-foundation.org>
Cc:	"Borislav Petkov" <bp@...64.org>,
	"Peter Zijlstra" <a.p.zijlstra@...llo.nl>,
	"Ingo Molnar" <mingo@...e.hu>, "Nick Piggin" <npiggin@...nel.dk>,
	"x86@...nel.org" <x86@...nel.org>,
	"Thomas Gleixner" <tglx@...utronix.de>,
	"Andrew Morton" <akpm@...ux-foundation.org>,
	"Arnaldo Carvalho de Melo" <acme@...hat.com>,
	"Ingo Molnar" <mingo@...hat.com>, "Jack Steiner" <steiner@....com>,
	<tee@....com>, "Nikanth Karthikesan" <knikanth@...e.de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH RFC] x86: avoid atomic operation in
	 test_and_set_bit_lock if possible

>>> On 25.03.11 at 17:29, Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> On Fri, Mar 25, 2011 at 3:06 AM, Jan Beulich <JBeulich@...ell.com> wrote:
>>
>> The problem was observed with __lock_page() (in a variant not
>> upstream for reasons not known to me), and prefixing e.g.
>> trylock_page() with an extra PageLocked() check yielded the
>> below quoted improvements.
> 
> Ok. __lock_page() _definitely_ should do the test_bit() thing first,
> because it's normally called from lock_page() that has already tested
> the bit.
> 
> But it already seems to do that, so I'm wondering what your variant is.

lock_page() does, but here it's really about __lock_page().

We've got a patch from Nick Piggin in the tree that aims at
speeding up unlock_page, which turns __lock_page() (on
2.6.32) into

void __lock_page(struct page *page)
{
	wait_queue_head_t *wq = page_waitqueue(page);
	DEFINE_WAIT_BIT(wait, &page->flags, PG_locked);

	do {
		prepare_to_wait(wq, &wait.wait, TASK_UNINTERRUPTIBLE);
		SetPageWaiters(page);
		if (likely(PageLocked(page)))
			sync_page(page);
	} while (!trylock_page(page));
	finish_wait(wq, &wait.wait);
}

(No, I do not know why the patch isn't upstream, but Nick is on Cc,
so maybe he can tell.)

> I'm also a bit surprised that lock_page() is that hot (unless your
> _lock_page() variant is simply too broken and ends up spinning?).
> Maybe we have some path that takes the page lock unnecessarily? What's
> the load?

So yes, there is spinning involved. A first improvement from Jack
was to make the SetPageWaiters() (introduced by that patch)
conditional upon the bit not already being set.

But that wasn't yielding the desired improvement, hence they
added a PageLocked() || ... in front of the trylock_page(), which
in turn raised the question why trylock_page() wouldn't do this
generally.

Jan

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