Re: [PATCH v3 1/7] seqnum_ops: Introduce Sequence Number Ops

[Luck, Tony]

On Fri, Feb 05, 2021 at 01:03:18PM -0700, Shuah Khan wrote:
> On 2/5/21 2:58 AM, Greg KH wrote:
> > On Wed, Feb 03, 2021 at 11:11:57AM -0700, Shuah Khan wrote:
> > > +static inline u32 seqnum32_inc(struct seqnum32 *seq)
> > > +{
> > > +	atomic_t val = ATOMIC_INIT(seq->seqnum);
> > > +
> > > +	seq->seqnum = (u32) atomic_inc_return(&val);
> > > +	if (seq->seqnum >= UINT_MAX)
> > > +		pr_info("Sequence Number overflow %u detected\n",
> > > +			seq->seqnum);
> > > +	return seq->seqnum;
> > 
> > As Peter points out, this is doing doing what you think it is doing :(
> > 
> > Why do you not just have seq->seqnum be a real atomic variable?  Trying
> > to switch to/from one like this does not work as there is no
> > "atomic-ness" happening here at all.
> > 
> 
> Yes. This is sloppy on my part. As Peter and Rafael also pointed. I have
> to start paying more attention to my inner voice.

What's the end goal with this sequence number type?

Apart from the functional issues with this code already pointed
out, it doesn't seem overly useful to just print the *value* of
the sequence number when it hits the max value.  It might be
more helpful if each instance had a seq->name field that is
used here to tell the user *which* user of sequence numbers
had seen the wrap arounnd.

But that just begs the question of what should users of sequence
numbers do when wrap around occurs? Any user that can run through
sequence numbers at greater than 10 Hz is going to cause a problem
for systems that stay running for years.

Maybe you should force users to code for wraparound by initializing
to something like 0xffffff00 so that they all get a wraparound event
quickly, rather than slowly, (same as was done with jiffies)?