[PATCH v2 00/13] Introduce seqnum_ops

[PATCH v2 00/13] Introduce seqnum_ops

[Shuah Khan]

Sequence Number api provides interfaces for unsigned atomic up counters
leveraging atomic_t and atomic64_t ops underneath.

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.

The atomic_t api provides a wide range of atomic operations as a base
api to implement atomic counters, bitops, spinlock interfaces. The usages
also evolved into being used for resource lifetimes and state management.
The refcount_t api was introduced to address resource lifetime problems
related to atomic_t wrapping. There is a large overlap between the
atomic_t api used for resource lifetimes and just counters, stats, and
sequence numbers. It has become difficult to differentiate between the
atomic_t usages that should be converted to refcount_t and the ones that
can be left alone. Introducing seqnum_ops to wrap the usages that are
stats, counters, sequence numbers makes it easier for tools that scan
for underflow and overflow on atomic_t usages to detect overflow and
underflows to scan just the cases that are prone to errors.

In addition, to supporting sequence number use-cases, Sequence Number Ops
helps differentiate atomic_t counter usages from atomic_t usages that guard
object lifetimes, hence prone to overflow and underflow errors from up
counting use-cases. It becomes easier for tools that scan for underflow and
overflow on atomic_t usages to detect overflow and underflows to scan just
the cases that are prone to errors.

Changes since v1:
- Removed dec based on Greg KH's comments
- Removed read/set/inc based on the discussion with Peter Zijlstra
- Interfaces are restricted to init, increment and return new value,
  and fetch current value.
- Interfaces return u32 and u64 - a few reviewers suggested unsigned.
  After reviewing a few use-cases, I determined this is a good path
  forward. It adds unsigned atomic support that doesn't exist now,
  and simplifies code in drivers that currently convert atomic_t return
  values to unsigned. All the drivers changes included in this series
  used to convert atomic_t returns to unsigned.

Patch v1 thread:
https://lore.kernel.org/lkml/cover.1605027593.git.skhan@linuxfoundation.org/

Counters thread:
lore.kernel.org/lkml/cover.1602209970.git.skhan@linuxfoundation.org

Shuah Khan (13):
  seqnum_ops: Introduce Sequence Number Ops
  selftests: lib:test_seqnum_ops: add new test for seqnum_ops
  drivers/acpi: convert seqno seqnum_ops
  drivers/acpi/apei: convert seqno to seqnum_ops
  drivers/base/test/test_async_driver_probe: convert to use seqnum_ops
  drivers/char/ipmi: convert stats to use seqnum_ops
  drivers/edac: convert pci counters to seqnum_ops
  drivers/oprofile: convert stats to use seqnum_ops
  drivers/staging/rtl8723bs: convert stats to use seqnum_ops
  usb: usbip/vhci: convert seqno to seqnum_ops
  drivers/staging/rtl8188eu: convert stats to use seqnum_ops
  drivers/staging/unisys/visorhba: convert stats to use seqnum_ops
  security/integrity/ima: converts stats to seqnum_ops

 Documentation/core-api/atomic_ops.rst         |   4 +
 Documentation/core-api/index.rst              |   1 +
 Documentation/core-api/seqnum_ops.rst         |  89 +++++++++++++
 MAINTAINERS                                   |   8 ++
 drivers/acpi/acpi_extlog.c                    |   8 +-
 drivers/acpi/apei/ghes.c                      |   8 +-
 drivers/base/test/test_async_driver_probe.c   |  28 +++--
 drivers/char/ipmi/ipmi_msghandler.c           |   9 +-
 drivers/char/ipmi/ipmi_si_intf.c              |   9 +-
 drivers/char/ipmi/ipmi_ssif.c                 |   9 +-
 drivers/edac/edac_pci.h                       |   5 +-
 drivers/edac/edac_pci_sysfs.c                 |  30 ++---
 drivers/oprofile/buffer_sync.c                |   9 +-
 drivers/oprofile/event_buffer.c               |   3 +-
 drivers/oprofile/oprof.c                      |   3 +-
 drivers/oprofile/oprofile_stats.c             |  11 +-
 drivers/oprofile/oprofile_stats.h             |  11 +-
 drivers/oprofile/oprofilefs.c                 |   3 +-
 drivers/staging/rtl8188eu/core/rtw_mlme_ext.c |  23 +++-
 .../staging/rtl8188eu/include/rtw_mlme_ext.h  |   3 +-
 drivers/staging/rtl8723bs/core/rtw_cmd.c      |   3 +-
 drivers/staging/rtl8723bs/core/rtw_mlme_ext.c |  33 +++--
 drivers/staging/rtl8723bs/include/rtw_cmd.h   |   3 +-
 .../staging/rtl8723bs/include/rtw_mlme_ext.h  |   3 +-
 .../staging/unisys/visorhba/visorhba_main.c   |  21 ++--
 drivers/usb/usbip/vhci.h                      |   3 +-
 drivers/usb/usbip/vhci_hcd.c                  |   7 +-
 drivers/usb/usbip/vhci_rx.c                   |   5 +-
 include/linux/oprofile.h                      |   3 +-
 include/linux/seqnum_ops.h                    | 118 +++++++++++++++++
 lib/Kconfig                                   |   9 ++
 lib/Makefile                                  |   1 +
 lib/test_seqnum_ops.c                         | 119 ++++++++++++++++++
 security/integrity/ima/ima.h                  |   5 +-
 security/integrity/ima/ima_api.c              |   3 +-
 security/integrity/ima/ima_fs.c               |   5 +-
 security/integrity/ima/ima_queue.c            |   7 +-
 tools/testing/selftests/lib/Makefile          |   1 +
 tools/testing/selftests/lib/config            |   1 +
 .../testing/selftests/lib/test_seqnum_ops.sh  |  10 ++
 40 files changed, 524 insertions(+), 110 deletions(-)
 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
 create mode 100755 tools/testing/selftests/lib/test_seqnum_ops.sh

-- 
2.27.0

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

[Shuah Khan]

