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

[David Howells]

Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:

> We have a macro which must only have a single usage in any particular
> kernel build (and nothing to detect a violation of that).

That's not true.  It's a macro containing a _static_ local variable, therefore
the macro may be used multiple times, and each time it's used the compiler
will allocate a new piece of storage.

> It apparently tries to avoid races via ordering tricks, as long
> as it is called with sufficient frequency.  But nothing guarantees
> that it _is_ called sufficiently frequently?

The comment attached to it clearly states this restriction.  Therefore the
caller must guarantee it.  That is something Mathieu's code and my code must
deal with, not Nicolas's.

> There is absolutely no reason why the first two of these quite bad things
> needed to be done.  In fact there is no reason why it needed to be
> implemented as a macro at all.

There's a very good reason to implement it as either a macro or an inline
function: it's faster.  Moving the whole thing out of line would impose an
additional function call overhead - with a 64-bit return value on 32-bit
platforms.  For my case - sched_clock() - I'm willing to burn a bit of extra
space to get the extra speed.

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

I'd be quite happy if it was:

	static inline u64 cnt32_to_63(u32 cnt_lo, u32 *__m_cnt_hi)
	{
		union cnt32_to_63 __x;
		__x.hi = *__m_cnt_hi;
		__x.lo = cnt_lo;
		if (unlikely((s32)(__x.hi ^ __x.lo) < 0))
			*__m_cnt_hi =
				__x.hi = (__x.hi ^ 0x80000000) + (__x.hi >> 31);
		return __x.val;
	}

I imagine this would compile pretty much the same as the macro.  I think it
would make it more obvious about the independence of the storage.

Alternatively, perhaps Nicolas just needs to mention this in the comment more
clearly.

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