Re: [PATCH 0/24] make atomic_read() behave consistently across all architectures

Nick Piggin wrote:
Stefan Richter wrote:
For architecture port authors, there is Documentation/atomic_ops.txt.
Driver authors also can learn something from that document, as it
indirectly documents the atomic_t and bitops APIs.

"Semantics and Behavior of Atomic and Bitmask Operations" is
pretty direct :)

"Indirect", "pretty direct"... It's subjective.

(It is not an API documentation; it is an implementation specification.)

Sure, it says that it's for arch maintainers, but there is no
reason why users can't make use of it.


Prompted by this thread, I reread this document, and indeed, the
sentence "Unlike the above routines, it is required that explicit memory
barriers are performed before and after [atomic_{inc,dec}_return]"
indicates that atomic_read (one of the "above routines") is very
different from all other atomic_t accessors that return values.

This is strange. Why is it that atomic_read stands out that way? IMO

It is not just atomic_read of course. It is atomic_add,sub,inc,dec,set.

Yes, but unlike these, atomic_read returns a value.

Without me (the API user) providing extra barriers, that value may
become something else whenever someone touches code in the vicinity of
the atomic_read.

this API imbalance is quite unexpected by many people. Wouldn't it be
beneficial to change the atomic_read API to behave the same like all
other atomic_t accessors that return values?

It is very consistent and well defined. Operations which both modify
the data _and_ return something are defined to have full barriers
before and after.

You are right, atomic_read is not only different from accessors that
don't retunr values, it is also different from all other accessors that
return values (because they all also modify the value). There is just
no actual API documentation, which contributes to the issue that some
people (or at least one: me) learn a little bit late how special
atomic_read is.

What do you want to add to the other atomic accessors? Full memory
barriers? Only compiler barriers? It's quite likely that if you think
some barriers will fix bugs, then there are other bugs lurking there
anyway.

A lot of different though related issues are discussed in this thread,
but I personally am only occupied by one particular thing: What kind of
return values do I get from atomic_read.

Just use spinlocks if you're not absolutely clear about potential
races and memory ordering issues -- they're pretty cheap and simple.

Probably good advice, like generally if driver guys consider lockless
algorithms.

OK, it is also different from the other accessors that return data in so
far as it doesn't modify the data. But as driver "author", i.e. user of
the API, I can't see much use of an atomic_read that can be reordered
and, more importantly, can be optimized away by the compiler.

It will return to you an atomic snapshot of the data (loaded from
memory at some point since the last compiler barrier). All you have
to be aware of compiler barriers and the Linux SMP memory ordering
model, which should be a given if you are writing lockless code.

OK, that's what I slowly realized during this discussion, and I
appreciate the explanations that were given here.

Sure, now
that I learned of these properties I can start to audit code and insert
barriers where I believe they are needed, but this simply means that
almost all occurrences of atomic_read will get barriers (unless there
already are implicit but more or less obvious barriers like msleep).

You might find that these places that appear to need barriers are
buggy for other reasons anyway. Can you point to some in-tree code
we can have a look at?

I could, or could not, if I were through with auditing the code. I
remembered one case and posted it (nodemgr_host_thread) which was safe
because msleep_interruptible provided the necessary barrier there, and
this implicit barrier is not in danger to be removed by future patches.
--
Stefan Richter
-=====-=-=== =--- =---=
http://arcgraph.de/sr/
-
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/

Relevant Pages

  • Re: [PATCH] ide write barrier support
    ... > patch that introduces a new internal API. ... So let me state again that this is NOT a performance patch. ... > or read/write barriers, if that is in fact the intention. ... Sometimes the only reliable way to detect whether you can support ...
    (Linux-Kernel)
  • Re: [PATCH] ide write barrier support
    ... It seems to me there are a few unresolved issues with the barrier API. ... or read/write barriers, if that is in fact the intention. ... barrier carried by a write BIO or a write barrier carried by a read BIO. ... device mapper, ...
    (Linux-Kernel)
  • Re: Waiting on data from driver
    ... Sure, but your API had better be well documented, which in my book is full ... documentation of everything that they may need to effectively use the ... driver, ... > smaillet at EmbeddedFusion dot com ...
    (microsoft.public.windowsce.platbuilder)
  • Re: Opinion on ordering of writel vs. stores to RAM
    ... writel/readl are fully synchronous ... The driver decides at ioremap time wether it wants a fully ordered ... - writel/readlbehave differently depending on the mapping ... enabled mapping using barriers. ...
    (Linux-Kernel)
  • Block devices and barriers
    ... I'm adding cache flush support to the PS3 disk driver and have a few questions ... Should I care about barriers? ...
    (Linux-Kernel)
Loading