Sequence Number api provides interfaces for unsigned atomic up counters
leveraging atomic_t and atomic64_t ops underneath.

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.

The atomic_t api provides a wide range of atomic operations as a base
api to implement atomic counters, bitops, spinlock interfaces. The usages
also evolved into being used for resource lifetimes and state management.
The refcount_t api was introduced to address resource lifetime problems
related to atomic_t wrapping. There is a large overlap between the
atomic_t api used for resource lifetimes and just counters, stats, and
sequence numbers. It has become difficult to differentiate between the
atomic_t usages that should be converted to refcount_t and the ones that
can be left alone. Introducing seqnum_ops to wrap the usages that are
stats, counters, sequence numbers makes it easier for tools that scan
for underflow and overflow on atomic_t usages to detect overflow and
underflows to scan just the cases that are prone to errors.

In addition, to supporting sequence number use-cases, Sequence Number Ops
helps differentiate atomic_t counter usages from atomic_t usages that guard
object lifetimes, hence prone to overflow and underflow errors from up
counting use-cases.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 Documentation/core-api/atomic_ops.rst |   4 +
 Documentation/core-api/index.rst      |   1 +
 Documentation/core-api/seqnum_ops.rst |  80 +++++++++++++++++
 MAINTAINERS                           |   7 ++
 include/linux/seqnum_ops.h            | 116 +++++++++++++++++++++++++
 lib/Kconfig                           |   9 ++
 lib/Makefile                          |   1 +
 lib/test_seqnum_ops.c                 | 119 ++++++++++++++++++++++++++
 8 files changed, 337 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/atomic_ops.rst b/Documentation/core-api/atomic_ops.rst
index 724583453e1f..762cbc0947e7 100644
--- a/Documentation/core-api/atomic_ops.rst
+++ b/Documentation/core-api/atomic_ops.rst
@@ -1,3 +1,7 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+.. _atomic_ops:
+
 =======================================================
 Semantics and Behavior of Atomic and Bitmask Operations
 =======================================================
diff --git a/Documentation/core-api/index.rst b/Documentation/core-api/index.rst
index 69171b1799f2..be958afe757c 100644
--- a/Documentation/core-api/index.rst
+++ b/Documentation/core-api/index.rst
@@ -55,6 +55,7 @@ How Linux keeps everything from happening at the same time.  See
 
    atomic_ops
    refcount-vs-atomic
+   seqnum_ops
    irq/index
    local_ops
    padata
diff --git a/Documentation/core-api/seqnum_ops.rst b/Documentation/core-api/seqnum_ops.rst
new file mode 100644
index 000000000000..10b775a9ac05
--- /dev/null
+++ b/Documentation/core-api/seqnum_ops.rst
@@ -0,0 +1,80 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+.. include:: <isonum.txt>
+
+.. _seqnum_ops:
+
+==========================
+Sequence Number Operations
+==========================
+
+:Author: Shuah Khan
+:Copyright: |copy| 2020, The Linux Foundation
+:Copyright: |copy| 2020, Shuah Khan <skhan@linuxfoundation.org>
+
+Sequence Number api provides interfaces for unsigned up counters
+leveraging atomic_t and atomic64_t ops underneath.
+
+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.
+
+The atomic_t api provides a wide range of atomic operations as a base
+api to implement atomic counters, bitops, spinlock interfaces. The usages
+also evolved into being used for resource lifetimes and state management.
+The refcount_t api was introduced to address resource lifetime problems
+related to atomic_t wrapping. There is a large overlap between the
+atomic_t api used for resource lifetimes and just counters, stats, and
+sequence numbers. It has become difficult to differentiate between the
+atomic_t usages that should be converted to refcount_t and the ones that
+can be left alone. Introducing seqnum_ops to wrap the usages that are
+stats, counters, sequence numbers makes it easier for tools that scan
+for underflow and overflow on atomic_t usages to detect overflow and
+underflows to scan just the cases that are prone to errors.
+
+In addition, to supporting sequence number use-cases, Sequence Number Ops
+helps differentiate atomic_t counter usages from atomic_t usages that guard
+object lifetimes, hence prone to overflow and underflow errors from up
+counting use-cases.
+
+Sequence Number Ops
+===================
+
+seqnum32 and seqnum64 types use atomic_t and atomic64_t underneath to
+leverage atomic_t api, to provide increment by 1 and return new value
+and fetch current value interfaces necessary to support unsigned up
+counters. ::
+
+        struct seqnum32 { atomic_t seqnum; };
+        struct seqnum64 { atomic64_t seqnum; };
+
+Please see :ref:`Documentation/core-api/atomic_ops.rst <atomic_ops>` for
+information on the Semantics and Behavior of Atomic operations.
+
+Initializers
+------------
+
+Interfaces for initializing sequence numbers are write operations which
+in turn invoke their ``ATOMIC_INIT() and atomic_set()`` counterparts ::
+
+        #define SEQNUM_INIT(i)    { .seqnum = ATOMIC_INIT(i) }
+        seqnum32_init() --> atomic_set() to 0
+        seqnum64_init() --> atomic64_set() to 0
+
+Increment interface
+-------------------
+
+Increments sequence number and returns the new value. ::
+
+        seqnum32_inc_return() --> (u32) atomic_inc_return(seqnum)
+        seqnum64_inc_return() --> (u64) atomic64_inc_return(seqnum)
+
+Fetch interface
+---------------
+
+Fetched and returns current sequence number value. ::
+
+        seqnum32_fetch() --> (u32) atomic_add_return(0, seqnum)
+        seqnum64_fetch() --> (u64) atomic64_add_return(0, seqnum)
diff --git a/MAINTAINERS b/MAINTAINERS
index b516bb34a8d5..c83a6f05610b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15977,6 +15977,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..17d327b78050
--- /dev/null
+++ b/include/linux/seqnum_ops.h
@@ -0,0 +1,116 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * seqnum_ops.h - Interfaces for sequential and statistical counters.
+ *
+ * Copyright (c) 2020 Shuah Khan <skhan@linuxfoundation.org>
+ * Copyright (c) 2020 The Linux Foundation
+ *
+ * Sequence Number functions provide support for unsgined atomic up
+ * counters.
+ *
+ * The interface provides:
+ * seqnumu32 & seqnumu64 functions:
+ *	initialization
+ *	increment and return
+ *	fetch 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 - Sequential/Statistical atomic counter
+ * @seqnum: atomic_t
+ *
+ **/
+struct seqnum32 {
+	atomic_t seqnum;
+};
+
+#define SEQNUM_INIT(i)		{ .seqnum = ATOMIC_INIT(i) }
+
+/*
+ * seqnum32_init() - initialize seqnum value
+ * @seq: struct seqnum32 pointer
+ *
+ */
+static inline void seqnum32_init(struct seqnum32 *seq)
+{
+	atomic_set(&seq->seqnum, 0);
+}
+
+/*
+ * seqnum32_inc_return() - increment seqnum value and return the new value
+ * @seq: struct seqnum32 pointer
+ *
+ * Return u32
+ */
+static inline u32 seqnum32_inc_return(struct seqnum32 *seq)
+{
+	return (u32) atomic_inc_return(&seq->seqnum);
+}
+
+/*
+ * seqnum32_fetch() - return the current value
+ * @seq: struct seqnum32 pointer
+ *
+ * Return u32
+ */
+static inline u32 seqnum32_fetch(struct seqnum32 *seq)
+{
+	return (u32) atomic_add_return(0, &seq->seqnum);
+}
+
+#ifdef CONFIG_64BIT
+/* atomic64_t is defined in CONFIG_64BIT in include/linux/types.h */
+/*
+ * struct seqnum64 - Sequential/Statistical atomic counter
+ * @seq: atomic64_t
+ *
+ */
+struct seqnum64 {
+	atomic64_t seqnum;
+};
+
+/*
+ * seqnum64_init() - initialize seqnum value
+ * @seq: struct seqnum64 pointer
+ *
+ */
+static inline void seqnum64_init(struct seqnum64 *seq)
+{
+	atomic64_set(&seq->seqnum, 0);
+}
+
+/*
+ * seqnum64_inc() - increment seqnum value and return the new value
+ * @seq: struct seqnum64 pointer
+ *
+ * Return u64
+ */
+static inline u64 seqnum64_inc_return(struct seqnum64 *seq)
+{
+	return (u64) atomic64_inc_return(&seq->seqnum);
+}
+
+/*
+ * seqnum64_fetch() - return the current value
+ * @seq: struct seqnum64 pointer
+ *
+ * Return u64
+ */
+static inline u64 seqnum64_fetch(struct seqnum64 *seq)
+{
+	return (u64) atomic64_add_return(0, &seq->seqnum);
+}
+#endif /* CONFIG_64BIT */
+
+#endif /* __LINUX_SEQNUM_OPS_H */
diff --git a/lib/Kconfig b/lib/Kconfig
index b46a9fd122c8..c362c2713e11 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -663,6 +663,15 @@ config OBJAGG
 config STRING_SELFTEST
 	tristate "Test string functions"
 
