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

From: Shuah Khan
Date: Tue Nov 17 2020 - 13:24:09 EST


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