Re: [PATCH 0/7] preempt_count rework -v2

["H. Peter Anvin" <hpa@zytor.com>]

On 09/10/2013 02:43 PM, Linus Torvalds wrote:
> On Tue, Sep 10, 2013 at 2:25 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>>
>> Here's one that builds and boots on kvm until wanting to mount root.
>>
>> I'm not entirely sure on the "ir" vs "er" thing and atomic64_t and
>> local_t are inconsistent wrt that so I'm too.
> 
> "i" is "any constant", while "e" is "32-bit signed constant".
> 
> And I think all of the 64-bit ones should probably be "e", because
> afaik there is no way to add a 64-bit constant directly to memory (you
> have to load it into a register first).
> 
> Of course, in reality, the constant is always just 1 or -1 or
> something like that, so nobody will ever notice the incorrect case...
> 
> And it doesn't matter for the 32-bit cases, obviously, but we could
> just make them all be "e" for simplicity.
> 
> That said, looking at your patch, I get the *very* strong feeling that
> we could make a macro that does all the repetitions for us, and then
> have a
> 
>   GENERATE_RMW(atomic_sub_and_test, LOCK_PREFIX "subl", "e", "")
>   GENERATE_RMW(atomic_dec_and_test, LOCK_PREFIX "decl", "e", "")
>   ..
>   GENERATE_RMW(atomic_add_negative, LOCK_PREFIX "addl", "s", "")
> 
>   GENERATE_RMW(local_sub_and_test, "subl", "e", __percpu_prefix)
>   ...
> 
> etc.
> 
> I'm sure the macro would be nasty as hell (and I bet it needs a few
> more arguments), but then we'd avoid the repetition..
> 

Actually, the right thing here really is "er" (which I think you meant,
but just to make it clear.)  Why?  Even if the value is representable as
a signed immediate, if gcc already happens to have it in a register it
will be better to use the register.

"e" doesn't work on versions of gcc older than the first x86-64 release,
but we don't care about that anymore.

A final good question is if we should encapsulate the add/inc and
sub/dec into a single function; one could easily do somethin glike:

static inline int atomic_sub_and_test(int i, atomic_t *)
{
	if (__builtin_constant_p(i) && i == 1)
		/* Use incl */
	else if (__builtin_constant_p(i) && i == -1)
		/* Use decl */
	else
		/* Use addl */
}

	-hpa