Re: [PATCH v3 1/2] x86: simplify non‑atomic bitops

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 26/03/15 08:00, Jan Beulich wrote:
>>>> On 25.03.15 at 18:49, <andrew.cooper3@citrix.com> wrote:
>> On 20/03/15 14:53, Jan Beulich wrote:
>>> - being non-atomic, their pointer arguments shouldn't be volatile-
>>>     qualified
>>> - their (half fake) memory operands can be a single "+m" instead of
>>>     being both an output and an input
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> After further consideration, would it not be better to change the
>> non-atomic variants to being straight C.
>>
>> e.g.
>>
>> static inline void __set_bit(int nr, void *_addr)
>> {
>>       int *addr = _addr;
>>       addr[nr / sizeof(int)] |= (1U << (nr % sizeof(int)));
>> }
>>
>> This would drop the memory clobber from the asm statement and allow the
>> compiler to optimise repeated __set_bit() calls to the same word into a
>> single action.
> I wouldn't want to do this in this patch - a similar change that I
> proposed years ago for Linux (not eliminating the asm(), but
> dropping the clobbers) caused not really understood regressions and
> hence needed to be reverted.

Presumably some code was lacking barrier()s

>   We could still try whether this works in
> Xen, but in another patch. Apart from that I doubt the operation
> above would reliably get converted to BTS by the compiler
> (independent of version).

Ok - lets leave that for v2.

Both patches Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
>> (On a separate note, I feel that all of these operations should be
>> acting on unsigned rather than signed ints, but that applies to all of
>> these operations, not just the non-atomic ones)
> That makes zero difference for these ops - all that really matters
> is the width. In the C version above however I agree that using
> "unsigned int" would be more natural.
>
> Jan
>