+config TEST_SEQNUM_OPS
+	tristate "Test Sequence Number Ops API"
+	help
+	   A test module for Sequence Number Ops API. A corresponding
+	   selftest can be used to test the Seqnum Ops API. Select this
+	   for testing Sequence Number Ops API.
+
+	   See Documentation/core-api/seqnum_ops.rst
+
 endmenu
 
 config GENERIC_IOREMAP
diff --git a/lib/Makefile b/lib/Makefile
index ce45af50983a..7d17c25e4d73 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -101,6 +101,7 @@ obj-$(CONFIG_TEST_MEMINIT) += test_meminit.o
 obj-$(CONFIG_TEST_LOCKUP) += test_lockup.o
 obj-$(CONFIG_TEST_HMM) += test_hmm.o
 obj-$(CONFIG_TEST_FREE_PAGES) += test_free_pages.o
+obj-$(CONFIG_TEST_SEQNUM_OPS) += test_seqnum_ops.o
 
 #
 # CFLAGS for compiling floating point code inside the kernel. x86/Makefile turns
diff --git a/lib/test_seqnum_ops.c b/lib/test_seqnum_ops.c
new file mode 100644
index 000000000000..6e58b6a0a2be
--- /dev/null
+++ b/lib/test_seqnum_ops.c
@@ -0,0 +1,119 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * test_seqnum_ops.c - Kernel module for testing Seqnum API
+ *
+ * Copyright (c) 2020 Shuah Khan <skhan@linuxfoundation.org>
+ * Copyright (c) 2020 The Linux Foundation
+ *
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/seqnum_ops.h>
+
+static inline void
+test_seqnum32_result(char *msg, u32 start, u32 end, u32 expected)
+{
+	pr_info("%s: %u to %u - %s\n",
+		msg, start, end,
+		((expected == end) ? "PASS" : "FAIL"));
+}
+
+
+static void test_seqnum32(void)
+{
+	static struct seqnum32 seq = SEQNUM_INIT(0);
+	u32 start_val = seqnum32_fetch(&seq);
+	u32 end_val;
+
+	end_val = seqnum32_inc_return(&seq);
+	test_seqnum32_result("Test fetch and increment",
+			     start_val, end_val, start_val+1);
+
+	start_val = seqnum32_fetch(&seq);
+	seqnum32_init(&seq);
+	end_val = seqnum32_fetch(&seq);
+	test_seqnum32_result("Test init", start_val, end_val, 0);
+
+}
+
+static void test_seqnum32_overflow(void)
+{
+	static struct seqnum32 seq = SEQNUM_INIT(UINT_MAX-1);
+	u32 start_val = seqnum32_fetch(&seq);
+	u32 end_val = seqnum32_inc_return(&seq);
+
+	test_seqnum32_result("Test UINT_MAX limit compare with (val+1)",
+			     start_val, end_val, start_val+1);
+
+	test_seqnum32_result("Test UINT_MAX limit compare with (UINT_MAX)",
+			     start_val, end_val, UINT_MAX);
+}
+
+#ifdef CONFIG_64BIT
+static inline void
+test_seqnum64_result(char *msg, u64 start, u64 end, u64 expected)
+{
+	pr_info("%s: %llu to %llu - %s\n",
+		msg, start, end,
+		((expected == end) ? "PASS" : "FAIL"));
+}
+
+static void test_seqnum64(void)
+{
+	static struct seqnum64 seq = SEQNUM_INIT(0);
+	u64 start_val = seqnum64_fetch(&seq);
+	u64 end_val;
+
+	end_val = seqnum64_inc_return(&seq);
+	test_seqnum64_result("Test fetch and increment",
+			     start_val, end_val, start_val+1);
+
+	start_val = seqnum64_fetch(&seq);
+	seqnum64_init(&seq);
+	end_val = seqnum64_fetch(&seq);
+	test_seqnum64_result("Test init", start_val, end_val, 0);
+}
+
+static void test_seqnum64_overflow(void)
+{
+	static struct seqnum64 seq = SEQNUM_INIT(UINT_MAX-1);
+	u64 start_val = seqnum64_fetch(&seq);
+	u64 end_val = seqnum64_inc_return(&seq);
+
+	test_seqnum64_result("Test UINT_MAX limit compare with (val+1)",
+			     start_val, end_val, start_val+1);
+	test_seqnum64_result("Test UINT_MAX limit compare with (UINT_MAX)",
+			     start_val, end_val, UINT_MAX);
+}
+#endif /* CONFIG_64BIT */
+
+static int __init test_seqnum_ops_init(void)
+{
+	pr_info("Start seqnum32_*() interfaces test\n");
+	test_seqnum32();
+	test_seqnum32_overflow();
+	pr_info("End seqnum32_*() interfaces test\n\n");
+
+#ifdef CONFIG_64BIT
+	pr_info("Start seqnum64_*() interfaces test\n");
+	test_seqnum64();
+	test_seqnum64_overflow();
+	pr_info("End seqnum64_*() interfaces test\n\n");
+#endif /* CONFIG_64BIT */
+
+	return 0;
+}
+
+module_init(test_seqnum_ops_init);
+
+static void __exit test_seqnum_ops_exit(void)
+{
+	pr_info("exiting.\n");
+}
+
+module_exit(test_seqnum_ops_exit);
+
+MODULE_AUTHOR("Shuah Khan <skhan@linuxfoundation.org>");
+MODULE_LICENSE("GPL v2");
-- 
2.27.0

