Re: [PATCH v2 01/13] seqnum_ops: Introduce Sequence Number Ops

From: Shuah Khan <skhan@linuxfoundation.org>
To: Matthew Wilcox <willy@infradead.org>
Cc: corbet@lwn.net, keescook@chromium.org,
	gregkh@linuxfoundation.org, peterz@infradead.org,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	Shuah Khan <skhan@linuxfoundation.org>
Subject: Re: [PATCH v2 01/13] seqnum_ops: Introduce Sequence Number Ops
Date: Tue, 17 Nov 2020 09:34:24 -0700	[thread overview]
Message-ID: <13467f88-7e22-ce3e-60b6-44e7c3dfa7dc@linuxfoundation.org> (raw)
In-Reply-To: <20201113210327.GJ17076@casper.infradead.org>

On 11/13/20 2:03 PM, Matthew Wilcox wrote:
>> +==========================
>> +Sequence Number Operations
>> +==========================
>> +
>> +:Author: Shuah Khan
>> +:Copyright: |copy| 2020, The Linux Foundation
>> +:Copyright: |copy| 2020, Shuah Khan <skhan@linuxfoundation.org>
>> +
>> +Sequence Number api provides interfaces for unsigned up counters
>> +leveraging atomic_t and atomic64_t ops underneath.
> 
> As I said last time you posted this, the documentation is all
> back-to-front.  You're describing what it isn't, not what it is.
>

I will rephrase it to read better.

>> +There are a number of atomic_t usages in the kernel where atomic_t api
>> +is used for counting sequence numbers and other statistical counters.
>> +Several of these usages, convert atomic_read() and atomic_inc_return()
>> +return values to unsigned. Introducing sequence number ops supports
>> +these use-cases with a standard core-api.
>> +
>> +The atomic_t api provides a wide range of atomic operations as a base
>> +api to implement atomic counters, bitops, spinlock interfaces. The usages
>> +also evolved into being used for resource lifetimes and state management.
>> +The refcount_t api was introduced to address resource lifetime problems
>> +related to atomic_t wrapping. There is a large overlap between the
>> +atomic_t api used for resource lifetimes and just counters, stats, and
>> +sequence numbers. It has become difficult to differentiate between the
>> +atomic_t usages that should be converted to refcount_t and the ones that
>> +can be left alone. Introducing seqnum_ops to wrap the usages that are
>> +stats, counters, sequence numbers makes it easier for tools that scan
>> +for underflow and overflow on atomic_t usages to detect overflow and
>> +underflows to scan just the cases that are prone to errors.
>> +
>> +In addition, to supporting sequence number use-cases, Sequence Number Ops
>> +helps differentiate atomic_t counter usages from atomic_t usages that guard
>> +object lifetimes, hence prone to overflow and underflow errors from up
>> +counting use-cases.
> 
> I think almost all of this information should go into atomic_ops.rst
> pushing people towards using the other APIs instead of atomic_t.
> Someone who already landed here doesn't want to read about refcount_t.
> They want to know what a seqnum_t is and how to use it.
> 

Looks like this is resolved with atomic_ops.rst is now gone.

>> +Sequence Number Ops
>> +===================
>> +
>> +seqnum32 and seqnum64 types use atomic_t and atomic64_t underneath to
> 
> Don't talk about the implementation.
> 
>> +leverage atomic_t api, to provide increment by 1 and return new value
>> +and fetch current value interfaces necessary to support unsigned up
>> +counters. ::
>> +
>> +        struct seqnum32 { atomic_t seqnum; };
>> +        struct seqnum64 { atomic64_t seqnum; };
>> +
>> +Please see :ref:`Documentation/core-api/atomic_ops.rst <atomic_ops>` for
>> +information on the Semantics and Behavior of Atomic operations.
>> +
>> +Initializers
>> +------------
>> +
>> +Interfaces for initializing sequence numbers are write operations which
>> +in turn invoke their ``ATOMIC_INIT() and atomic_set()`` counterparts ::
>> +
>> +        #define SEQNUM_INIT(i)    { .seqnum = ATOMIC_INIT(i) }
>> +        seqnum32_init() --> atomic_set() to 0
>> +        seqnum64_init() --> atomic64_set() to 0
>> +
>> +Increment interface
>> +-------------------
>> +
>> +Increments sequence number and returns the new value. ::
>> +
>> +        seqnum32_inc_return() --> (u32) atomic_inc_return(seqnum)
>> +        seqnum64_inc_return() --> (u64) atomic64_inc_return(seqnum)
> 
> seqnum_inc() should just return the new value -- seqnum_inc_return is
> too verbose.  And do we not need a seqnum_add()?
> 

I had the patch series with seqnum_inc() all ready to go and then
revisited the choice. My thinking is that matching the current atomic
api that has _inc() and inc_return() might be less confusing. That
being said, I have no problems with making just _inc(). The reason
for 32 and 64 appended is based on comments that it including size
in the api makes it very clear.

No need for atomic_add() - inc_return() is sufficient for this use-case.

> Also, this would be a good point to talk about behaviour on overflow.
> 

I can add some overflow information.

thanks,
-- Shuah