Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <1510732551-25547-2-git-send-email-elena.reshetova@intel.com>
References: <1510732551-25547-1-git-send-email-elena.reshetova@intel.com> <1510732551-25547-2-git-send-email-elena.reshetova@intel.com>
From:   Kees Cook <keescook@chromium.org>
Date:   Thu, 16 Nov 2017 15:31:34 -0800
Message-ID: <CAGXu5jJeste8gn5x2BrqGNqzri0M8yimVyoRTcV3oJ1MvSBi4A@mail.gmail.com>
Subject: Re: [PATCH] refcount_t: documentation for memory ordering differences
To:     Elena Reshetova <elena.reshetova@intel.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Greg KH <gregkh@linuxfoundation.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>,
        Hans Liljestrand <ishkamiel@gmail.com>,
        "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
        Alan Stern <stern@rowland.harvard.edu>,
        Jonathan Corbet <corbet@lwn.net>, linux-doc@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Nov 14, 2017 at 11:55 PM, Elena Reshetova
<elena.reshetova@intel.com> wrote:
> Some functions from refcount_t API provide different
> memory ordering guarantees that their atomic counterparts.
> This adds a document outlining these differences.

Thanks for writing this up! One bike-shedding thing I'll bring up
before anyone else does is: please format this in ReST and link to it
from somewhere (likely developer documentation) in the Documentation/
index.rst file somewhere.

Perhaps in Documentation/core-api/index.rst ?

Lots of notes here:
https://www.kernel.org/doc/html/latest/doc-guide/sphinx.html#writing-documentation

> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> ---
>  Documentation/refcount-vs-atomic.txt | 124 +++++++++++++++++++++++++++++++++++
>  1 file changed, 124 insertions(+)
>  create mode 100644 Documentation/refcount-vs-atomic.txt
>
> diff --git a/Documentation/refcount-vs-atomic.txt b/Documentation/refcount-vs-atomic.txt
> new file mode 100644
> index 0000000..e703039
> --- /dev/null
> +++ b/Documentation/refcount-vs-atomic.txt
> @@ -0,0 +1,124 @@
> +==================================
> +refcount_t API compare to atomic_t

"compared"

> +==================================
> +
> +The goal of refcount_t API is to provide a minimal API for implementing
> +object's reference counters. While a generic architecture-independent

"an object's"

> +implementation from lib/refcount.c uses atomic operations underneath,
> +there are a number of differences between some of the refcount_*() and
> +atomic_*() functions with regards to the memory ordering guarantees.
> +
> +This document outlines the differences and provides respective examples
> +in order to help maintainers validate their code against the change in
> +these memory ordering guarantees.
> +
> +memory-barriers.txt and atomic_t.txt provide more background to the
> +memory ordering in general and for atomic operations specifically.
> +
> +Notation

Should this section be called "Types of memory ordering"?

> +========
> +
> +An absence of memory ordering guarantees (i.e. fully unordered)
> +in case of atomics & refcounters only provides atomicity and

I can't parse this. "In an absense ... atomics & refcounts only provide ... "?

> +program order (po) relation (on the same CPU). It guarantees that
> +each atomic_*() and refcount_*() operation is atomic and instructions
> +are executed in program order on a single CPU.
> +Implemented using READ_ONCE()/WRITE_ONCE() and
> +compare-and-swap primitives.

For here an later, maybe "This is implemented ..."

> +
> +A strong (full) memory ordering guarantees that all prior loads and
> +stores (all po-earlier instructions) on the same CPU are completed
> +before any po-later instruction is executed on the same CPU.
> +It also guarantees that all po-earlier stores on the same CPU
> +and all propagated stores from other CPUs must propagate to all
> +other CPUs before any po-later instruction is executed on the original
> +CPU (A-cumulative property). Implemented using smp_mb().
> +
> +A RELEASE memory ordering guarantees that all prior loads and
> +stores (all po-earlier instructions) on the same CPU are completed
> +before the operation. It also guarantees that all po-earlier
> +stores on the same CPU and all propagated stores from other CPUs
> +must propagate to all other CPUs before the release operation
> +(A-cumulative property). Implemented using smp_store_release().
> +
> +A control dependency (on success) for refcounters guarantees that
> +if a reference for an object was successfully obtained (reference
> +counter increment or addition happened, function returned true),
> +then further stores are ordered against this operation.
> +Control dependency on stores are not implemented using any explicit
> +barriers, but rely on CPU not to speculate on stores. This is only
> +a single CPU relation and provides no guarantees for other CPUs.
> +
> +
> +Comparison of functions
> +==========================
> +
> +case 1) - non-RMW ops

Should this be spelled out "Read/Modify/Write"?

> +---------------------
> +
> +Function changes:
> +                atomic_set() --> refcount_set()
> +                atomic_read() --> refcount_read()
> +
> +Memory ordering guarantee changes:
> +                fully unordered --> fully unordered

Maybe say: "none (both fully unordered)"

> +case 2) - increment-based ops that return no value
> +--------------------------------------------------
> +
> +Function changes:
> +                atomic_inc() --> refcount_inc()
> +                atomic_add() --> refcount_add()
> +
> +Memory ordering guarantee changes:
> +                fully unordered --> fully unordered

Same.

> +case 3) - decrement-based RMW ops that return no value
> +------------------------------------------------------
> +Function changes:
> +                atomic_dec() --> refcount_dec()
> +
> +Memory ordering guarantee changes:
> +                fully unordered --> RELEASE ordering

Should the sections where there is a change include an example of how
this might matter to a developer?

> +
> +
> +case 4) - increment-based RMW ops that return a value
> +-----------------------------------------------------
> +
> +Function changes:
> +                atomic_inc_not_zero() --> refcount_inc_not_zero()
> +                no atomic counterpart --> refcount_add_not_zero()
> +
> +Memory ordering guarantees changes:
> +                fully ordered --> control dependency on success for stores
> +
> +*Note*: we really assume here that necessary ordering is provided as a result
> +of obtaining pointer to the object!

Same.

> +
> +
> +case 5) - decrement-based RMW ops that return a value
> +-----------------------------------------------------
> +
> +Function changes:
> +                atomic_dec_and_test() --> refcount_dec_and_test()
> +                atomic_sub_and_test() --> refcount_sub_and_test()
> +                no atomic counterpart --> refcount_dec_if_one()
> +                atomic_add_unless(&var, -1, 1) --> refcount_dec_not_one(&var)
> +
> +Memory ordering guarantees changes:
> +                fully ordered --> RELEASE ordering + control dependency
> +
> +Note: atomic_add_unless() only provides full order on success.

Same.

> +
> +
> +case 6) - lock-based RMW
> +------------------------
> +
> +Function changes:
> +
> +                atomic_dec_and_lock() --> refcount_dec_and_lock()
> +                atomic_dec_and_mutex_lock() --> refcount_dec_and_mutex_lock()
> +
> +Memory ordering guarantees changes:
> +                fully ordered --> RELEASE ordering + control dependency +
> +                                  hold spin_lock() on success

Same.

This looks like a good start to helping people answer questions about
refcount_t memory ordering. Thanks!

-Kees

-- 
Kees Cook
Pixel Security

From 1584118008493682834@xxx Wed Nov 15 07:57:38 +0000 2017
X-GM-THRID: 1584118008493682834
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread