Re: [PATCH] RFC: powerpc: expose the multi-bit ops that underlie single-bit ops.

[Geoff Thorpe <Geoff.Thorpe@freescale.com>]

Benjamin Herrenschmidt wrote:
> On Thu, 2009-06-18 at 16:30 -0400, Geoff Thorpe wrote:
>> I've left the volatile qualifier in the generated API because I didn't
>> feel so comfortable changing APIs, but I also added the "memory" clobber
>> for all cases - whereas it seems the existing set_bits(), clear_bits(),
>> [...] functions didn't declare this... Do you see any issue with having
>> the 'volatile' in the prototype as well as the clobber in the asm?
>>
>> Actually, might as well just respond to the new patch instead... :-) Thx.
> 
> I think the story with the memory clobber is that it depends whether
> we consider the functions as ordering accesses or not (ie, can
> potentially be used with lock/unlock semantics).
> 
> The general rule is that those who don't return anything don't need
> to have those semantics, and thus could only be advertised as clobbering
> p[word] -but- there are issues there. For example, despite the
> (relatively new) official _lock/_unlock variants, there's still code
> that abuses constructs like test_and_set_bit/clear_bit as locks and in
> that case, clear bits needs a clobber.
> 
> So I would say at this stage better safe than having to track down
> incredibly hard to find bugs, and let's make them all take that clobber.

Well I'm tempted agree because I'm abusing these constructs in  exactly
the manner you describe. :-) However I didn't know that this was abuse
until you mentioned it. Some time ago I noticed that the bitops code was
very similar to spinlocks, and so I presumed that a bitops word could
act as its own spinlock (ie. rather than spinlocking access to a u32).
Now that I look at spinlocks again, I see the presence of those
CLEAR_IO_SYNC definitions in the function preambles - is that the
distinction I'm abusing? CLEAR_IO_SYNC appears to be undefined except on
64-bit, in which case it's "(get_paca()->io_sync = 0)".

W.r.t the _lock/_unlock variants on the bitops side, the "lock"
particulars appear to depend on LWSYNC_ON_SMP and ISYNC_ON_SMP, which
are "isync" and "lwsync" for all platforms IIUC. So it seems the locking
intentions here are different from that of spinlocks? Is there something
I can look at that describes what semantics these primitives (are
supposed to) guarantee? I may be assuming other things that I shouldn't
be ...

Cheers,
Geoff