[PATCH v2 02/13] selftests: lib:test_seqnum_ops: add new test for seqnum_ops

[Shuah Khan]

Add a new selftest for testing seqnum_ops. This test loads test_seqnum_ops
test module and unloads it. The test module runs tests and prints results
to dmesg.

Sequence Number api provides interfaces for unsigned atomic up counters
leveraging atomic_t and atomic64_t ops underneath.

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.

The atomic_t api provides a wide range of atomic operations as a base
api to implement atomic counters, bitops, spinlock interfaces. The usages
also evolved into being used for resource lifetimes and state management.
The refcount_t api was introduced to address resource lifetime problems
related to atomic_t wrapping. There is a large overlap between the
atomic_t api used for resource lifetimes and just counters, stats, and
sequence numbers. It has become difficult to differentiate between the
atomic_t usages that should be converted to refcount_t and the ones that
can be left alone. Introducing seqnum_ops to wrap the usages that are
stats, counters, sequence numbers makes it easier for tools that scan
for underflow and overflow on atomic_t usages to detect overflow and
underflows to scan just the cases that are prone to errors.

In addition, to supporting sequence number use-cases, Sequence Number Ops
helps differentiate atomic_t counter usages from atomic_t usages that guard
object lifetimes, hence prone to overflow and underflow errors from up
counting use-cases.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 Documentation/core-api/seqnum_ops.rst          |  9 +++++++++
 MAINTAINERS                                    |  1 +
 include/linux/seqnum_ops.h                     |  2 ++
 tools/testing/selftests/lib/Makefile           |  1 +
 tools/testing/selftests/lib/config             |  1 +
 tools/testing/selftests/lib/test_seqnum_ops.sh | 10 ++++++++++
 6 files changed, 24 insertions(+)
 create mode 100755 tools/testing/selftests/lib/test_seqnum_ops.sh

diff --git a/Documentation/core-api/seqnum_ops.rst b/Documentation/core-api/seqnum_ops.rst
index 10b775a9ac05..f66d796b148e 100644
--- a/Documentation/core-api/seqnum_ops.rst
+++ b/Documentation/core-api/seqnum_ops.rst
@@ -78,3 +78,12 @@ Fetched and returns current sequence number value. ::
 
         seqnum32_fetch() --> (u32) atomic_add_return(0, seqnum)
         seqnum64_fetch() --> (u64) atomic64_add_return(0, seqnum)
+
+Where are the seqnum_ops and how to use and test them?
+------------------------------------------------------
+
+.. kernel-doc:: include/linux/seqnum_ops.h
+
+Please see lib/test_seqnum_ops.c for examples usages and test module.
+Please find selftest: testing/selftests/lib/test_seqnum_ops.sh
+Please check dmesg for results after running test_seqnum_ops.sh.
diff --git a/MAINTAINERS b/MAINTAINERS
index c83a6f05610b..e6ae131836a5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15983,6 +15983,7 @@ L:	linux-kernel@vger.kernel.org
 S:	Maintained
 F:	include/linux/seqnum_ops.h
 F:	lib/test_seqnum_ops.c
+F:	tools/testing/selftests/lib/test_seqnum_ops.sh
 
 SIMPLE FIRMWARE INTERFACE (SFI)
 S:	Obsolete
diff --git a/include/linux/seqnum_ops.h b/include/linux/seqnum_ops.h
index 17d327b78050..bbf724f21e03 100644
--- a/include/linux/seqnum_ops.h
+++ b/include/linux/seqnum_ops.h
@@ -19,6 +19,8 @@
  *
  * Reference and API guide:
  *	Documentation/core-api/seqnum_ops.rst for more information.
