Introduce Simple atomic and non-atomic counters

counter_atomic* is introduced to be used when a variable is used as a simple counter and doesn't guard object lifetimes. This clearly differentiates atomic_t usages that guard object lifetimes.

counter_atomic* variables will wrap around to 0 when it overflows and should not be used to guard resource lifetimes, device usage and open counts that control state changes, and pm states.

stats tracks per-process binder statistics. Unsure if there is a chance of this overflowing, other than stats getting reset to 0. Convert it to use counter_atomic.

binder_transaction_log:cur is used to keep track of the current log entry location. Overflow is handled in the code. Since it is used as a counter, convert it to use counter_atomic32.

This conversion doesn't change the overflow wrap around behavior.

Signed-off-by Shuah Khan <[email protected]>

---
 drivers/android/binder.c          | 41 ++++++++++++++++---------------
 drivers/android/binder_internal.h |  3 ++-
 2 files changed, 23 insertions(+), 21 deletions(-)

drivers/android/binder.c +21 -20

index f936530a19b0..52175cd6a62b 100644
@@ -66,6 +66,7 @@
 #include <linux/syscalls.h>
 #include <linux/task_work.h>
 #include <linux/sizes.h>
+#include <linux/counters.h>
 
 #include <uapi/linux/android/binder.h>
 #include <uapi/linux/android/binderfs.h>

@@ -172,22 +173,22 @@ enum binder_stat_types {
 };
 
 struct binder_stats {
-	atomic_t br[_IOC_NR(BR_FAILED_REPLY) + 1];
-	atomic_t bc[_IOC_NR(BC_REPLY_SG) + 1];
-	atomic_t obj_created[BINDER_STAT_COUNT];
-	atomic_t obj_deleted[BINDER_STAT_COUNT];
+	struct counter_atomic32 br[_IOC_NR(BR_FAILED_REPLY) + 1];
+	struct counter_atomic32 bc[_IOC_NR(BC_REPLY_SG) + 1];
+	struct counter_atomic32 obj_created[BINDER_STAT_COUNT];
+	struct counter_atomic32 obj_deleted[BINDER_STAT_COUNT];
 };
 
 static struct binder_stats binder_stats;
 
 static inline void binder_stats_deleted(enum binder_stat_types type)
 {
-	atomic_inc(&binder_stats.obj_deleted[type]);
+	counter_atomic32_inc(&binder_stats.obj_deleted[type]);
 }
 
 static inline void binder_stats_created(enum binder_stat_types type)
 {
-	atomic_inc(&binder_stats.obj_created[type]);
+	counter_atomic32_inc(&binder_stats.obj_created[type]);
 }
 
 struct binder_transaction_log binder_transaction_log;

