Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 7 Dec 2016 14:03:16 -0500
From: David Windsor <>
To: Peter Zijlstra <>
Cc: Boqun Feng <>, Kees Cook <>, 
	"Reshetova, Elena" <>, 
	"" <>, Greg KH <>, 
	"" <>, Hans Liljestrand <>, 
	"" <>, "" <>
Subject: Re: Conversion from atomic_t to refcount_t: summary of issues

On Wed, Dec 7, 2016 at 8:24 AM, Peter Zijlstra <> wrote:
> On Fri, Dec 02, 2016 at 03:25:42PM -0500, David Windsor wrote:
>> On Thu, Dec 1, 2016 at 8:17 PM, Boqun Feng <> wrote:
>> > So we currently don't have a clear semantics for stats_t, do we?
>> We had a discussion about the stats_t API in another thread.  We
>> agreed upon add(), sub(), inc(), dec(), read() and set().
>> > What kind of atomic_t should be replaced with stats_t?
>> stats_t is used for those cases in which an atomic variable is
>> required, but the overflow of this variable isn't of much concern.
>> Typically, these types of variables are counters of some kind (rx/tx
>> counts, etc), but not always.  Perhaps "stats_t" isn't the best type
>> name.  We actually used "atomic_wrap_t" in previous iterations.
> And atomic_wrap_t is a horrid trainwreck. Please as to explain the
> semantics of atomic_wrap_cmpxchg(). How does wrapping apply to something
> that doesn't do sign bits.

atomic_wrap_t grew out of a desire to fix an already broken system for
doing reference counting.  atomic_t is being widely used for both
reference counting (which should not overflow), non-reference
counting, and other operations.  atomic_wrap_t provides the semantics
of the "original" atomic_t: atomic operations with no overflow
protection.  Thus, atomic_wrap_cmpxchg(): the cmpxchg operation for
atomic_wrap_t types.  Abominations like this sometimes exist in RFC
series.  In this case, we came up with the atomic_wrap API by mostly
creating an atomic_wrap-ified version of each atomic_t API function.

>> > In the link David  pointed out, there are a few cases where a
>> > stats_t is put on a correctness-related variable. I don't think
>> > that's a good place to use stats_t.
>> >
>> Yeah, I just grabbed a few examples I noted during my stats_t
>> conversion work.  The drivers/ tree is littered with stats_t
>> instances.
> Not sure how to respond to this, if you're converting all that to
> stats_t then you're doing it wrong.
> Most of what you showed should very emphatically not be stats_t.

Yes, none of those examples were appropriate.  My apologies; they were
quickly chosen from a pool of potential stats_t conversions.

Further audit of atomic_wrap_t indicates that much of what we marked
as atomic_wrap_t are actually things like sequence numbers, IDs, etc.
struct parallel_data.seq_nr
struct btrfs_delayed_root.items_seq

These "identifier" types make up a good number of atomic_wrap_t cases.

With respect to stats_t variables, here are two more examples.

First, cm_listens_created and cm_listens_destroyed from
Only atomic_inc() and atomic_read() are used on these variables:
target_stat_values[++index] = atomic_read(&cm_listens_created);
target_stat_values[++index] = atomic_read(&cm_listens_destroyed);

Next, struct irq_desc.threads_handled.  It is defined in
include/linux/irqdesc.h and, in the struct's comments, is described
as, "[a] stats field for deferred spurious detection of threaded

Only atomic_inc() and atomic_read() are called to manage this variable:

handled = atomic_read(&desc->threads_handled);

irq_desc.threads_handled is part of threaded interrupt handlers and is
incremented in irq_thread():.

static int irq_thread(void *data)
    while (!irq_wait_for_interrupt(action)) {
        irqreturn_t action_ret;

        irq_thread_check_affinity(desc, action);

        action_ret = handler_fn(desc, action);
        if (action_ret == IRQ_HANDLED)

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.