+ *	lib/test_seqnum_ops.c - example usages and test module
+ *	tools/testing/selftests/lib/test_seqnum_ops.sh
  */
 
 #ifndef __LINUX_SEQNUM_OPS_H
diff --git a/tools/testing/selftests/lib/Makefile b/tools/testing/selftests/lib/Makefile
index a105f094676e..1818444f0e97 100644
--- a/tools/testing/selftests/lib/Makefile
+++ b/tools/testing/selftests/lib/Makefile
@@ -5,5 +5,6 @@
 all:
 
 TEST_PROGS := printf.sh bitmap.sh prime_numbers.sh strscpy.sh
+TEST_PROGS += test_seqnum_ops.sh
 
 include ../lib.mk
diff --git a/tools/testing/selftests/lib/config b/tools/testing/selftests/lib/config
index b80ee3f6e265..674ed2a2ac82 100644
--- a/tools/testing/selftests/lib/config
+++ b/tools/testing/selftests/lib/config
@@ -3,3 +3,4 @@ CONFIG_TEST_BITMAP=m
 CONFIG_PRIME_NUMBERS=m
 CONFIG_TEST_STRSCPY=m
 CONFIG_TEST_BITOPS=m
+CONFIG_TEST_SEQNUM_OPS=m
diff --git a/tools/testing/selftests/lib/test_seqnum_ops.sh b/tools/testing/selftests/lib/test_seqnum_ops.sh
new file mode 100755
index 000000000000..fdce16b220ba
--- /dev/null
+++ b/tools/testing/selftests/lib/test_seqnum_ops.sh
@@ -0,0 +1,10 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+#
+# Copyright (c) 2020 Shuah Khan <skhan@linuxfoundation.org>
+# Copyright (c) 2020 The Linux Foundation
+#
+# Tests the Sequence Number Ops interfaces using test_seqnum_ops
+# kernel module
+#
+$(dirname $0)/../kselftest/module.sh "test_seqnum_ops" test_seqnum_ops
-- 
2.27.0

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

[Matthew Wilcox]

> +==========================
> +Sequence Number Operations
> +==========================
> +
> +:Author: Shuah Khan
> +:Copyright: |copy| 2020, The Linux Foundation
> +:Copyright: |copy| 2020, Shuah Khan <skhan@linuxfoundation.org>
> +
> +Sequence Number api provides interfaces for unsigned up counters
> +leveraging atomic_t and atomic64_t ops underneath.

As I said last time you posted this, the documentation is all
back-to-front.  You're describing what it isn't, not what it is.

> +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.
> +
> +The atomic_t api provides a wide range of atomic operations as a base
> +api to implement atomic counters, bitops, spinlock interfaces. The usages
> +also evolved into being used for resource lifetimes and state management.
> +The refcount_t api was introduced to address resource lifetime problems
> +related to atomic_t wrapping. There is a large overlap between the
> +atomic_t api used for resource lifetimes and just counters, stats, and
> +sequence numbers. It has become difficult to differentiate between the
> +atomic_t usages that should be converted to refcount_t and the ones that
> +can be left alone. Introducing seqnum_ops to wrap the usages that are
> +stats, counters, sequence numbers makes it easier for tools that scan
> +for underflow and overflow on atomic_t usages to detect overflow and
> +underflows to scan just the cases that are prone to errors.
> +
> +In addition, to supporting sequence number use-cases, Sequence Number Ops
> +helps differentiate atomic_t counter usages from atomic_t usages that guard
> +object lifetimes, hence prone to overflow and underflow errors from up
> +counting use-cases.

I think almost all of this information should go into atomic_ops.rst
pushing people towards using the other APIs instead of atomic_t.
Someone who already landed here doesn't want to read about refcount_t.
They want to know what a seqnum_t is and how to use it.

> +Sequence Number Ops
> +===================
> +
> +seqnum32 and seqnum64 types use atomic_t and atomic64_t underneath to

Don't talk about the implementation.

> +leverage atomic_t api, to provide increment by 1 and return new value
> +and fetch current value interfaces necessary to support unsigned up
> +counters. ::
> +
> +        struct seqnum32 { atomic_t seqnum; };
> +        struct seqnum64 { atomic64_t seqnum; };
> +
> +Please see :ref:`Documentation/core-api/atomic_ops.rst <atomic_ops>` for
> +information on the Semantics and Behavior of Atomic operations.
> +
> +Initializers
> +------------
> +
> +Interfaces for initializing sequence numbers are write operations which
> +in turn invoke their ``ATOMIC_INIT() and atomic_set()`` counterparts ::
> +
> +        #define SEQNUM_INIT(i)    { .seqnum = ATOMIC_INIT(i) }
> +        seqnum32_init() --> atomic_set() to 0
> +        seqnum64_init() --> atomic64_set() to 0
> +
> +Increment interface
> +-------------------
> +
> +Increments sequence number and returns the new value. ::
> +
> +        seqnum32_inc_return() --> (u32) atomic_inc_return(seqnum)
> +        seqnum64_inc_return() --> (u64) atomic64_inc_return(seqnum)

seqnum_inc() should just return the new value -- seqnum_inc_return is
too verbose.  And do we not need a seqnum_add()?

Also, this would be a good point to talk about behaviour on overflow.

> +Fetch interface
> +---------------
> +
> +Fetched and returns current sequence number value. ::
> +
> +        seqnum32_fetch() --> (u32) atomic_add_return(0, seqnum)
> +        seqnum64_fetch() --> (u64) atomic64_add_return(0, seqnum)

Er ... why would you force an atomic operation instead of atomic_read()?

