linux-kernel - Re: [RFC patch 08/18] cnt32_to_63 should use smp

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: <8106.1226074054@redhat.com>
Date:	Fri, 07 Nov 2008 16:07:34 +0000
From:	David Howells <dhowells@...hat.com>
To:	Nicolas Pitre <nico@....org>
Cc:	dhowells@...hat.com, Andrew Morton <akpm@...ux-foundation.org>,
	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Ingo Molnar <mingo@...e.hu>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	linux-kernel@...r.kernel.org, Ralf Baechle <ralf@...ux-mips.org>,
	benh@...nel.crashing.org, paulus@...ba.org,
	David Miller <davem@...emloft.net>,
	Ingo Molnar <mingo@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Steven Rostedt <rostedt@...dmis.org>,
	linux-arch@...r.kernel.org
Subject: Re: [RFC patch 08/18] cnt32_to_63 should use smp_rmb()

Nicolas Pitre <nico@....org> wrote:

> The whole purpose of that thing is to be utterly fast and lightweight.  
> Having an out of line C call would trash the major advantage of this 
> code.

No argument there.

> > I imagine this would compile pretty much the same as the macro.

Having said that, I realise it's wrong.  The macro potentially takes a h/w
read operation (cnt_lo) and does it at a place of its choosing - which the
compiler may not be permitted to move if cnt_lo resolves to a bit of volatile
inline asm with memory constraints.  Converting it to an inline function
forces cnt_lo to be resolved first.

> Depends.  As everybody has noticed now, the read ordering is important, 
> and if gcc decides to not inline this for whatever reason then the 
> ordering is lost.  This is why this was a macro to start with.

Good point.  I wonder if you should've put a compiler barrier in there to at
least make the point.

> I don't think having the associated storage be outside the macro make any
> sense either.

It can have a comment attached to it to say what it represents.  On the other
hand, it'd probably need further comments attaching to it to fend off people
who start thinking they can make use of this variable in other ways...

> > Alternatively, perhaps Nicolas just needs to mention this in the comment
> > more clearly.
> 
> I wrote that code so to me it is cristal clear already.  Any suggestions 
> as to how this could be improved?

It ought to be, but clearly it isn't.  Sometimes the obvious is all too easy to
overlook.  I'll think about it, but perhaps something like:

 * This macro uses a static internal variable to retain the upper counter.
 * This has two consequences: firstly, it may be used in multiple ways by
 * different callers for different things without interference; and secondly,
 * each caller will get its own, independent counter, and so an out of line
 * wrapper must be used if multiple callers want to use the same pair of
 * counters.

It's a bit heavy-handed, but...

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