| Message ID | cover.1612314468.git.skhan@linuxfoundation.org |
|---|---|
| Headers | show |
| Series | Introduce Sequence Number Ops | expand |
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; > +} What kind of broken garbage is that?
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. Oh, and checkpatch should have complained about the extra ' ' in your cast :) thanks, greg k-h
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. thanks, -- Shuah
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)?
Hi-- Comments are inline. On 2/3/21 10:11 AM, Shuah Khan wrote: > Sequence Number api provides interfaces for unsigned atomic up counters. > > 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. > > Sequence Number ops provide interfaces to initialize, increment and get > the sequence number. These ops also check for overflow and log message to > indicate when overflow occurs. > > Signed-off-by: Shuah Khan <skhan@linuxfoundation.org> > --- > Documentation/core-api/index.rst | 1 + > Documentation/core-api/seqnum_ops.rst | 53 ++++++++++ > MAINTAINERS | 7 ++ > include/linux/seqnum_ops.h | 129 +++++++++++++++++++++++++ > lib/Kconfig | 9 ++ > lib/Makefile | 1 + > lib/test_seqnum_ops.c | 133 ++++++++++++++++++++++++++ > 7 files changed, 333 insertions(+) > create mode 100644 Documentation/core-api/seqnum_ops.rst > create mode 100644 include/linux/seqnum_ops.h > create mode 100644 lib/test_seqnum_ops.c > diff --git a/Documentation/core-api/seqnum_ops.rst b/Documentation/core-api/seqnum_ops.rst > new file mode 100644 > index 000000000000..ed4eba394799 > --- /dev/null > +++ b/Documentation/core-api/seqnum_ops.rst > @@ -0,0 +1,53 @@ > +.. SPDX-License-Identifier: GPL-2.0 > + > +.. include:: <isonum.txt> > + > +.. _seqnum_ops: > + > +========================== > +Sequence Number Operations > +========================== > + > +:Author: Shuah Khan > +:Copyright: |copy| 2021, The Linux Foundation > +:Copyright: |copy| 2021, Shuah Khan <skhan@linuxfoundation.org> > + > +Sequence Number api provides interfaces for unsigned up counters. API > + > +Sequence Number Ops > +=================== > + > +seqnum32 and seqnum64 types support implementing unsigned up counters. :: > + > + struct seqnum32 { u32 seqnum; }; > + struct seqnum64 { u64 seqnum; }; > + > +Initializers > +------------ > + > +Interfaces for initializing sequence numbers. :: > + > + #define SEQNUM_INIT(i) { .seqnum = i } > + seqnum32_init(seqnum, val) > + seqnum64_init(seqnum, val) > + > +Increment interface > +------------------- > + > +Increments sequence number and returns the new value. Checks for overflow > +conditions and logs message when overflow occurs. This check is intended > +to help catch cases where overflow could lead to problems. :: > + > + seqnum32_inc(seqnum): Calls atomic_inc_return(seqnum). > + seqnum64_inc(seqnum): Calls atomic64_inc_return(seqnum). > + > +Return/get value interface > +-------------------------- > + > +Returns sequence number value. :: > + > + seqnum32_get() - return seqnum value. > + seqnum64_get() - return seqnum value. > + > +.. warning:: > + seqnum32 wraps around to INT_MIN when it overflows. > diff --git a/MAINTAINERS b/MAINTAINERS > index cc1e6a5ee6e6..f9fe1438a8cd 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -16235,6 +16235,13 @@ S: Maintained > F: Documentation/fb/sm712fb.rst > F: drivers/video/fbdev/sm712* > > +SEQNUM OPS > +M: Shuah Khan <skhan@linuxfoundation.org> > +L: linux-kernel@vger.kernel.org > +S: Maintained > +F: include/linux/seqnum_ops.h > +F: lib/test_seqnum_ops.c > + > SIMPLE FIRMWARE INTERFACE (SFI) > S: Obsolete > W: http://simplefirmware.org/ > diff --git a/include/linux/seqnum_ops.h b/include/linux/seqnum_ops.h > new file mode 100644 > index 000000000000..e8d8481445d3 > --- /dev/null > +++ b/include/linux/seqnum_ops.h > @@ -0,0 +1,129 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * seqnum_ops.h - Interfaces for unsigned atomic sequential up counters. > + * > + * Copyright (c) 2021 Shuah Khan <skhan@linuxfoundation.org> > + * Copyright (c) 2021 The Linux Foundation > + * > + * Sequence Number functions provide support for unsgined atomic up unsigned > + * counters. > + * > + * The interface provides: > + * seqnumu32 & seqnumu64 functions: > + * initialization > + * increment and return > + * > + * seqnumu32 and seqnumu64 functions leverage/use atomic*_t ops to > + * implement support for unsigned atomic up counters. > + * > + * Reference and API guide: > + * Documentation/core-api/seqnum_ops.rst for more information. > + */ > + > +#ifndef __LINUX_SEQNUM_OPS_H > +#define __LINUX_SEQNUM_OPS_H > + > +#include <linux/atomic.h> > + > +/** > + * struct seqnum32 - Sequence number atomic counter > + * @seqnum: atomic_t > + * > + **/ > +struct seqnum32 { > + u32 seqnum; > +}; > + > +#define SEQNUM_INIT(i) { .seqnum = i } > + > +/* > + * seqnum32_init() - initialize seqnum value > + * @seq: struct seqnum32 pointer > + * > + */ > +static inline void seqnum32_init(struct seqnum32 *seq, u32 val) > +{ > + seq->seqnum = val; > +} > + > +/* > + * seqnum32_inc() - increment seqnum value and return the new value > + * @seq: struct seqnum32 pointer > + * > + * Return u32 It would be good to convert that to kernel-doc notation. > + */ > +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; > +} > + > +/* > + * seqnum32_get() - get seqnum value > + * @seq: struct seqnum32 pointer > + * > + * Return u32 > + */ > +static inline u32 seqnum32_get(struct seqnum32 *seq) > +{ > + return seq->seqnum; > +} > + > +/* > + * struct seqnum64 - Sequential/Statistical atomic counter > + * @seq: atomic64_t > + * > + */ > +struct seqnum64 { > + u64 seqnum; > +}; > + > +/* Add to a global include/vdso/limits.h and fix all other UINT64_MAX > + * duplicate defines? > + */ > +#define SEQ_UINT64_MAX ((u64)(~((u64) 0))) /* 0xFFFFFFFFFFFFFFFF */ > + > +/* > + * seqnum64_init() - initialize seqnum value > + * @seq: struct seqnum64 pointer > + * > + */ and kernel-doc there also. > +static inline void seqnum64_init(struct seqnum64 *seq, u64 val) > +{ > + seq->seqnum = val; > +} > + > +/* > + * seqnum64_inc() - increment seqnum value and return the new value > + * @seq: struct seqnum64 pointer > + * > + * Return u64 > + */ > +static inline u64 seqnum64_inc(struct seqnum64 *seq) > +{ > + atomic64_t val = ATOMIC_INIT(seq->seqnum); > + > + seq->seqnum = (u64) atomic64_inc_return(&val); > + if (seq->seqnum >= SEQ_UINT64_MAX) > + pr_info("Sequence Number overflow %llu detected\n", > + seq->seqnum); > + return seq->seqnum; > +} > + > +/* > + * seqnum64_get() - get seqnum value > + * @seq: struct seqnum64 pointer > + * > + * Return u64 > + */ > +static inline u64 seqnum64_get(struct seqnum64 *seq) > +{ > + return (u64) seq->seqnum; > +} > + > +#endif /* __LINUX_SEQNUM_OPS_H */ > diff --git a/lib/test_seqnum_ops.c b/lib/test_seqnum_ops.c > new file mode 100644 > index 000000000000..173278314f26 > --- /dev/null > +++ b/lib/test_seqnum_ops.c > @@ -0,0 +1,133 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * test_seqnum_ops.c - Kernel module for testing Seqnum API > + * > + * Copyright (c) 2021 Shuah Khan <skhan@linuxfoundation.org> > + * Copyright (c) 2021 The Linux Foundation > + * > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/module.h> > +#include <linux/seqnum_ops.h> > + > ... > +static void test_seqnum64(void) > +{ > + u64 start_val = 0; > + struct seqnum64 seq = SEQNUM_INIT(start_val); > + u64 end_val; > + > + end_val = seqnum64_inc(&seq); > + test_seqnum64_result("Test increment", > + start_val, end_val, start_val+1); > + > + /* Initialize sequence number to 0 */ > + seqnum64_init(&seq, start_val); > + end_val = seqnum64_inc(&seq); > + > + /* if seqnum642_init() works correctly end_val should be 1 */ seqnum64_init() AFAICT. > + test_seqnum64_result("Test init", start_val, end_val, 1); > + /* seqnum64_get() test for seqnum value == 1 */ > + start_val = end_val = seqnum64_get(&seq); > + test_seqnum64_result("Test get", start_val, end_val, 1); > +} > + -- ~Randy