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:15:34 -0700 |
| |
On 11/16/20 7:53 AM, Peter Zijlstra wrote: > On Fri, Nov 13, 2020 at 10:46:03AM -0700, Shuah Khan wrote: > >> +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) > > Did you think about the ordering? >
Looking at atomic_t.txt _inc_return() can be fully ordered without loosing or making the intermediate state visible. This is good for this sequence number use-case. Is there something I am overlooking?
>> +Fetch interface >> +--------------- >> + >> +Fetched and returns current sequence number value. :: >> + >> + seqnum32_fetch() --> (u32) atomic_add_return(0, seqnum) >> + seqnum64_fetch() --> (u64) atomic64_add_return(0, seqnum) > > That's horrible. Please explain how that is not broken garbage. > > Per the fact that it is atomic, nothing prevents the counter being > incremented concurrently. There is no such thing as a 'current' sequence > number. >
Correct. Some usages of this _fecth() in this patch series are for printing sequence numbers in debug message and others are stats.
I will review the patches in this series and drop the ones that use read/fetch - the reason being sequence numbers are strictly up counters and don't need read/fetch.
thanks, -- Shuah
| |