Re: [RFC patch 08/18] cnt32_to_63 should use smp_rmb()

[Nicolas Pitre]

On Fri, 7 Nov 2008, Andrew Morton wrote:

> On Fri, 07 Nov 2008 10:01:01 -0500 (EST) Nicolas Pitre <nico@xxxxxxx> wrote:
> 
> > On Fri, 7 Nov 2008, David Howells wrote:
> > 
> > > Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> > > 
> > > > As I said in the text which you deleted and ignored, this would be
> > > > better if it was implemented as a C function which requires that the
> > > > caller explicitly pass in a reference to the state storage.
> > 
> > The whole purpose of that thing is to be utterly fast and lightweight.  
> 
> Well I'm glad it wasn't designed to demonstrate tastefulness.

Fast tricks aren't always meant to be beautiful.  That,s why we have 
abstraction layers.

> btw, do you know how damned irritating and frustrating it is for a code
> reviewer to have his comments deliberately ignored and deleted in
> replies?

Do you know how irritating and frustrating it is when reviewers don't 
care reading the damn comments along with the code?  Maybe it wasn't the 
case, but you gave the impression of jumping to conclusion without even 
bothering about the associated explanation in the code until I directed 
you at it.

> > Having an out of line C call would trash the major advantage of this 
> > code.
> 
> Not really.

Your opinion.

> > > I imagine this would compile pretty much the same as the macro.
> > 
> > Depends.  As everybody has noticed now, the read ordering is important, 
> > and if gcc decides to not inline this 
> 
> If gcc did that then it would need to generate static instances of
> inlined functions within individual compilation units.  It would be a
> disaster for the kernel.  For a start, functions which are "inlined" in kernel
> modules wouldn't be able to access their static storage and modprobing
> them would fail.

That doesn't mean that access ordering is preserved within the function, 
unless the interface is pointer based, but that won't work if the 
counter is accessed through some special instruction rather than a 
memory location.

> > for whatever reason then the 
> > ordering is lost.
> 
> Uninlining won't affect any ordering I can see.

See above.

> >  This is why this was a macro to start with.
> > 
> > > I think it
> > > would make it more obvious about the independence of the storage.
> > 
> > I don't think having the associated storage be outside the macro make 
> > any sense either.  There is simply no valid reason for having it shared 
> > between multiple invokations of the macro, as well as making its 
> > interface more complex for no gain.
> 
> oh god.

Thank you.  ;-)

> > > 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?
> > 
> 
> Does mn10300's get_cycles() really count backwards?  The first two
> callsites I looked at (crypto/tcrypt.c and fs/ext4/mballoc.c) assume
> that it is an upcounter.

I know nothing about mn10300.


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