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

[Randy Dunlap <rdunlap@infradead.org>]

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