Messages in this thread | | | Subject | Re: [PATCH v2 01/13] seqnum_ops: Introduce Sequence Number Ops | From | Shuah Khan <> | Date | Tue, 17 Nov 2020 11:24:03 -0700 |
| |
On 11/17/20 10:38 AM, Matthew Wilcox wrote: > On Tue, Nov 17, 2020 at 09:34:24AM -0700, Shuah Khan wrote: >>> 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 > > No, it's more confusing. I know you're converting things from using > atomic_t, but you really need to think about this in terms of "What > makes sense for this API". Unless you really want to have inc that > returns void and inc_return that returns the new value, having only > inc_return makes no sense. >
I am fine with that. As I said I have a patch series saved with just seqnum_inc() that increments and returns. I anticipated people would have problems with seqnum_inc() that returns. :)
>> 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. > > By putting 32 and 64 in the name of the API, I would contend you're making > people think about something that they should not need to think about. >
Are you recommending seqnum32_*() for 32bit and seqnum_*() for 64bit which would make 64bit as a default? We have to make a distinction for 32bit vs. 64-bit api.
>> No need for atomic_add() - inc_return() is sufficient for this use-case. > > I haven't looked at the various potential users of this API, but there > are often cases where we account, eg, number of bytes transmitted. > > There are also cases where read-and-zero would be a useful operation > to have. I'm thinking about sampling statistics. >
The idea is isolating sequence number use-case first and restrict this api for that.
thanks, -- Shuah
| |