Benjamin Herrenschmidt wrote:

On Thu, 2009-06-18 at 16:30 -0400, Geoff Thorpe wrote:

quoted

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