> diff --git a/MAINTAINERS b/MAINTAINERS
> index b516bb34a8d5..c83a6f05610b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15977,6 +15977,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..17d327b78050
> --- /dev/null
> +++ b/include/linux/seqnum_ops.h
> @@ -0,0 +1,116 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * seqnum_ops.h - Interfaces for sequential and statistical counters.
> + *
> + * Copyright (c) 2020 Shuah Khan <skhan@linuxfoundation.org>
> + * Copyright (c) 2020 The Linux Foundation
> + *
> + * Sequence Number functions provide support for unsgined atomic up
> + * counters.
> + *
> + * The interface provides:
> + * seqnumu32 & seqnumu64 functions:
> + *	initialization
> + *	increment and return
> + *	fetch 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 - Sequential/Statistical atomic counter
> + * @seqnum: atomic_t
> + *
> + **/
> +struct seqnum32 {
> +	atomic_t seqnum;
> +};
> +
> +#define SEQNUM_INIT(i)		{ .seqnum = ATOMIC_INIT(i) }
> +
> +/*
> + * seqnum32_init() - initialize seqnum value
> + * @seq: struct seqnum32 pointer
> + *
> + */
> +static inline void seqnum32_init(struct seqnum32 *seq)
> +{
> +	atomic_set(&seq->seqnum, 0);
> +}
> +
> +/*
> + * seqnum32_inc_return() - increment seqnum value and return the new value
> + * @seq: struct seqnum32 pointer
> + *
> + * Return u32
> + */
> +static inline u32 seqnum32_inc_return(struct seqnum32 *seq)
> +{
> +	return (u32) atomic_inc_return(&seq->seqnum);
> +}
> +
> +/*
> + * seqnum32_fetch() - return the current value
> + * @seq: struct seqnum32 pointer
> + *
> + * Return u32
> + */
> +static inline u32 seqnum32_fetch(struct seqnum32 *seq)
> +{
> +	return (u32) atomic_add_return(0, &seq->seqnum);
> +}
> +
> +#ifdef CONFIG_64BIT
> +/* atomic64_t is defined in CONFIG_64BIT in include/linux/types.h */
> +/*
> + * struct seqnum64 - Sequential/Statistical atomic counter
> + * @seq: atomic64_t
> + *
> + */
> +struct seqnum64 {
> +	atomic64_t seqnum;
> +};
> +
> +/*
> + * seqnum64_init() - initialize seqnum value
> + * @seq: struct seqnum64 pointer
> + *
> + */
> +static inline void seqnum64_init(struct seqnum64 *seq)
> +{
> +	atomic64_set(&seq->seqnum, 0);
> +}
> +
> +/*
> + * seqnum64_inc() - increment seqnum value and return the new value
> + * @seq: struct seqnum64 pointer
> + *
> + * Return u64
> + */
> +static inline u64 seqnum64_inc_return(struct seqnum64 *seq)
> +{
> +	return (u64) atomic64_inc_return(&seq->seqnum);
> +}
> +
> +/*
> + * seqnum64_fetch() - return the current value
> + * @seq: struct seqnum64 pointer
> + *
> + * Return u64
> + */
> +static inline u64 seqnum64_fetch(struct seqnum64 *seq)
> +{
> +	return (u64) atomic64_add_return(0, &seq->seqnum);
> +}
> +#endif /* CONFIG_64BIT */
> +
> +#endif /* __LINUX_SEQNUM_OPS_H */
> diff --git a/lib/Kconfig b/lib/Kconfig
> index b46a9fd122c8..c362c2713e11 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -663,6 +663,15 @@ config OBJAGG
>  config STRING_SELFTEST
>  	tristate "Test string functions"
>  
> +config TEST_SEQNUM_OPS
> +	tristate "Test Sequence Number Ops API"
> +	help
> +	   A test module for Sequence Number Ops API. A corresponding
> +	   selftest can be used to test the Seqnum Ops API. Select this
> +	   for testing Sequence Number Ops API.
> +
> +	   See Documentation/core-api/seqnum_ops.rst
> +
>  endmenu
>  
>  config GENERIC_IOREMAP
> diff --git a/lib/Makefile b/lib/Makefile
> index ce45af50983a..7d17c25e4d73 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -101,6 +101,7 @@ obj-$(CONFIG_TEST_MEMINIT) += test_meminit.o
>  obj-$(CONFIG_TEST_LOCKUP) += test_lockup.o
>  obj-$(CONFIG_TEST_HMM) += test_hmm.o
>  obj-$(CONFIG_TEST_FREE_PAGES) += test_free_pages.o
> +obj-$(CONFIG_TEST_SEQNUM_OPS) += test_seqnum_ops.o
>  
>  #
>  # CFLAGS for compiling floating point code inside the kernel. x86/Makefile turns
> diff --git a/lib/test_seqnum_ops.c b/lib/test_seqnum_ops.c
> new file mode 100644
> index 000000000000..6e58b6a0a2be
> --- /dev/null
> +++ b/lib/test_seqnum_ops.c
> @@ -0,0 +1,119 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * test_seqnum_ops.c - Kernel module for testing Seqnum API
> + *
> + * Copyright (c) 2020 Shuah Khan <skhan@linuxfoundation.org>
> + * Copyright (c) 2020 The Linux Foundation
> + *
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/module.h>
> +#include <linux/seqnum_ops.h>
> +
> +static inline void
> +test_seqnum32_result(char *msg, u32 start, u32 end, u32 expected)
> +{
> +	pr_info("%s: %u to %u - %s\n",
> +		msg, start, end,
> +		((expected == end) ? "PASS" : "FAIL"));
> +}
> +
> +
> +static void test_seqnum32(void)
> +{
> +	static struct seqnum32 seq = SEQNUM_INIT(0);
> +	u32 start_val = seqnum32_fetch(&seq);
> +	u32 end_val;
> +
> +	end_val = seqnum32_inc_return(&seq);
> +	test_seqnum32_result("Test fetch and increment",
> +			     start_val, end_val, start_val+1);
> +
> +	start_val = seqnum32_fetch(&seq);
> +	seqnum32_init(&seq);
> +	end_val = seqnum32_fetch(&seq);
> +	test_seqnum32_result("Test init", start_val, end_val, 0);
> +
> +}
> +
> +static void test_seqnum32_overflow(void)
> +{
> +	static struct seqnum32 seq = SEQNUM_INIT(UINT_MAX-1);
> +	u32 start_val = seqnum32_fetch(&seq);
> +	u32 end_val = seqnum32_inc_return(&seq);
> +
> +	test_seqnum32_result("Test UINT_MAX limit compare with (val+1)",
> +			     start_val, end_val, start_val+1);
> +
> +	test_seqnum32_result("Test UINT_MAX limit compare with (UINT_MAX)",
> +			     start_val, end_val, UINT_MAX);
> +}
> +
> +#ifdef CONFIG_64BIT
> +static inline void
> +test_seqnum64_result(char *msg, u64 start, u64 end, u64 expected)
> +{
> +	pr_info("%s: %llu to %llu - %s\n",
> +		msg, start, end,
> +		((expected == end) ? "PASS" : "FAIL"));
> +}
> +
> +static void test_seqnum64(void)
> +{
> +	static struct seqnum64 seq = SEQNUM_INIT(0);
> +	u64 start_val = seqnum64_fetch(&seq);
> +	u64 end_val;
> +
> +	end_val = seqnum64_inc_return(&seq);
> +	test_seqnum64_result("Test fetch and increment",
> +			     start_val, end_val, start_val+1);
> +
> +	start_val = seqnum64_fetch(&seq);
> +	seqnum64_init(&seq);
> +	end_val = seqnum64_fetch(&seq);
> +	test_seqnum64_result("Test init", start_val, end_val, 0);
> +}
> +
> +static void test_seqnum64_overflow(void)
> +{
> +	static struct seqnum64 seq = SEQNUM_INIT(UINT_MAX-1);
> +	u64 start_val = seqnum64_fetch(&seq);
> +	u64 end_val = seqnum64_inc_return(&seq);
> +
> +	test_seqnum64_result("Test UINT_MAX limit compare with (val+1)",
> +			     start_val, end_val, start_val+1);
> +	test_seqnum64_result("Test UINT_MAX limit compare with (UINT_MAX)",
> +			     start_val, end_val, UINT_MAX);
> +}
> +#endif /* CONFIG_64BIT */
> +
> +static int __init test_seqnum_ops_init(void)
> +{
> +	pr_info("Start seqnum32_*() interfaces test\n");
> +	test_seqnum32();
> +	test_seqnum32_overflow();
> +	pr_info("End seqnum32_*() interfaces test\n\n");
> +
> +#ifdef CONFIG_64BIT
> +	pr_info("Start seqnum64_*() interfaces test\n");
> +	test_seqnum64();
> +	test_seqnum64_overflow();
> +	pr_info("End seqnum64_*() interfaces test\n\n");
> +#endif /* CONFIG_64BIT */
> +
> +	return 0;
> +}
> +
> +module_init(test_seqnum_ops_init);
> +
> +static void __exit test_seqnum_ops_exit(void)
> +{
> +	pr_info("exiting.\n");
> +}
> +
> +module_exit(test_seqnum_ops_exit);
> +
> +MODULE_AUTHOR("Shuah Khan <skhan@linuxfoundation.org>");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.27.0
> 

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

