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

[Shuah Khan]

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@xxxxxxxxxxxxxxxxxxx>
> > +
> > +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