There are a number of atomic_t usages in the kernel where atomic_t api
is used strictly for counting sequence numbers and other statistical
counters and not for managing object lifetime.
The purpose of these Sequence Number Ops is to clearly differentiate
atomic_t counter usages from atomic_t usages that guard object lifetimes,
hence prone to overflow and underflow errors.
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.
Sequence Number api provides interfaces for simple atomic_t counter usages
that just count, and don't guard resource lifetimes. The seqnum_ops are
built on top of atomic_t api, providing a smaller subset of atomic_t
interfaces necessary to support atomic_t usages as simple counters.
This api has init/set/inc/dec/read and doesn't support any other atomic_t
ops with the intent to restrict the use of these interfaces as simple
counting usages.
Sequence Numbers wrap around to INT_MIN when it overflows and should not
be used to guard resource lifetimes, device usage and open counts that
control state changes, and pm states. Overflowing to INT_MIN is consistent
with the atomic_t api, which it is built on top of.
Using seqnum 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.
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.
4. Note: inc_return() usages are changed to _inc() followed by _read()
Patches: 03/13, 04/13, 09/13, 10/13, 11/13
5. drivers/acpi and drivers/acpi/apei patches have been reviewed
before the rename, however in addition to rename, inc_return()
usages are changed to _inc() followed by _read()
6. test_async_driver_probe, char/ipmi, and edac patches have been
reviewed and no changes other than the rename to seqnum_ops.
7. security/integrity/ima: Okay to depend on CONFIG_64BIT?
The work for this is a follow-on to the discussion and review of
Introduce Simple atomic counters patch series:
//lore.kernel.org/lkml/[email protected]/
Based on the feedback to restrict and limit the scope:
- dropped inc_return()
- renamed interfaces to match the intent and also shorten the
interface names.
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 | 126 ++++++++++++++
MAINTAINERS | 8 +
drivers/acpi/acpi_extlog.c | 6 +-
drivers/acpi/apei/ghes.c | 6 +-
drivers/base/test/test_async_driver_probe.c | 26 +--
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 | 28 ++--
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 | 37 +++--
drivers/usb/usbip/vhci.h | 3 +-
drivers/usb/usbip/vhci_hcd.c | 9 +-
drivers/usb/usbip/vhci_rx.c | 3 +-
include/linux/oprofile.h | 3 +-
include/linux/seqnum_ops.h | 154 ++++++++++++++++++
lib/Kconfig | 9 +
lib/Makefile | 1 +
lib/test_seqnum_ops.c | 154 ++++++++++++++++++
security/integrity/ima/ima.h | 5 +-
security/integrity/ima/ima_api.c | 2 +-
security/integrity/ima/ima_fs.c | 4 +-
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, 637 insertions(+), 111 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
seqnum_ops api is introduced to be used when a variable is used as
a sequence/stat counter and doesn't guard object lifetimes. This
clearly differentiates atomic_t usages that guard object lifetimes.
seqnum32 variables wrap around to INT_MIN when it overflows and
should not be used to guard resource lifetimes, device usage and
open counts that control state changes, and pm states.
seqno is a sequence number counter for logging. This counter gets
incremented. Unsure if there is a chance of this overflowing. It
doesn't look like overflowing causes any problems since it is used
to tag the log messages and nothing more. This conversion doesn't
change the overflow wrap around behavior.
Convert it to use seqnum_ops. This conversion replaces inc_return()
with _inc() followed by _read().
Signed-off-by: Shuah Khan <[email protected]>
---
drivers/acpi/acpi_extlog.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index 72f1fb77abcd..1e2b36aab9aa 100644
--- a/drivers/acpi/acpi_extlog.c
+++ b/drivers/acpi/acpi_extlog.c
@@ -12,6 +12,7 @@
#include <linux/ratelimit.h>
#include <linux/edac.h>
#include <linux/ras.h>
+#include <linux/seqnum_ops.h>
#include <asm/cpu.h>
#include <asm/mce.h>
@@ -93,7 +94,7 @@ static struct acpi_hest_generic_status *extlog_elog_entry_check(int cpu, int ban
static void __print_extlog_rcd(const char *pfx,
struct acpi_hest_generic_status *estatus, int cpu)
{
- static atomic_t seqno;
+ static struct seqnum32 seqno;
unsigned int curr_seqno;
char pfx_seq[64];
@@ -103,7 +104,8 @@ static void __print_extlog_rcd(const char *pfx,
else
pfx = KERN_ERR;
}
- curr_seqno = atomic_inc_return(&seqno);
+ seqnum32_inc(&seqno);
+ curr_seqno = seqnum32_read(&seqno);
snprintf(pfx_seq, sizeof(pfx_seq), "%s{%u}", pfx, curr_seqno);
printk("%s""Hardware error detected on CPU%d\n", pfx_seq, cpu);
cper_estatus_print(pfx_seq, estatus);
--
2.27.0
seqnum_ops api is introduced to be used when a variable is used as
a sequence/stat counter and doesn't guard object lifetimes. This
clearly differentiates atomic_t usages that guard object lifetimes.
seqnum32 variables wrap around to INT_MIN 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 error_count and ios_threshold are atomic
counters and guarded by max. values. No change to the behavior with
this change.
Signed-off-by: Shuah Khan <[email protected]>
---
.../staging/unisys/visorhba/visorhba_main.c | 37 ++++++++++---------
1 file changed, 19 insertions(+), 18 deletions(-)
diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c b/drivers/staging/unisys/visorhba/visorhba_main.c
index 7ae5306b92fe..3209958b8aaa 100644
--- a/drivers/staging/unisys/visorhba/visorhba_main.c
+++ b/drivers/staging/unisys/visorhba/visorhba_main.c
@@ -10,6 +10,7 @@
#include <linux/module.h>
#include <linux/seq_file.h>
#include <linux/visorbus.h>
+#include <linux/seqnum_ops.h>
#include <scsi/scsi.h>
#include <scsi/scsi_host.h>
#include <scsi/scsi_cmnd.h>
@@ -41,8 +42,8 @@ MODULE_ALIAS("visorbus:" VISOR_VHBA_CHANNEL_GUID_STR);
struct visordisk_info {
struct scsi_device *sdev;
u32 valid;
- atomic_t ios_threshold;
- atomic_t error_count;
+ struct seqnum32 ios_threshold;
+ struct seqnum32 error_count;
struct visordisk_info *next;
};
@@ -374,10 +375,10 @@ static int visorhba_abort_handler(struct scsi_cmnd *scsicmd)
scsidev = scsicmd->device;
vdisk = scsidev->hostdata;
- if (atomic_read(&vdisk->error_count) < VISORHBA_ERROR_COUNT)
- atomic_inc(&vdisk->error_count);
+ if (seqnum32_read(&vdisk->error_count) < VISORHBA_ERROR_COUNT)
+ seqnum32_inc(&vdisk->error_count);
else
- atomic_set(&vdisk->ios_threshold, IOS_ERROR_THRESHOLD);
+ seqnum32_set(&vdisk->ios_threshold, IOS_ERROR_THRESHOLD);
rtn = forward_taskmgmt_command(TASK_MGMT_ABORT_TASK, scsidev);
if (rtn == SUCCESS) {
scsicmd->result = DID_ABORT << 16;
@@ -401,10 +402,10 @@ static int visorhba_device_reset_handler(struct scsi_cmnd *scsicmd)
scsidev = scsicmd->device;
vdisk = scsidev->hostdata;
- if (atomic_read(&vdisk->error_count) < VISORHBA_ERROR_COUNT)
- atomic_inc(&vdisk->error_count);
+ if (seqnum32_read(&vdisk->error_count) < VISORHBA_ERROR_COUNT)
+ seqnum32_inc(&vdisk->error_count);
else
- atomic_set(&vdisk->ios_threshold, IOS_ERROR_THRESHOLD);
+ seqnum32_set(&vdisk->ios_threshold, IOS_ERROR_THRESHOLD);
rtn = forward_taskmgmt_command(TASK_MGMT_LUN_RESET, scsidev);
if (rtn == SUCCESS) {
scsicmd->result = DID_RESET << 16;
@@ -429,10 +430,10 @@ static int visorhba_bus_reset_handler(struct scsi_cmnd *scsicmd)
scsidev = scsicmd->device;
shost_for_each_device(scsidev, scsidev->host) {
vdisk = scsidev->hostdata;
- if (atomic_read(&vdisk->error_count) < VISORHBA_ERROR_COUNT)
- atomic_inc(&vdisk->error_count);
+ if (seqnum32_read(&vdisk->error_count) < VISORHBA_ERROR_COUNT)
+ seqnum32_inc(&vdisk->error_count);
else
- atomic_set(&vdisk->ios_threshold, IOS_ERROR_THRESHOLD);
+ seqnum32_set(&vdisk->ios_threshold, IOS_ERROR_THRESHOLD);
}
rtn = forward_taskmgmt_command(TASK_MGMT_BUS_RESET, scsidev);
if (rtn == SUCCESS) {
@@ -803,9 +804,9 @@ static void do_scsi_linuxstat(struct uiscmdrsp *cmdrsp,
return;
/* Okay see what our error_count is here.... */
vdisk = scsidev->hostdata;
- if (atomic_read(&vdisk->error_count) < VISORHBA_ERROR_COUNT) {
- atomic_inc(&vdisk->error_count);
- atomic_set(&vdisk->ios_threshold, IOS_ERROR_THRESHOLD);
+ if (seqnum32_read(&vdisk->error_count) < VISORHBA_ERROR_COUNT) {
+ seqnum32_inc(&vdisk->error_count);
+ seqnum32_set(&vdisk->ios_threshold, IOS_ERROR_THRESHOLD);
}
}
@@ -881,10 +882,10 @@ static void do_scsi_nolinuxstat(struct uiscmdrsp *cmdrsp,
kfree(buf);
} else {
vdisk = scsidev->hostdata;
- if (atomic_read(&vdisk->ios_threshold) > 0) {
- atomic_dec(&vdisk->ios_threshold);
- if (atomic_read(&vdisk->ios_threshold) == 0)
- atomic_set(&vdisk->error_count, 0);
+ if (seqnum32_read(&vdisk->ios_threshold) > 0) {
+ seqnum32_dec(&vdisk->ios_threshold);
+ if (seqnum32_read(&vdisk->ios_threshold) == 0)
+ seqnum32_set(&vdisk->error_count, 0);
}
}
}
--
2.27.0
seqnum_ops api is introduced to be used when a variable is used as
a sequence/stat counter and doesn't guard object lifetimes. This
clearly differentiates atomic_t usages that guard object lifetimes.
seqnum32 variables wrap around to INT_MIN 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 seqnum_ops. This conversion replaces inc_return()
with _inc() followed by _read().
Signed-off-by: Shuah Khan <[email protected]>
---
drivers/staging/rtl8188eu/core/rtw_mlme_ext.c | 23 ++++++++++++++-----
.../staging/rtl8188eu/include/rtw_mlme_ext.h | 3 ++-
2 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
index b3eddcb83cd0..31dd9d31dd9a 100644
--- a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
@@ -7,6 +7,7 @@
#define _RTW_MLME_EXT_C_
#include <linux/ieee80211.h>
+#include <linux/seqnum_ops.h>
#include <asm/unaligned.h>
#include <osdep_service.h>
@@ -3860,7 +3861,7 @@ static void init_mlme_ext_priv_value(struct adapter *padapter)
_12M_RATE_, _24M_RATE_, 0xff,
};
- atomic_set(&pmlmeext->event_seq, 0);
+ seqnum32_set(&pmlmeext->event_seq, 0);
pmlmeext->mgnt_seq = 0;/* reset to zero when disconnect at client mode */
pmlmeext->cur_channel = padapter->registrypriv.channel;
@@ -4189,7 +4190,9 @@ void report_survey_event(struct adapter *padapter,
pc2h_evt_hdr = (struct C2HEvent_Header *)(pevtcmd);
pc2h_evt_hdr->len = sizeof(struct survey_event);
pc2h_evt_hdr->ID = _Survey_EVT_;
- pc2h_evt_hdr->seq = atomic_inc_return(&pmlmeext->event_seq);
+
+ seqnum32_inc(&pmlmeext->event_seq);
+ pc2h_evt_hdr->seq = seqnum32_read(&pmlmeext->event_seq);
psurvey_evt = (struct survey_event *)(pevtcmd + sizeof(struct C2HEvent_Header));
@@ -4239,7 +4242,9 @@ void report_surveydone_event(struct adapter *padapter)
pc2h_evt_hdr = (struct C2HEvent_Header *)(pevtcmd);
pc2h_evt_hdr->len = sizeof(struct surveydone_event);
pc2h_evt_hdr->ID = _SurveyDone_EVT_;
- pc2h_evt_hdr->seq = atomic_inc_return(&pmlmeext->event_seq);
+
+ seqnum32_inc(&pmlmeext->event_seq);
+ pc2h_evt_hdr->seq = seqnum32_read(&pmlmeext->event_seq);
psurveydone_evt = (struct surveydone_event *)(pevtcmd + sizeof(struct C2HEvent_Header));
psurveydone_evt->bss_cnt = pmlmeext->sitesurvey_res.bss_cnt;
@@ -4283,7 +4288,9 @@ void report_join_res(struct adapter *padapter, int res)
pc2h_evt_hdr = (struct C2HEvent_Header *)(pevtcmd);
pc2h_evt_hdr->len = sizeof(struct joinbss_event);
pc2h_evt_hdr->ID = _JoinBss_EVT_;
- pc2h_evt_hdr->seq = atomic_inc_return(&pmlmeext->event_seq);
+
+ seqnum32_inc(&pmlmeext->event_seq);
+ pc2h_evt_hdr->seq = seqnum32_read(&pmlmeext->event_seq);
pjoinbss_evt = (struct joinbss_event *)(pevtcmd + sizeof(struct C2HEvent_Header));
memcpy((unsigned char *)(&pjoinbss_evt->network.network), &pmlmeinfo->network, sizeof(struct wlan_bssid_ex));
@@ -4333,7 +4340,9 @@ void report_del_sta_event(struct adapter *padapter, unsigned char *MacAddr,
pc2h_evt_hdr = (struct C2HEvent_Header *)(pevtcmd);
pc2h_evt_hdr->len = sizeof(struct stadel_event);
pc2h_evt_hdr->ID = _DelSTA_EVT_;
- pc2h_evt_hdr->seq = atomic_inc_return(&pmlmeext->event_seq);
+
+ seqnum32_inc(&pmlmeext->event_seq);
+ pc2h_evt_hdr->seq = seqnum32_read(&pmlmeext->event_seq);
pdel_sta_evt = (struct stadel_event *)(pevtcmd + sizeof(struct C2HEvent_Header));
ether_addr_copy((unsigned char *)(&pdel_sta_evt->macaddr), MacAddr);
@@ -4386,7 +4395,9 @@ void report_add_sta_event(struct adapter *padapter, unsigned char *MacAddr,
pc2h_evt_hdr = (struct C2HEvent_Header *)(pevtcmd);
pc2h_evt_hdr->len = sizeof(struct stassoc_event);
pc2h_evt_hdr->ID = _AddSTA_EVT_;
- pc2h_evt_hdr->seq = atomic_inc_return(&pmlmeext->event_seq);
+
+ seqnum32_inc(&pmlmeext->event_seq);
+ pc2h_evt_hdr->seq = seqnum32_read(&pmlmeext->event_seq);
padd_sta_evt = (struct stassoc_event *)(pevtcmd + sizeof(struct C2HEvent_Header));
ether_addr_copy((unsigned char *)(&padd_sta_evt->macaddr), MacAddr);
diff --git a/drivers/staging/rtl8188eu/include/rtw_mlme_ext.h b/drivers/staging/rtl8188eu/include/rtw_mlme_ext.h
index b11a6886a083..09b7e3bb2738 100644
--- a/drivers/staging/rtl8188eu/include/rtw_mlme_ext.h
+++ b/drivers/staging/rtl8188eu/include/rtw_mlme_ext.h
@@ -7,6 +7,7 @@
#ifndef __RTW_MLME_EXT_H_
#define __RTW_MLME_EXT_H_
+#include <linux/seqnum_ops.h>
#include <osdep_service.h>
#include <drv_types.h>
#include <wlan_bssdef.h>
@@ -393,7 +394,7 @@ struct p2p_oper_class_map {
struct mlme_ext_priv {
struct adapter *padapter;
u8 mlmeext_init;
- atomic_t event_seq;
+ struct seqnum32 event_seq;
u16 mgnt_seq;
unsigned char cur_channel;
--
2.27.0
seqnum_ops api is introduced to be used when a variable is used as
a sequence/stat counter and doesn't guard object lifetimes. This
clearly differentiates atomic_t usages that guard object lifetimes.
seqnum32 variables wrap around to INT_MIN 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 seqnum_ops.
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(-)
diff --git a/drivers/edac/edac_pci.h b/drivers/edac/edac_pci.h
index 5175f5724cfa..33b33e62c37f 100644
--- a/drivers/edac/edac_pci.h
+++ b/drivers/edac/edac_pci.h
@@ -30,12 +30,13 @@
#include <linux/pci.h>
#include <linux/types.h>
#include <linux/workqueue.h>
+#include <linux/seqnum_ops.h>
#ifdef CONFIG_PCI
struct edac_pci_counter {
- atomic_t pe_count;
- atomic_t npe_count;
+ struct seqnum32 pe_count;
+ struct seqnum32 npe_count;
};
/*
diff --git a/drivers/edac/edac_pci_sysfs.c b/drivers/edac/edac_pci_sysfs.c
index 53042af7262e..96f950fb39b0 100644
--- a/drivers/edac/edac_pci_sysfs.c
+++ b/drivers/edac/edac_pci_sysfs.c
@@ -23,8 +23,8 @@ static int edac_pci_log_pe = 1; /* log PCI parity errors */
static int edac_pci_log_npe = 1; /* log PCI non-parity error errors */
static int edac_pci_poll_msec = 1000; /* one second workq period */
-static atomic_t pci_parity_count = ATOMIC_INIT(0);
-static atomic_t pci_nonparity_count = ATOMIC_INIT(0);
+static struct seqnum32 pci_parity_count = SEQNUM_INIT(0);
+static struct seqnum32 pci_nonparity_count = SEQNUM_INIT(0);
static struct kobject *edac_pci_top_main_kobj;
static atomic_t edac_pci_sysfs_refcount = ATOMIC_INIT(0);
@@ -58,13 +58,13 @@ int edac_pci_get_poll_msec(void)
/**************************** EDAC PCI sysfs instance *******************/
static ssize_t instance_pe_count_show(struct edac_pci_ctl_info *pci, char *data)
{
- return sprintf(data, "%u\n", atomic_read(&pci->counters.pe_count));
+ return sprintf(data, "%u\n", seqnum32_read(&pci->counters.pe_count));
}
static ssize_t instance_npe_count_show(struct edac_pci_ctl_info *pci,
char *data)
{
- return sprintf(data, "%u\n", atomic_read(&pci->counters.npe_count));
+ return sprintf(data, "%u\n", seqnum32_read(&pci->counters.npe_count));
}
#define to_instance(k) container_of(k, struct edac_pci_ctl_info, kobj)
@@ -553,7 +553,7 @@ static void edac_pci_dev_parity_test(struct pci_dev *dev)
edac_printk(KERN_CRIT, EDAC_PCI,
"Signaled System Error on %s\n",
pci_name(dev));
- atomic_inc(&pci_nonparity_count);
+ seqnum32_inc(&pci_nonparity_count);
}
if (status & (PCI_STATUS_PARITY)) {
@@ -561,7 +561,7 @@ static void edac_pci_dev_parity_test(struct pci_dev *dev)
"Master Data Parity Error on %s\n",
pci_name(dev));
- atomic_inc(&pci_parity_count);
+ seqnum32_inc(&pci_parity_count);
}
if (status & (PCI_STATUS_DETECTED_PARITY)) {
@@ -569,7 +569,7 @@ static void edac_pci_dev_parity_test(struct pci_dev *dev)
"Detected Parity Error on %s\n",
pci_name(dev));
- atomic_inc(&pci_parity_count);
+ seqnum32_inc(&pci_parity_count);
}
}
@@ -592,7 +592,7 @@ static void edac_pci_dev_parity_test(struct pci_dev *dev)
edac_printk(KERN_CRIT, EDAC_PCI, "Bridge "
"Signaled System Error on %s\n",
pci_name(dev));
- atomic_inc(&pci_nonparity_count);
+ seqnum32_inc(&pci_nonparity_count);
}
if (status & (PCI_STATUS_PARITY)) {
@@ -600,7 +600,7 @@ static void edac_pci_dev_parity_test(struct pci_dev *dev)
"Master Data Parity Error on "
"%s\n", pci_name(dev));
- atomic_inc(&pci_parity_count);
+ seqnum32_inc(&pci_parity_count);
}
if (status & (PCI_STATUS_DETECTED_PARITY)) {
@@ -608,7 +608,7 @@ static void edac_pci_dev_parity_test(struct pci_dev *dev)
"Detected Parity Error on %s\n",
pci_name(dev));
- atomic_inc(&pci_parity_count);
+ seqnum32_inc(&pci_parity_count);
}
}
}
@@ -646,7 +646,7 @@ void edac_pci_do_parity_check(void)
if (!check_pci_errors)
return;
- before_count = atomic_read(&pci_parity_count);
+ before_count = seqnum32_read(&pci_parity_count);
/* scan all PCI devices looking for a Parity Error on devices and
* bridges.
@@ -658,7 +658,7 @@ void edac_pci_do_parity_check(void)
/* Only if operator has selected panic on PCI Error */
if (edac_pci_get_panic_on_pe()) {
/* If the count is different 'after' from 'before' */
- if (before_count != atomic_read(&pci_parity_count))
+ if (before_count != seqnum32_read(&pci_parity_count))
panic("EDAC: PCI Parity Error");
}
}
@@ -686,7 +686,7 @@ void edac_pci_handle_pe(struct edac_pci_ctl_info *pci, const char *msg)
{
/* global PE counter incremented by edac_pci_do_parity_check() */
- atomic_inc(&pci->counters.pe_count);
+ seqnum32_inc(&pci->counters.pe_count);
if (edac_pci_get_log_pe())
edac_pci_printk(pci, KERN_WARNING,
@@ -711,7 +711,7 @@ void edac_pci_handle_npe(struct edac_pci_ctl_info *pci, const char *msg)
{
/* global NPE counter incremented by edac_pci_do_parity_check() */
- atomic_inc(&pci->counters.npe_count);
+ seqnum32_inc(&pci->counters.npe_count);
if (edac_pci_get_log_npe())
edac_pci_printk(pci, KERN_WARNING,
--
2.27.0
On Tue, Nov 10, 2020 at 12:53:26PM -0700, Shuah Khan wrote:
> There are a number of atomic_t usages in the kernel where atomic_t api
> is used strictly for counting sequence numbers and other statistical
> counters and not for managing object lifetime.
>
> The purpose of these Sequence Number Ops is to clearly differentiate
> atomic_t counter usages from atomic_t usages that guard object lifetimes,
> hence prone to overflow and underflow errors.
>
> 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.
>
> Sequence Number api provides interfaces for simple atomic_t counter usages
> that just count, and don't guard resource lifetimes. The seqnum_ops are
> built on top of atomic_t api, providing a smaller subset of atomic_t
> interfaces necessary to support atomic_t usages as simple counters.
> This api has init/set/inc/dec/read and doesn't support any other atomic_t
> ops with the intent to restrict the use of these interfaces as simple
> counting usages.
>
> Sequence Numbers wrap around to INT_MIN when it overflows and should not
> be used to guard resource lifetimes, device usage and open counts that
> control state changes, and pm states. Overflowing to INT_MIN is consistent
> with the atomic_t api, which it is built on top of.
If Sequence Numbers are subject to wraparound then they aren't reliable.
Given that they aren't reliable, why use atomic instructions at all?
Why not just use plain regular integers with READ_ONCE and WRITE_ONCE?
Alan Stern
On Tue, Nov 10, 2020 at 12:53:38PM -0700, Shuah Khan wrote:
> seqnum_ops api is introduced to be used when a variable is used as
> a sequence/stat counter and doesn't guard object lifetimes. This
> clearly differentiates atomic_t usages that guard object lifetimes.
>
> seqnum32 variables wrap around to INT_MIN 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 error_count and ios_threshold are atomic
> counters and guarded by max. values. No change to the behavior with
> this change.
>
> Signed-off-by: Shuah Khan <[email protected]>
> ---
> .../staging/unisys/visorhba/visorhba_main.c | 37 ++++++++++---------
> 1 file changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c b/drivers/staging/unisys/visorhba/visorhba_main.c
> index 7ae5306b92fe..3209958b8aaa 100644
> --- a/drivers/staging/unisys/visorhba/visorhba_main.c
> +++ b/drivers/staging/unisys/visorhba/visorhba_main.c
> @@ -10,6 +10,7 @@
> #include <linux/module.h>
> #include <linux/seq_file.h>
> #include <linux/visorbus.h>
> +#include <linux/seqnum_ops.h>
> #include <scsi/scsi.h>
> #include <scsi/scsi_host.h>
> #include <scsi/scsi_cmnd.h>
> @@ -41,8 +42,8 @@ MODULE_ALIAS("visorbus:" VISOR_VHBA_CHANNEL_GUID_STR);
> struct visordisk_info {
> struct scsi_device *sdev;
> u32 valid;
> - atomic_t ios_threshold;
> - atomic_t error_count;
> + struct seqnum32 ios_threshold;
> + struct seqnum32 error_count;
Are you sure the threshold variable is a sequence number?
It goes up and down, not just up and up and up.
An error count just goes up :)
thanks,
greg k-h
On 11/10/20 1:42 PM, Greg KH wrote:
> On Tue, Nov 10, 2020 at 12:53:38PM -0700, Shuah Khan wrote:
>> seqnum_ops api is introduced to be used when a variable is used as
>> a sequence/stat counter and doesn't guard object lifetimes. This
>> clearly differentiates atomic_t usages that guard object lifetimes.
>>
>> seqnum32 variables wrap around to INT_MIN 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 error_count and ios_threshold are atomic
>> counters and guarded by max. values. No change to the behavior with
>> this change.
>>
>> Signed-off-by: Shuah Khan <[email protected]>
>> ---
>> .../staging/unisys/visorhba/visorhba_main.c | 37 ++++++++++---------
>> 1 file changed, 19 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c b/drivers/staging/unisys/visorhba/visorhba_main.c
>> index 7ae5306b92fe..3209958b8aaa 100644
>> --- a/drivers/staging/unisys/visorhba/visorhba_main.c
>> +++ b/drivers/staging/unisys/visorhba/visorhba_main.c
>> @@ -10,6 +10,7 @@
>> #include <linux/module.h>
>> #include <linux/seq_file.h>
>> #include <linux/visorbus.h>
>> +#include <linux/seqnum_ops.h>
>> #include <scsi/scsi.h>
>> #include <scsi/scsi_host.h>
>> #include <scsi/scsi_cmnd.h>
>> @@ -41,8 +42,8 @@ MODULE_ALIAS("visorbus:" VISOR_VHBA_CHANNEL_GUID_STR);
>> struct visordisk_info {
>> struct scsi_device *sdev;
>> u32 valid;
>> - atomic_t ios_threshold;
>> - atomic_t error_count;
>> + struct seqnum32 ios_threshold;
>> + struct seqnum32 error_count;
>
> Are you sure the threshold variable is a sequence number >
> It goes up and down, not just up and up and up.
Right. I does go down. Turns out this is the only place seqnum32_dec()
is used. :)
I will fix. I noticed you made a comment about _dec() interfaces on
1/13. I can drop those as well. This way seqnum_ops can be just used
for up counters.
thanks,
-- Shuah
On 11/10/20 1:44 PM, Alan Stern wrote:
> On Tue, Nov 10, 2020 at 12:53:26PM -0700, Shuah Khan wrote:
>> There are a number of atomic_t usages in the kernel where atomic_t api
>> is used strictly for counting sequence numbers and other statistical
>> counters and not for managing object lifetime.
>>
>> The purpose of these Sequence Number Ops is to clearly differentiate
>> atomic_t counter usages from atomic_t usages that guard object lifetimes,
>> hence prone to overflow and underflow errors.
>>
>> 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.
>>
>> Sequence Number api provides interfaces for simple atomic_t counter usages
>> that just count, and don't guard resource lifetimes. The seqnum_ops are
>> built on top of atomic_t api, providing a smaller subset of atomic_t
>> interfaces necessary to support atomic_t usages as simple counters.
>> This api has init/set/inc/dec/read and doesn't support any other atomic_t
>> ops with the intent to restrict the use of these interfaces as simple
>> counting usages.
>>
>> Sequence Numbers wrap around to INT_MIN when it overflows and should not
>> be used to guard resource lifetimes, device usage and open counts that
>> control state changes, and pm states. Overflowing to INT_MIN is consistent
>> with the atomic_t api, which it is built on top of.
>
> If Sequence Numbers are subject to wraparound then they aren't reliable.
> Given that they aren't reliable, why use atomic instructions at all?
> Why not just use plain regular integers with READ_ONCE and WRITE_ONCE?
>
You still need atomic update for these numbers. The intent is to provide
atomic api for cases where the variable doesn't guard lifetimes and yet
needs atomic instructions.
Several such usages where atomic_t is used for up counting, also use
upper bounds. It is also an option to switch to seqnum64 to avoid
wrap around in case there is a concern.
thanks,
-- Shuah
On Tue, Nov 10, 2020 at 12:53:26PM -0700, Shuah Khan wrote:
> There are a number of atomic_t usages in the kernel where atomic_t api
> is used strictly for counting sequence numbers and other statistical
> counters and not for managing object lifetime.
We already have something in Linux called a sequence counter, and it's
different from this. ID counter? instance number? monotonic_t? stat_t?
On 11/10/20 9:33 PM, Matthew Wilcox wrote:
> On Tue, Nov 10, 2020 at 12:53:26PM -0700, Shuah Khan wrote:
>> There are a number of atomic_t usages in the kernel where atomic_t api
>> is used strictly for counting sequence numbers and other statistical
>> counters and not for managing object lifetime.
>
> We already have something in Linux called a sequence counter, and it's
> different from this. ID counter? instance number? monotonic_t? stat_t?
>
No results for monotonic_t or stat_t. Can you give me a pointer to what
your referring to.
thanks,
-- Shuah
On Wed, Nov 11, 2020 at 09:03:20AM -0700, Shuah Khan wrote:
> On 11/10/20 9:33 PM, Matthew Wilcox wrote:
> > On Tue, Nov 10, 2020 at 12:53:26PM -0700, Shuah Khan wrote:
> > > There are a number of atomic_t usages in the kernel where atomic_t api
> > > is used strictly for counting sequence numbers and other statistical
> > > counters and not for managing object lifetime.
> >
> > We already have something in Linux called a sequence counter, and it's
> > different from this. ID counter? instance number? monotonic_t? stat_t?
> >
>
> No results for monotonic_t or stat_t. Can you give me a pointer to what
> your referring to.
We have a seqcount_t. We need to call this something different.
maybe we should call it stat_t (and for that usage, stat_add() as well
as stat_inc() is a legitimate API to have).