[Peter Zijlstra]

On Fri, Nov 13, 2020 at 09:03:27PM +0000, Matthew Wilcox wrote:
> I think almost all of this information should go into atomic_ops.rst

No, we should delete atomic_ops.rst. It's bitrotted nonsense. The only
reason it still exists is because I can't seem to get around to
verifying we've actually covered everything in the new documentation:

	Documentation/atomic_*.txt

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

[Peter Zijlstra]

On Fri, Nov 13, 2020 at 10:46:03AM -0700, Shuah Khan wrote:

> +Increment interface
> +-------------------
> +
> +Increments sequence number and returns the new value. ::
> +
> +        seqnum32_inc_return() --> (u32) atomic_inc_return(seqnum)
> +        seqnum64_inc_return() --> (u64) atomic64_inc_return(seqnum)

Did you think about the ordering?

> +Fetch interface
> +---------------
> +
> +Fetched and returns current sequence number value. ::
> +
> +        seqnum32_fetch() --> (u32) atomic_add_return(0, seqnum)
> +        seqnum64_fetch() --> (u64) atomic64_add_return(0, seqnum)

That's horrible. Please explain how that is not broken garbage.

Per the fact that it is atomic, nothing prevents the counter being
incremented concurrently. There is no such thing as a 'current' sequence
number.

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

[Peter Zijlstra]

On Mon, Nov 16, 2020 at 03:49:14PM +0100, Peter Zijlstra wrote:
> On Fri, Nov 13, 2020 at 09:03:27PM +0000, Matthew Wilcox wrote:
> > I think almost all of this information should go into atomic_ops.rst
> 
> No, we should delete atomic_ops.rst. It's bitrotted nonsense. The only

Sod it, I'll just queue a deletion. That'll stop people from trying to
update the trainwreck.

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

[Shuah Khan]

On 11/16/20 7:58 AM, Peter Zijlstra wrote:
> On Mon, Nov 16, 2020 at 03:49:14PM +0100, Peter Zijlstra wrote:
>> On Fri, Nov 13, 2020 at 09:03:27PM +0000, Matthew Wilcox wrote:
>>> I think almost all of this information should go into atomic_ops.rst
>>
>> No, we should delete atomic_ops.rst. It's bitrotted nonsense. The only
> 
> Sod it, I'll just queue a deletion. That'll stop people from trying to
> update the trainwreck.
> 

Please don't delete the document. This is one of the documents that has
the information to make decisions about the api usage.

atomic_t information is spread out in various Doc directories. It should
be consolidated, instead of removing files.

thanks,
-- Shuah

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

[Peter Zijlstra]

