Messages in this thread | | | Subject | Re: [PATCH v2 01/13] seqnum_ops: Introduce Sequence Number Ops | From | Shuah Khan <> | Date | Tue, 17 Nov 2020 09:34:24 -0700 |
| |
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
| |