Re: [PATCH] Add adaptive elision to rwlocks

[Roland McGrath <roland at hack dot frob dot com>]

Please preserve the two blank lines between #include's and code.

Use <> rather than "" for #include of any file that might be sysdeps (elide.h).

elide.h is missing the standard header comment (explanatory line plus
license header).

Use uint8_t rather than 'unsigned char' for a one-byte counter
(perhaps not in the installed header if it's one that doesn't/can't
rely on <stdint.h>, but certainly in the rest of the code).

Never use bare 'unsigned', always 'unsigned int'.  (For the type
returned by _xbegin et al, perhaps uint32_t would be better, but
rtmintin.h uses 'unsigned int', so that is probably right.)

GNU style for function definitions is:
	type
	name (argtype ...)
not:
	type name(argtype ...)

Use bool rather than int (and #include <stdbool.h>) for Booleans
(e.g. elision_adapt).

Use C99 style, defining local variables only at the site of first
setting (including inside the 'for' initializer clause).

There is no need for braces around an if body that is a single
statement, even if that statement is for or while.  (It's not a real
problem to have the extra braces if you strongly prefer them, but
don't expect they will necessarily be preserved.)

__rwelision is an unsigned type, yet ELIDE_LOCK tests with <= 0.
Make it == 0.

In elision_adapt you have two cases of:

	if (var1 != var2)
	  va1 = var2;

If it is meaningfully worthwhile to write that instead of simply:

	var1 = var2;

then it warrants a comment explaining why.

I see no reason why ELIDE_*LOCK must be macros rather than functions.
If you really know better than the compiler it's important that they
be inlined, then use always_inline.  Give each a comment describing
its interface contract.  (AFAICT elision_adapt should just be rolled
directly into elide_lock.)  

Oh, I see, you are passing in an expression to be evaluated inside.
Can you try passing in the address of a nested function marked as
always_inline and see if the compiler manages to inline everything?
Hmm.  But it's the same expression every time!  So why the false
generality?  They could just be inline functions taking the
pthread_rwlock_t pointer directly.


Thanks,
Roland