On Tue, Nov 17, 2020 at 07:50:15AM -0700, Shuah Khan wrote:
> On 11/16/20 7:58 AM, Peter Zijlstra wrote:
> > On Mon, Nov 16, 2020 at 03:49:14PM +0100, Peter Zijlstra wrote:
> > > On Fri, Nov 13, 2020 at 09:03:27PM +0000, Matthew Wilcox wrote:
> > > > I think almost all of this information should go into atomic_ops.rst
> > > 
> > > No, we should delete atomic_ops.rst. It's bitrotted nonsense. The only
> > 
> > Sod it, I'll just queue a deletion. That'll stop people from trying to
> > update the trainwreck.
> > 
> 
> Please don't delete the document. This is one of the documents that has
> the information to make decisions about the api usage.
> 
> atomic_t information is spread out in various Doc directories. It should
> be consolidated, instead of removing files.

It is gone.. Please read Documentation/atomic_*.txt if you want to know
about atomic_t or bitops.

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

[Shuah Khan]

On 11/16/20 7:53 AM, Peter Zijlstra wrote:
> On Fri, Nov 13, 2020 at 10:46:03AM -0700, Shuah Khan wrote:
> 
>> +Increment interface
>> +-------------------
>> +
>> +Increments sequence number and returns the new value. ::
>> +
>> +        seqnum32_inc_return() --> (u32) atomic_inc_return(seqnum)
>> +        seqnum64_inc_return() --> (u64) atomic64_inc_return(seqnum)
> 
> Did you think about the ordering?
> 

Looking at atomic_t.txt _inc_return() can be fully ordered without
loosing or making the intermediate state visible. This is good for
this sequence number use-case. Is there something I am overlooking?

>> +Fetch interface
>> +---------------
>> +
>> +Fetched and returns current sequence number value. ::
>> +
>> +        seqnum32_fetch() --> (u32) atomic_add_return(0, seqnum)
>> +        seqnum64_fetch() --> (u64) atomic64_add_return(0, seqnum)
> 
> That's horrible. Please explain how that is not broken garbage.
> 
> Per the fact that it is atomic, nothing prevents the counter being
> incremented concurrently. There is no such thing as a 'current' sequence
> number.
> 

Correct. Some usages of this _fecth() in this patch series are for
printing sequence numbers in debug message and others are stats.

I will review the patches in this series and drop the ones that use
read/fetch - the reason being sequence numbers are strictly up counters
and don't need read/fetch.

thanks,
-- Shuah

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

[Shuah Khan]

On 11/13/20 2:03 PM, Matthew Wilcox wrote:
>> +==========================
>> +Sequence Number Operations
>> +==========================
>> +
>> +:Author: Shuah Khan
>> +:Copyright: |copy| 2020, The Linux Foundation
>> +:Copyright: |copy| 2020, Shuah Khan <skhan@linuxfoundation.org>
>> +
>> +Sequence Number api provides interfaces for unsigned up counters
>> +leveraging atomic_t and atomic64_t ops underneath.
> 
> As I said last time you posted this, the documentation is all
> back-to-front.  You're describing what it isn't, not what it is.
>

I will rephrase it to read better.

>> +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.
>> +
>> +The atomic_t api provides a wide range of atomic operations as a base
>> +api to implement atomic counters, bitops, spinlock interfaces. The usages
>> +also evolved into being used for resource lifetimes and state management.
>> +The refcount_t api was introduced to address resource lifetime problems
>> +related to atomic_t wrapping. There is a large overlap between the
>> +atomic_t api used for resource lifetimes and just counters, stats, and
>> +sequence numbers. It has become difficult to differentiate between the
>> +atomic_t usages that should be converted to refcount_t and the ones that
>> +can be left alone. Introducing seqnum_ops to wrap the usages that are
>> +stats, counters, sequence numbers makes it easier for tools that scan
>> +for underflow and overflow on atomic_t usages to detect overflow and
>> +underflows to scan just the cases that are prone to errors.
>> +
>> +In addition, to supporting sequence number use-cases, Sequence Number Ops
>> +helps differentiate atomic_t counter usages from atomic_t usages that guard
>> +object lifetimes, hence prone to overflow and underflow errors from up
>> +counting use-cases.
> 
> I think almost all of this information should go into atomic_ops.rst
> pushing people towards using the other APIs instead of atomic_t.
> Someone who already landed here doesn't want to read about refcount_t.
> They want to know what a seqnum_t is and how to use it.
> 

Looks like this is resolved with atomic_ops.rst is now gone.

>> +Sequence Number Ops
>> +===================
>> +
>> +seqnum32 and seqnum64 types use atomic_t and atomic64_t underneath to
> 
> Don't talk about the implementation.
> 
>> +leverage atomic_t api, to provide increment by 1 and return new value
>> +and fetch current value interfaces necessary to support unsigned up
>> +counters. ::
>> +
>> +        struct seqnum32 { atomic_t seqnum; };
>> +        struct seqnum64 { atomic64_t seqnum; };
>> +
>> +Please see :ref:`Documentation/core-api/atomic_ops.rst <atomic_ops>` for
>> +information on the Semantics and Behavior of Atomic operations.
>> +
>> +Initializers
>> +------------
>> +
>> +Interfaces for initializing sequence numbers are write operations which
>> +in turn invoke their ``ATOMIC_INIT() and atomic_set()`` counterparts ::
>> +
>> +        #define SEQNUM_INIT(i)    { .seqnum = ATOMIC_INIT(i) }
>> +        seqnum32_init() --> atomic_set() to 0
>> +        seqnum64_init() --> atomic64_set() to 0
>> +
>> +Increment interface
>> +-------------------
>> +
>> +Increments sequence number and returns the new value. ::
>> +
>> +        seqnum32_inc_return() --> (u32) atomic_inc_return(seqnum)
>> +        seqnum64_inc_return() --> (u64) atomic64_inc_return(seqnum)
> 
> 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
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.

No need for atomic_add() - inc_return() is sufficient for this use-case.

> Also, this would be a good point to talk about behaviour on overflow.
> 

I can add some overflow information.

thanks,
-- Shuah

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

[Matthew Wilcox]

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.

> 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.

> 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.

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

[Shuah Khan]

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

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

[Shuah Khan]

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