* [PATCH v2 00/13] Introduce seqnum_ops @ 2020-11-13 17:46 Shuah Khan 2020-11-13 17:46 ` [PATCH v2 01/13] seqnum_ops: Introduce Sequence Number Ops Shuah Khan 2020-11-13 17:46 ` [PATCH v2 02/13] selftests: lib:test_seqnum_ops: add new test for seqnum_ops Shuah Khan 0 siblings, 2 replies; 14+ messages in thread From: Shuah Khan @ 2020-11-13 17:46 UTC (permalink / raw) To: corbet, keescook, gregkh, peterz, rafael, lenb, james.morse, tony.luck, bp, minyard, arnd, mchehab, rric, valentina.manea.m, shuah, zohar, dmitry.kasatkin, jmorris, serge Cc: Shuah Khan, linux-doc, linux-kernel, linux-kselftest, linux-acpi, openipmi-developer, linux-edac, linux-usb, linux-integrity, linux-security-module 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 01/13] seqnum_ops: Introduce Sequence Number Ops 2020-11-13 17:46 [PATCH v2 00/13] Introduce seqnum_ops Shuah Khan @ 2020-11-13 17:46 ` Shuah Khan 2020-11-13 21:03 ` Matthew Wilcox 2020-11-16 14:53 ` Peter Zijlstra 2020-11-13 17:46 ` [PATCH v2 02/13] selftests: lib:test_seqnum_ops: add new test for seqnum_ops Shuah Khan 1 sibling, 2 replies; 14+ messages in thread From: Shuah Khan @ 2020-11-13 17:46 UTC (permalink / raw) To: corbet, keescook, gregkh, peterz; +Cc: Shuah Khan, linux-doc, linux-kernel 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 01/13] seqnum_ops: Introduce Sequence Number Ops 2020-11-13 17:46 ` [PATCH v2 01/13] seqnum_ops: Introduce Sequence Number Ops Shuah Khan @ 2020-11-13 21:03 ` Matthew Wilcox 2020-11-16 14:49 ` Peter Zijlstra 2020-11-17 16:34 ` Shuah Khan 2020-11-16 14:53 ` Peter Zijlstra 1 sibling, 2 replies; 14+ messages in thread From: Matthew Wilcox @ 2020-11-13 21:03 UTC (permalink / raw) To: Shuah Khan; +Cc: corbet, keescook, gregkh, peterz, linux-doc, linux-kernel > +========================== > +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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 01/13] seqnum_ops: Introduce Sequence Number Ops 2020-11-13 21:03 ` Matthew Wilcox @ 2020-11-16 14:49 ` Peter Zijlstra 2020-11-16 14:58 ` Peter Zijlstra 2020-11-17 16:34 ` Shuah Khan 1 sibling, 1 reply; 14+ messages in thread From: Peter Zijlstra @ 2020-11-16 14:49 UTC (permalink / raw) To: Matthew Wilcox Cc: Shuah Khan, corbet, keescook, gregkh, linux-doc, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 01/13] seqnum_ops: Introduce Sequence Number Ops 2020-11-16 14:49 ` Peter Zijlstra @ 2020-11-16 14:58 ` Peter Zijlstra 2020-11-17 14:50 ` Shuah Khan 0 siblings, 1 reply; 14+ messages in thread From: Peter Zijlstra @ 2020-11-16 14:58 UTC (permalink / raw) To: Matthew Wilcox Cc: Shuah Khan, corbet, keescook, gregkh, linux-doc, linux-kernel 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 01/13] seqnum_ops: Introduce Sequence Number Ops 2020-11-16 14:58 ` Peter Zijlstra @ 2020-11-17 14:50 ` Shuah Khan 2020-11-17 15:21 ` Peter Zijlstra 0 siblings, 1 reply; 14+ messages in thread From: Shuah Khan @ 2020-11-17 14:50 UTC (permalink / raw) To: Peter Zijlstra, Matthew Wilcox Cc: corbet, keescook, gregkh, linux-doc, linux-kernel, 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 01/13] seqnum_ops: Introduce Sequence Number Ops 2020-11-17 14:50 ` Shuah Khan @ 2020-11-17 15:21 ` Peter Zijlstra 0 siblings, 0 replies; 14+ messages in thread From: Peter Zijlstra @ 2020-11-17 15:21 UTC (permalink / raw) To: Shuah Khan Cc: Matthew Wilcox, corbet, keescook, gregkh, linux-doc, linux-kernel 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 01/13] seqnum_ops: Introduce Sequence Number Ops 2020-11-13 21:03 ` Matthew Wilcox 2020-11-16 14:49 ` Peter Zijlstra @ 2020-11-17 16:34 ` Shuah Khan 2020-11-17 17:38 ` Matthew Wilcox 1 sibling, 1 reply; 14+ messages in thread From: Shuah Khan @ 2020-11-17 16:34 UTC (permalink / raw) To: Matthew Wilcox Cc: corbet, keescook, gregkh, peterz, linux-doc, linux-kernel, 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 01/13] seqnum_ops: Introduce Sequence Number Ops 2020-11-17 16:34 ` Shuah Khan @ 2020-11-17 17:38 ` Matthew Wilcox 2020-11-17 18:23 ` Shuah Khan 2020-11-17 18:24 ` Shuah Khan 0 siblings, 2 replies; 14+ messages in thread From: Matthew Wilcox @ 2020-11-17 17:38 UTC (permalink / raw) To: Shuah Khan; +Cc: corbet, keescook, gregkh, peterz, linux-doc, linux-kernel 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 01/13] seqnum_ops: Introduce Sequence Number Ops 2020-11-17 17:38 ` Matthew Wilcox @ 2020-11-17 18:23 ` Shuah Khan 2020-11-17 18:24 ` Shuah Khan 1 sibling, 0 replies; 14+ messages in thread From: Shuah Khan @ 2020-11-17 18:23 UTC (permalink / raw) To: Matthew Wilcox Cc: corbet, keescook, gregkh, peterz, linux-doc, linux-kernel, 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 01/13] seqnum_ops: Introduce Sequence Number Ops 2020-11-17 17:38 ` Matthew Wilcox 2020-11-17 18:23 ` Shuah Khan @ 2020-11-17 18:24 ` Shuah Khan 1 sibling, 0 replies; 14+ messages in thread From: Shuah Khan @ 2020-11-17 18:24 UTC (permalink / raw) To: Matthew Wilcox Cc: corbet, keescook, gregkh, peterz, linux-doc, linux-kernel, 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 01/13] seqnum_ops: Introduce Sequence Number Ops 2020-11-13 17:46 ` [PATCH v2 01/13] seqnum_ops: Introduce Sequence Number Ops Shuah Khan 2020-11-13 21:03 ` Matthew Wilcox @ 2020-11-16 14:53 ` Peter Zijlstra 2020-11-17 16:15 ` Shuah Khan 1 sibling, 1 reply; 14+ messages in thread From: Peter Zijlstra @ 2020-11-16 14:53 UTC (permalink / raw) To: Shuah Khan; +Cc: corbet, keescook, gregkh, linux-doc, linux-kernel 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 01/13] seqnum_ops: Introduce Sequence Number Ops 2020-11-16 14:53 ` Peter Zijlstra @ 2020-11-17 16:15 ` Shuah Khan 0 siblings, 0 replies; 14+ messages in thread From: Shuah Khan @ 2020-11-17 16:15 UTC (permalink / raw) To: Peter Zijlstra; +Cc: corbet, keescook, gregkh, linux-doc, linux-kernel, skhan 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 02/13] selftests: lib:test_seqnum_ops: add new test for seqnum_ops 2020-11-13 17:46 [PATCH v2 00/13] Introduce seqnum_ops Shuah Khan 2020-11-13 17:46 ` [PATCH v2 01/13] seqnum_ops: Introduce Sequence Number Ops Shuah Khan @ 2020-11-13 17:46 ` Shuah Khan 1 sibling, 0 replies; 14+ messages in thread From: Shuah Khan @ 2020-11-13 17:46 UTC (permalink / raw) To: corbet, keescook, gregkh, peterz Cc: Shuah Khan, linux-doc, linux-kernel, linux-kselftest 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-11-17 18:24 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-11-13 17:46 [PATCH v2 00/13] Introduce seqnum_ops Shuah Khan 2020-11-13 17:46 ` [PATCH v2 01/13] seqnum_ops: Introduce Sequence Number Ops Shuah Khan 2020-11-13 21:03 ` Matthew Wilcox 2020-11-16 14:49 ` Peter Zijlstra 2020-11-16 14:58 ` Peter Zijlstra 2020-11-17 14:50 ` Shuah Khan 2020-11-17 15:21 ` Peter Zijlstra 2020-11-17 16:34 ` Shuah Khan 2020-11-17 17:38 ` Matthew Wilcox 2020-11-17 18:23 ` Shuah Khan 2020-11-17 18:24 ` Shuah Khan 2020-11-16 14:53 ` Peter Zijlstra 2020-11-17 16:15 ` Shuah Khan 2020-11-13 17:46 ` [PATCH v2 02/13] selftests: lib:test_seqnum_ops: add new test for seqnum_ops Shuah Khan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).