@@ -197,7 +198,7 @@ static struct binder_transaction_log_entry *binder_transaction_log_add(
 	struct binder_transaction_log *log)
 {
 	struct binder_transaction_log_entry *e;
-	unsigned int cur = atomic_inc_return(&log->cur);
+	unsigned int cur = counter_atomic32_inc_return(&log->cur);
 
 	if (cur >= ARRAY_SIZE(log->entry))
 		log->full = true;

@@ -3615,9 +3616,9 @@ static int binder_thread_write(struct binder_proc *proc,
 		ptr += sizeof(uint32_t);
 		trace_binder_command(cmd);
 		if (_IOC_NR(cmd) < ARRAY_SIZE(binder_stats.bc)) {
-			atomic_inc(&binder_stats.bc[_IOC_NR(cmd)]);
-			atomic_inc(&proc->stats.bc[_IOC_NR(cmd)]);
-			atomic_inc(&thread->stats.bc[_IOC_NR(cmd)]);
+			counter_atomic32_inc(&binder_stats.bc[_IOC_NR(cmd)]);
+			counter_atomic32_inc(&proc->stats.bc[_IOC_NR(cmd)]);
+			counter_atomic32_inc(&thread->stats.bc[_IOC_NR(cmd)]);
 		}
 		switch (cmd) {
 		case BC_INCREFS:

@@ -4047,9 +4048,9 @@ static void binder_stat_br(struct binder_proc *proc,
 {
 	trace_binder_return(cmd);
 	if (_IOC_NR(cmd) < ARRAY_SIZE(binder_stats.br)) {
-		atomic_inc(&binder_stats.br[_IOC_NR(cmd)]);
-		atomic_inc(&proc->stats.br[_IOC_NR(cmd)]);
-		atomic_inc(&thread->stats.br[_IOC_NR(cmd)]);
+		counter_atomic32_inc(&binder_stats.br[_IOC_NR(cmd)]);
+		counter_atomic32_inc(&proc->stats.br[_IOC_NR(cmd)]);
+		counter_atomic32_inc(&thread->stats.br[_IOC_NR(cmd)]);
 	}
 }
 

@@ -5841,7 +5842,7 @@ static void print_binder_stats(struct seq_file *m, const char *prefix,
 	BUILD_BUG_ON(ARRAY_SIZE(stats->bc) !=
 		     ARRAY_SIZE(binder_command_strings));
 	for (i = 0; i < ARRAY_SIZE(stats->bc); i++) {
-		int temp = atomic_read(&stats->bc[i]);
+		int temp = counter_atomic32_read(&stats->bc[i]);
 
 		if (temp)
 			seq_printf(m, "%s%s: %d\n", prefix,

@@ -5851,7 +5852,7 @@ static void print_binder_stats(struct seq_file *m, const char *prefix,
 	BUILD_BUG_ON(ARRAY_SIZE(stats->br) !=
 		     ARRAY_SIZE(binder_return_strings));
 	for (i = 0; i < ARRAY_SIZE(stats->br); i++) {
-		int temp = atomic_read(&stats->br[i]);
+		int temp = counter_atomic32_read(&stats->br[i]);
 
 		if (temp)
 			seq_printf(m, "%s%s: %d\n", prefix,

@@ -5863,8 +5864,8 @@ static void print_binder_stats(struct seq_file *m, const char *prefix,
 	BUILD_BUG_ON(ARRAY_SIZE(stats->obj_created) !=
 		     ARRAY_SIZE(stats->obj_deleted));
 	for (i = 0; i < ARRAY_SIZE(stats->obj_created); i++) {
-		int created = atomic_read(&stats->obj_created[i]);
-		int deleted = atomic_read(&stats->obj_deleted[i]);
+		int created = counter_atomic32_read(&stats->obj_created[i]);
+		int deleted = counter_atomic32_read(&stats->obj_deleted[i]);
 
 		if (created || deleted)
 			seq_printf(m, "%s%s: active %d total %d\n",

@@ -6054,7 +6055,7 @@ static void print_binder_transaction_log_entry(struct seq_file *m,
 int binder_transaction_log_show(struct seq_file *m, void *unused)
 {
 	struct binder_transaction_log *log = m->private;
-	unsigned int log_cur = atomic_read(&log->cur);
+	unsigned int log_cur = counter_atomic32_read(&log->cur);
 	unsigned int count;
 	unsigned int cur;
 	int i;

@@ -6124,8 +6125,8 @@ static int __init binder_init(void)
 	if (ret)
 		return ret;
 
-	atomic_set(&binder_transaction_log.cur, ~0U);
-	atomic_set(&binder_transaction_log_failed.cur, ~0U);
+	counter_atomic32_set(&binder_transaction_log.cur, ~0U);
+	counter_atomic32_set(&binder_transaction_log_failed.cur, ~0U);
 
 	binder_debugfs_dir_entry_root = debugfs_create_dir("binder", NULL);
 	if (binder_debugfs_dir_entry_root)

drivers/android/binder_internal.h +2 -1

index 283d3cb9c16e..c77960c01430 100644
@@ -12,6 +12,7 @@
 #include <linux/stddef.h>
 #include <linux/types.h>
 #include <linux/uidgid.h>
+#include <linux/counters.h>
 
 struct binder_context {
 	struct binder_node *binder_context_mgr_node;

@@ -136,7 +137,7 @@ struct binder_transaction_log_entry {
 };
 
 struct binder_transaction_log {
-	atomic_t cur;
+	struct counter_atomic32 cur;
 	bool full;
 	struct binder_transaction_log_entry entry[32];
 };

Signature · 1 line

2.25.1

counter_atomic* is introduced to be used when a variable is used as a simple counter and doesn't guard object lifetimes. This clearly differentiates atomic_t usages that guard object lifetimes.

counter_atomic* variables will wrap around to 0 when it overflows and should not be used to guard resource lifetimes, device usage and open counts that control state changes, and pm states.

atomic_t variables used for stats are atomic counters. Overflow will wrap around and reset the stats and no change with the conversion.

Convert them to use counter_atomic32.

Signed-off-by Shuah Khan <[email protected]>

---
 drivers/char/ipmi/ipmi_msghandler.c | 9 +++++----
 drivers/char/ipmi/ipmi_si_intf.c    | 9 +++++----
 2 files changed, 10 insertions(+), 8 deletions(-)

drivers/char/ipmi/ipmi_msghandler.c +5 -4

index 737c0b6b24ea..36c0b1be22fb 100644
@@ -34,6 +34,7 @@
 #include <linux/uuid.h>
 #include <linux/nospec.h>
 #include <linux/vmalloc.h>
+#include <linux/counters.h>
 
 #define IPMI_DRIVER_VERSION "39.2"
 

@@ -584,7 +585,7 @@ struct ipmi_smi {
 	struct ipmi_my_addrinfo addrinfo[IPMI_MAX_CHANNELS];
 	bool channels_ready;
 
-	atomic_t stats[IPMI_NUM_STATS];
+	struct counter_atomic32 stats[IPMI_NUM_STATS];
 
 	/*
 	 * run_to_completion duplicate of smb_info, smi_info

@@ -630,9 +631,9 @@ static LIST_HEAD(smi_watchers);
 static DEFINE_MUTEX(smi_watchers_mutex);
 
 #define ipmi_inc_stat(intf, stat) \
-	atomic_inc(&(intf)->stats[IPMI_STAT_ ## stat])
+	counter_atomic32_inc(&(intf)->stats[IPMI_STAT_ ## stat])
 #define ipmi_get_stat(intf, stat) \
-	((unsigned int) atomic_read(&(intf)->stats[IPMI_STAT_ ## stat]))
+	((unsigned int) counter_atomic32_read(&(intf)->stats[IPMI_STAT_ ## stat]))
 
 static const char * const addr_src_to_str[] = {
 	"invalid", "hotmod", "hardcoded", "SPMI", "ACPI", "SMBIOS", "PCI",

@@ -3448,7 +3449,7 @@ int ipmi_add_smi(struct module         *owner,
 	INIT_LIST_HEAD(&intf->cmd_rcvrs);
 	init_waitqueue_head(&intf->waitq);
 	for (i = 0; i < IPMI_NUM_STATS; i++)
-		atomic_set(&intf->stats[i], 0);
+		counter_atomic32_set(&intf->stats[i], 0);
 
 	mutex_lock(&ipmi_interfaces_mutex);
 	/* Look for a hole in the numbers. */

drivers/char/ipmi/ipmi_si_intf.c +5 -4

index 77b8d551ae7f..0909a3461f05 100644
@@ -43,6 +43,7 @@
 #include "ipmi_si_sm.h"
 #include <linux/string.h>
 #include <linux/ctype.h>
+#include <linux/counters.h>
 
 /* Measure times between events in the driver. */
 #undef DEBUG_TIMING

@@ -237,7 +238,7 @@ struct smi_info {
 	bool dev_group_added;
 
 	/* Counters and things for the proc filesystem. */
-	atomic_t stats[SI_NUM_STATS];
+	struct counter_atomic32 stats[SI_NUM_STATS];
 
 	struct task_struct *thread;
 

@@ -245,9 +246,9 @@ struct smi_info {
 };
 
 #define smi_inc_stat(smi, stat) \
-	atomic_inc(&(smi)->stats[SI_STAT_ ## stat])
+	counter_atomic32_inc(&(smi)->stats[SI_STAT_ ## stat])
 #define smi_get_stat(smi, stat) \
-	((unsigned int) atomic_read(&(smi)->stats[SI_STAT_ ## stat]))
+	((unsigned int) counter_atomic32_read(&(smi)->stats[SI_STAT_ ## stat]))
 
 #define IPMI_MAX_INTFS 4
 static int force_kipmid[IPMI_MAX_INTFS];

@@ -2013,7 +2014,7 @@ static int try_smi_init(struct smi_info *new_smi)
 	atomic_set(&new_smi->req_events, 0);
 	new_smi->run_to_completion = false;
 	for (i = 0; i < SI_NUM_STATS; i++)
-		atomic_set(&new_smi->stats[i], 0);
+		counter_atomic32_set(&new_smi->stats[i], 0);
 
 	new_smi->interrupt_disabled = true;
 	atomic_set(&new_smi->need_watch, 0);

Signature · 1 line

2.25.1

Quoted 8 lines · Shuah Khan

counter_atomic* is introduced to be used when a variable is used as a simple counter and doesn't guard object lifetimes. This clearly differentiates atomic_t usages that guard object lifetimes.

counter_atomic* variables will wrap around to 0 when it overflows and should not be used to guard resource lifetimes, device usage and open counts that control state changes, and pm states.

Quoted 6 lines

atomic_t variables used for stats are atomic counters. Overflow will wrap around and reset the stats and no change with the conversion.

Convert them to use counter_atomic32.

Signed-off-by Shuah Khan <[email protected]>

Reviewed-by Corey Minyard <[email protected]>

I assume for this conversion that the plan is to eliminate atomic_t completely and convert all atomic counters used for object lifetime to struct kref? The new naming is certainly more clear and I'm happy with this change.

counter_atomic* is introduced to be used when a variable is used as a simple counter and doesn't guard object lifetimes. This clearly differentiates atomic_t usages that guard object lifetimes.

counter_atomic* variables will wrap around to 0 when it overflows and should not be used to guard resource lifetimes, device usage and open counts that control state changes, and pm states.

atomic_t variables used for pci counters keep track of pci parity and non-parity errors. Convert them to use counter_atomic32.

Overflow will wrap around and reset the counts as was the case prior to the conversion.

Signed-off-by Shuah Khan <[email protected]>

---
 drivers/edac/edac_pci.h       |  5 +++--
 drivers/edac/edac_pci_sysfs.c | 28 ++++++++++++++--------------
 2 files changed, 17 insertions(+), 16 deletions(-)

-- Addressed Kees's comments:
1. Non-atomic counters renamed to counter_simple32 and counter_simple64 to clearly indicate size.
2. Added warning for counter_simple* usage and it should be used only

when there is no need for atomicity.
3. Renamed counter_atomic to counter_atomic32 to clearly indicate size.
4. Renamed counter_atomic_long to counter_atomic64 and it now uses atomic64_t ops and indicates size.
5. Test updated for the API renames.
6. Added helper functions for test results printing
7. Verified that the test module compiles in kunit env. and test module can be loaded to run the test.

8. Updated Documentation to reflect the intent to make the API restricted so it can never be used to guard object lifetimes and state management. I left _return ops for now, inc_return is necessary for now as per the discussion we had on this topic.

Quoted 4 lines · Shuah Khan

-- Addressed Kees's comments:
1. Non-atomic counters renamed to counter_simple32 and counter_simple64 to clearly indicate size.
2. Added warning for counter_simple* usage and it should be used only

Quoted 8 lines

when there is no need for atomicity.
3. Renamed counter_atomic to counter_atomic32 to clearly indicate size.
4. Renamed counter_atomic_long to counter_atomic64 and it now uses atomic64_t ops and indicates size.
5. Test updated for the API renames.
6. Added helper functions for test results printing
7. Verified that the test module compiles in kunit env. and test module can be loaded to run the test.

Thanks for all of this!

Quoted 4 lines

8. Updated Documentation to reflect the intent to make the API restricted so it can never be used to guard object lifetimes and state management. I left _return ops for now, inc_return is necessary for now as per the discussion we had on this topic.

I still *really* do not want dec_return() to exist. That is asking for trouble. I'd prefer inc_return() not exist either, but I can live with it. ;)

Quoted 21 lines · Kees Cook

Quoted 4 lines · Shuah Khan

-- Addressed Kees's comments:
1. Non-atomic counters renamed to counter_simple32 and counter_simple64 to clearly indicate size.
2. Added warning for counter_simple* usage and it should be used only

Quoted 8 lines

when there is no need for atomicity.
3. Renamed counter_atomic to counter_atomic32 to clearly indicate size.
4. Renamed counter_atomic_long to counter_atomic64 and it now uses atomic64_t ops and indicates size.
5. Test updated for the API renames.
6. Added helper functions for test results printing
7. Verified that the test module compiles in kunit env. and test module can be loaded to run the test.

Thanks for all of this!

Quoted 4 lines

8. Updated Documentation to reflect the intent to make the API restricted so it can never be used to guard object lifetimes and state management. I left _return ops for now, inc_return is necessary for now as per the discussion we had on this topic.

Quoted 4 lines

I still *really* do not want dec_return() to exist. That is asking for trouble. I'd prefer inc_return() not exist either, but I can live with it. ;)

Thanks. I am equally concerned about adding anything that can be used to guard object lifetimes. So I will make sure this set won't expand and plan to remove dec_return() if we don't find any usages.

Quoted 22 lines · Shuah Khan

Quoted 21 lines · Kees Cook

Quoted 4 lines · Shuah Khan

-- Addressed Kees's comments:
1. Non-atomic counters renamed to counter_simple32 and counter_simple64 to clearly indicate size.
2. Added warning for counter_simple* usage and it should be used only

Quoted 8 lines

when there is no need for atomicity.
3. Renamed counter_atomic to counter_atomic32 to clearly indicate size.
4. Renamed counter_atomic_long to counter_atomic64 and it now uses atomic64_t ops and indicates size.
5. Test updated for the API renames.
6. Added helper functions for test results printing
7. Verified that the test module compiles in kunit env. and test module can be loaded to run the test.

Thanks for all of this!

Quoted 4 lines

8. Updated Documentation to reflect the intent to make the API restricted so it can never be used to guard object lifetimes and state management. I left _return ops for now, inc_return is necessary for now as per the discussion we had on this topic.

Quoted 5 lines

Quoted 4 lines

I still *really* do not want dec_return() to exist. That is asking for trouble. I'd prefer inc_return() not exist either, but I can live with it. ;)

Quoted 3 lines

Thanks. I am equally concerned about adding anything that can be used to guard object lifetimes. So I will make sure this set won't expand and plan to remove dec_return() if we don't find any usages.

I would like it much stronger than "if". dec_return() needs to be just dec() and read(). It will not be less efficient (since they're both inlines), but it _will_ create a case where the atomicity cannot be used for ref counting. My point is that anything that _requires_ dec_return() (or, frankly, inc_return()) is _not_ "just" a statistical counter. It may not be a refcounter, but it relies on the inc/dec atomicity for some reason beyond counting in once place and reporting it in another.

This patch series is a result of discussion at the refcount_t BOF the Linux Plumbers Conference. In this discussion, we identified a need for looking closely and investigating atomic_t usages in the kernel when it is used strictly as a counter without it controlling object lifetimes and state changes.

Quoted 5 lines · Shuah Khan

This patch series is a result of discussion at the refcount_t BOF the Linux Plumbers Conference. In this discussion, we identified a need for looking closely and investigating atomic_t usages in the kernel when it is used strictly as a counter without it controlling object lifetimes and state changes.

BTW, I realized the KSPP issue tracker hadn't broken this task out of the refcount_t conversion issue[1] into a separate issue, so I've created it now: https://github.com/KSPP/linux/issues/106

7. Verified that the test module compiles in kunit env. and test module can be loaded to run the test.

Quoted 2 lines · Shuah Khan

7. Verified that the test module compiles in kunit env. and test module can be loaded to run the test.

I meant write it using KUnit interfaces (e.g. KUNIT_EXPECT*(), kunit_test_suite(), etc):
https://www.kernel.org/doc/html/latest/dev-tools/kunit/

Though I see the docs are still not updated[1] to reflect the Kconfig (CONFIG_foo_KUNIT_TEST) and file naming conventions (foo_kunit.c).

Quoted 8 lines · Kees Cook

Quoted 2 lines · Shuah Khan

7. Verified that the test module compiles in kunit env. and test module can be loaded to run the test.

I meant write it using KUnit interfaces (e.g. KUNIT_EXPECT*(), kunit_test_suite(), etc):
https://www.kernel.org/doc/html/latest/dev-tools/kunit/

Quoted 3 lines

Though I see the docs are still not updated[1] to reflect the Kconfig (CONFIG_foo_KUNIT_TEST) and file naming conventions (foo_kunit.c).

I would like to be able to run this test outside Kunit env., hence the choice to go with a module and kselftest script. It makes it easier to test as part of my workflow as opposed to doing a kunit and build and running it that way.

Quoted 13 lines · Shuah Khan

Quoted 11 lines · Kees Cook

Quoted 2 lines · Shuah Khan

7. Verified that the test module compiles in kunit env. and test module can be loaded to run the test.

I meant write it using KUnit interfaces (e.g. KUNIT_EXPECT*(), kunit_test_suite(), etc):
https://www.kernel.org/doc/html/latest/dev-tools/kunit/

Though I see the docs are still not updated[1] to reflect the Kconfig (CONFIG_foo_KUNIT_TEST) and file naming conventions (foo_kunit.c).

Quoted 4 lines

I would like to be able to run this test outside Kunit env., hence the choice to go with a module and kselftest script. It makes it easier to test as part of my workflow as opposed to doing a kunit and build and running it that way.

It does -- you just load it normally like before and it prints out everything just fine. This is how I use the lib/test_user_copy.c and lib/test_overflow.c before/after their conversions.

This patch series is a result of discussion at the refcount_t BOF the Linux Plumbers Conference. In this discussion, we identified a need for looking closely and investigating atomic_t usages in the kernel when it is used strictly as a counter without it controlling object lifetimes and state changes.

There are a number of atomic_t usages in the kernel where atomic_t api is used strictly for counting and not for managing object lifetime. In some cases, atomic_t might not even be needed.

The purpose of these counters is twofold: 1. clearly differentiate atomic_t counters from atomic_t usages that guard object lifetimes, hence prone to overflow and underflow errors. It allows 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. 2. provides non-atomic counters for cases where atomic isn't necessary.

Simple atomic and non-atomic counters api provides interfaces for simple atomic and non-atomic counters that just count, and don't guard resource lifetimes. Counters will wrap around to 0 when it overflows and should not be used to guard resource lifetimes, device usage and open counts that control state changes, and pm states.

Using counter_atomic to guard lifetimes could lead to use-after free when it overflows and undefined behavior when used to manage state changes and device usage/open states.

This patch series introduces Simple atomic and non-atomic counters. Counter atomic ops leverage atomic_t and provide a sub-set of atomic_t ops.

In addition this patch series converts a few drivers to use the new api. The following criteria is used for select variables for conversion:

1. Variable doesn't guard object lifetimes, manage state changes e.g:
device usage counts, device open counts, and pm states.
2. Variable is used for stats and counters.
3. The conversion doesn't change the overflow behavior.

Changes since RFC:
-- Thanks for reviews and reviewed-by, and Acked-by tags. Updated
the patches with the tags.
-- Addressed Kees's comments:
1. Non-atomic counters renamed to counter_simple32 and counter_simple64 to clearly indicate size.
2. Added warning for counter_simple* usage and it should be used only when there is no need for atomicity.
3. Renamed counter_atomic to counter_atomic32 to clearly indicate size.
4. Renamed counter_atomic_long to counter_atomic64 and it now uses atomic64_t ops and indicates size.
5. Test updated for the API renames.
6. Added helper functions for test results printing
7. Verified that the test module compiles in kunit env. and test module can be loaded to run the test.
8. Updated Documentation to reflect the intent to make the API restricted so it can never be used to guard object lifetimes and state management. I left _return ops for now, inc_return is necessary for now as per the discussion we had on this topic.
-- Updated driver patches with API name changes.
-- We discussed if binder counters can be non-atomic. For now I left
them the same as the RFC patch - using counter_atomic32
-- Unrelated to this patch series:
The patch series review uncovered improvements could be made to test_async_driver_probe and vmw_vmci/vmci_guest. I will track these for fixing later.

Shuah Khan (11):
counters: Introduce counter_simple* and counter_atomic* counters selftests:lib:test_counters: add new test for counters drivers/base: convert deferred_trigger_count and probe_count to
counter_atomic32
drivers/base/devcoredump: convert devcd_count to counter_atomic32 drivers/acpi: convert seqno counter_atomic32 drivers/acpi/apei: convert seqno counter_atomic32 drivers/android/binder: convert stats, transaction_log to
counter_atomic32
drivers/base/test/test_async_driver_probe: convert to use
counter_atomic32
drivers/char/ipmi: convert stats to use counter_atomic32 drivers/misc/vmw_vmci: convert num guest devices counter to
counter_atomic32
drivers/edac: convert pci counters to counter_atomic32

 Documentation/core-api/counters.rst          | 174 +++++++++
 MAINTAINERS                                  |   8 +
 drivers/acpi/acpi_extlog.c                   |   5 +-
 drivers/acpi/apei/ghes.c                     |   5 +-
 drivers/android/binder.c                     |  41 +--
 drivers/android/binder_internal.h            |   3 +-
 drivers/base/dd.c                            |  19 +-
 drivers/base/devcoredump.c                   |   5 +-
 drivers/base/test/test_async_driver_probe.c  |  23 +-
 drivers/char/ipmi/ipmi_msghandler.c          |   9 +-
 drivers/char/ipmi/ipmi_si_intf.c             |   9 +-
 drivers/edac/edac_pci.h                      |   5 +-
 drivers/edac/edac_pci_sysfs.c                |  28 +-
 drivers/misc/vmw_vmci/vmci_guest.c           |   9 +-
 include/linux/counters.h                     | 350 +++++++++++++++++++
 lib/Kconfig                                  |  10 +
 lib/Makefile                                 |   1 +
 lib/test_counters.c                          | 276 +++++++++++++++
 tools/testing/selftests/lib/Makefile         |   1 +
 tools/testing/selftests/lib/config           |   1 +
 tools/testing/selftests/lib/test_counters.sh |   5 +
 21 files changed, 913 insertions(+), 74 deletions(-)
 create mode 100644 Documentation/core-api/counters.rst
 create mode 100644 include/linux/counters.h
 create mode 100644 lib/test_counters.c
 create mode 100755 tools/testing/selftests/lib/test_counters.sh

Signature · 1 line

2.25.1

Quoted 10 lines · Shuah Khan

This patch series is a result of discussion at the refcount_t BOF the Linux Plumbers Conference. In this discussion, we identified a need for looking closely and investigating atomic_t usages in the kernel when it is used strictly as a counter without it controlling object lifetimes and state changes.

There are a number of atomic_t usages in the kernel where atomic_t api is used strictly for counting and not for managing object lifetime. In some cases, atomic_t might not even be needed.

Quoted 6 lines

The purpose of these counters is twofold: 1. clearly differentiate atomic_t counters from atomic_t usages that guard object lifetimes, hence prone to overflow and underflow errors. It allows 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. 2. provides non-atomic counters for cases where atomic isn't necessary.

Nice series :)

It appears there is no user of counter_simple in this series other than the selftest. Would you be planning to add any conversions in the series itself, for illustration of use? Sorry if I missed a usage.

Also how do we guard against atomicity of counter_simple RMW operations? Is the implication that it should be guarded using other synchronization to prevent lost-update problem?

Some more comments:

1. atomic RMW operations that have a return value are fully ordered. Would you be adding support to counter_simple for such ordering as well, for consistency?

2. I felt counter_atomic and counter_atomic64 would be nice equivalents to the atomic and atomic64 naming currently used (i.e. dropping the '32'). However that is just my opinion and I am ok with either naming.

Quoted 28 lines · Joel Fernandes

Quoted 10 lines · Shuah Khan

This patch series is a result of discussion at the refcount_t BOF the Linux Plumbers Conference. In this discussion, we identified a need for looking closely and investigating atomic_t usages in the kernel when it is used strictly as a counter without it controlling object lifetimes and state changes.

There are a number of atomic_t usages in the kernel where atomic_t api is used strictly for counting and not for managing object lifetime. In some cases, atomic_t might not even be needed.

Quoted 6 lines

The purpose of these counters is twofold: 1. clearly differentiate atomic_t counters from atomic_t usages that guard object lifetimes, hence prone to overflow and underflow errors. It allows 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. 2. provides non-atomic counters for cases where atomic isn't necessary.

Nice series :)

It appears there is no user of counter_simple in this series other than the selftest. Would you be planning to add any conversions in the series itself, for illustration of use? Sorry if I missed a usage.

Also how do we guard against atomicity of counter_simple RMW operations? Is the implication that it should be guarded using other synchronization to prevent lost-update problem?

Quoted 5 lines

Some more comments:

1. atomic RMW operations that have a return value are fully ordered. Would you be adding support to counter_simple for such ordering as well, for consistency?

No -- there is no atomicity guarantee for counter_simple. I would prefer counter_simple not exist at all, specifically for this reason.

Quoted 3 lines

2. I felt counter_atomic and counter_atomic64 would be nice equivalents to the atomic and atomic64 naming currently used (i.e. dropping the '32'). However that is just my opinion and I am ok with either naming.

I had asked that they be size-named to avoid any confusion (i.e. we're making a new API).

Quoted 19 lines · Kees Cook

Quoted 18 lines · Joel Fernandes

Quoted 16 lines · Shuah Khan

This patch series is a result of discussion at the refcount_t BOF the Linux Plumbers Conference. In this discussion, we identified a need for looking closely and investigating atomic_t usages in the kernel when it is used strictly as a counter without it controlling object lifetimes and state changes.

There are a number of atomic_t usages in the kernel where atomic_t api is used strictly for counting and not for managing object lifetime. In some cases, atomic_t might not even be needed.

The purpose of these counters is twofold: 1. clearly differentiate atomic_t counters from atomic_t usages that guard object lifetimes, hence prone to overflow and underflow errors. It allows 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. 2. provides non-atomic counters for cases where atomic isn't necessary.

Quoted 2 lines

Quoted 2 lines

Nice series :)

Quoted 14 lines

Quoted 13 lines

It appears there is no user of counter_simple in this series other than the selftest. Would you be planning to add any conversions in the series itself, for illustration of use? Sorry if I missed a usage.

Also how do we guard against atomicity of counter_simple RMW operations? Is the implication that it should be guarded using other synchronization to prevent lost-update problem?

Some more comments:

1. atomic RMW operations that have a return value are fully ordered. Would you be adding support to counter_simple for such ordering as well, for consistency?

Quoted 2 lines

No -- there is no atomicity guarantee for counter_simple. I would prefer counter_simple not exist at all, specifically for this reason.

Yeah I am ok with it not existing, especially also as there are no examples of its conversion/usage in the series.