Download raw body.
Please test: mpsafe sensors
On Wed, Aug 07, 2024 at 06:04:11PM +0300, Vitaliy Makkoveev wrote:
> I'm not happy with sensor pointers dereference after sleeping malloc(9)
> in sysctl_sensors(), so let's start making them mpsafe.
>
> New `sensordev_mtx' protects `sensordev_count' and `sensordev_list'.
> Reference counters are used to make sensor device and attached sensors
> dereference safe. Not all sensors do sensor_detach(), so sensor
> 'ksensordev' and 'ksensor' share reference counter.
>
> Each 'ksensordev' has it's own mutex(9) to protect mutable data. Each
> 'ksensor' has mutex too, but since we have many of them, only
> SENSOR_MPSAFE marked sensors rely on it. This diff doesn't introduce any
> of them, but it could be easily made on top. Anyway, we have many of
> sensors so they need to be unlocked separately.
>
> Please note, except sparc64 only sysctl_sensors() use sensordev_get()
> and sensor_find(), drivers have their instances and we don't need to
> acquire and release them.
>
> I'm not asking for OK's, this diff was shared to receive feedback and
> help with testing. That's why sensors manual page left unmodified.
>
I want to start pushing this forward. Updated against the latest
sources, but actually the diff is the same.
Index: sys/arch/sparc64/dev/tda.c
===================================================================
RCS file: /cvs/src/sys/arch/sparc64/dev/tda.c,v
retrieving revision 1.9
diff -u -p -r1.9 tda.c
--- sys/arch/sparc64/dev/tda.c 24 Oct 2021 17:05:04 -0000 1.9
+++ sys/arch/sparc64/dev/tda.c 19 May 2025 17:42:58 -0000
@@ -115,10 +115,12 @@ tda_attach(struct device *parent, struct
tda_setspeed(sc);
/* Get the number of sensor devices. */
+ mtx_enter(&sensordev_mtx);
for (i = 0; ; i++) {
- if (sensordev_get(i, &ksens) == ENOENT)
+ if (sensordev_get_locked(i, &ksens) == ENOENT)
break;
}
+ mtx_leave(&sensordev_mtx);
sc->sc_nsensors = i;
if (!sc->sc_nsensors) {
@@ -199,6 +201,7 @@ tda_adjust(void *v)
goto out;
}
ctemp = MAX(ctemp, ks->value);
+ sensor_rele(ks);
err = sensor_find(i, SENSOR_TEMP, SENSOR_TEMP_INT, &ks);
if (err) {
@@ -207,6 +210,7 @@ tda_adjust(void *v)
goto out;
}
stemp = MAX(stemp, ks->value);
+ sensor_rele(ks);
}
if (ctemp < CPU_TEMP_MIN)
Index: sys/kern/kern_sensors.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_sensors.c,v
retrieving revision 1.40
diff -u -p -r1.40 kern_sensors.c
--- sys/kern/kern_sensors.c 5 Dec 2022 23:18:37 -0000 1.40
+++ sys/kern/kern_sensors.c 19 May 2025 17:43:01 -0000
@@ -31,18 +31,27 @@
#include <sys/sensors.h>
#include "hotplug.h"
+/*
+ * Locks used to protect data:
+ * D sensordev_mtx
+ */
+
+struct mutex sensordev_mtx = MUTEX_INITIALIZER(IPL_MPFLOOR);
+
struct taskq *sensors_taskq;
-int sensordev_count;
+int sensordev_count; /* [D] */
SLIST_HEAD(, ksensordev) sensordev_list =
- SLIST_HEAD_INITIALIZER(sensordev_list);
+ SLIST_HEAD_INITIALIZER(sensordev_list); /* [D] */
void
sensordev_install(struct ksensordev *sensdev)
{
struct ksensordev *v, *nv;
- int s;
- s = splhigh();
+ refcnt_init(&sensdev->refcnt);
+ mtx_init(&sensdev->mtx, IPL_MPFLOOR);
+
+ mtx_enter(&sensordev_mtx);
if (sensordev_count == 0) {
sensdev->num = 0;
SLIST_INSERT_HEAD(&sensordev_list, sensdev, list);
@@ -54,8 +63,9 @@ sensordev_install(struct ksensordev *sen
sensdev->num = v->num + 1;
SLIST_INSERT_AFTER(v, sensdev, list);
}
+
sensordev_count++;
- splx(s);
+ mtx_leave(&sensordev_mtx);
#if NHOTPLUG > 0
hotplug_device_attach(DV_DULL, "sensordev");
@@ -67,9 +77,12 @@ sensor_attach(struct ksensordev *sensdev
{
struct ksensor *v, *nv;
struct ksensors_head *sh;
- int s, i;
+ int i;
+
+ sens->parent = sensdev;
+ mtx_init(&sens->mtx, IPL_MPFLOOR);
- s = splhigh();
+ mtx_enter(&sensdev->mtx);
sh = &sensdev->sensors_list;
if (sensdev->sensors_count == 0) {
for (i = 0; i < SENSOR_MAX_TYPES; i++)
@@ -95,31 +108,30 @@ sensor_attach(struct ksensordev *sensdev
if (sensdev->maxnumt[sens->type] == sens->numt)
sensdev->maxnumt[sens->type]++;
sensdev->sensors_count++;
- splx(s);
+ mtx_leave(&sensdev->mtx);
}
void
sensordev_deinstall(struct ksensordev *sensdev)
{
- int s;
-
- s = splhigh();
+ mtx_enter(&sensordev_mtx);
sensordev_count--;
SLIST_REMOVE(&sensordev_list, sensdev, ksensordev, list);
- splx(s);
+ mtx_leave(&sensordev_mtx);
#if NHOTPLUG > 0
hotplug_device_detach(DV_DULL, "sensordev");
#endif
+
+ refcnt_finalize(&sensdev->refcnt, "sensordev");
}
void
sensor_detach(struct ksensordev *sensdev, struct ksensor *sens)
{
struct ksensors_head *sh;
- int s;
- s = splhigh();
+ mtx_enter(&sensdev->mtx);
sh = &sensdev->sensors_list;
sensdev->sensors_count--;
SLIST_REMOVE(sh, sens, ksensor, list);
@@ -128,14 +140,16 @@ sensor_detach(struct ksensordev *sensdev
*/
if (sens->numt == sensdev->maxnumt[sens->type] - 1)
sensdev->maxnumt[sens->type]--;
- splx(s);
+ mtx_leave(&sensdev->mtx);
}
int
-sensordev_get(int num, struct ksensordev **sensdev)
+sensordev_get_locked(int num, struct ksensordev **sensdev)
{
struct ksensordev *sd;
+ MUTEX_ASSERT_LOCKED(&sensordev_mtx);
+
SLIST_FOREACH(sd, &sensordev_list, list) {
if (sd->num == num) {
*sensdev = sd;
@@ -148,25 +162,76 @@ sensordev_get(int num, struct ksensordev
}
int
+sensordev_get(int num, struct ksensordev **sensdev)
+{
+ int error;
+
+ mtx_enter(&sensordev_mtx);
+ if ((error = sensordev_get_locked(num, sensdev)) == 0)
+ refcnt_take(&(*sensdev)->refcnt);
+ mtx_leave(&sensordev_mtx);
+
+ return (error);
+}
+
+void
+sensordev_put(struct ksensordev *sensdev)
+{
+ refcnt_rele_wake(&sensdev->refcnt);
+}
+
+int
sensor_find(int dev, enum sensor_type type, int numt, struct ksensor **ksensorp)
{
struct ksensor *s;
struct ksensordev *sensdev;
struct ksensors_head *sh;
- int ret;
+ int error;
- ret = sensordev_get(dev, &sensdev);
- if (ret)
- return (ret);
+ error = sensordev_get(dev, &sensdev);
+ if (error)
+ return (error);
+ mtx_enter(&sensdev->mtx);
sh = &sensdev->sensors_list;
- SLIST_FOREACH(s, sh, list)
- if (s->type == type && s->numt == numt) {
- *ksensorp = s;
- return (0);
- }
+ SLIST_FOREACH(s, sh, list) {
+ if (s->type == type && s->numt == numt)
+ break;
+ }
- return (ENOENT);
+ if (s)
+ *ksensorp = s;
+ else {
+ sensordev_put(sensdev);
+ error = ENOENT;
+ }
+ mtx_leave(&sensdev->mtx);
+
+ return (error);
+}
+
+void
+sensor_rele(struct ksensor *sens)
+{
+ refcnt_rele_wake(&sens->parent->refcnt);
+}
+
+void
+sensor_lock(struct ksensor *sens)
+{
+ if (sens->flags & SENSOR_MPSAFE)
+ mtx_enter(&sens->mtx);
+ else
+ KERNEL_LOCK();
+}
+
+void
+sensor_unlock(struct ksensor *sens)
+{
+ if (sens->flags & SENSOR_MPSAFE)
+ mtx_leave(&sens->mtx);
+ else
+ KERNEL_UNLOCK();
}
struct sensor_task {
Index: sys/kern/kern_sysctl.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
retrieving revision 1.468
diff -u -p -r1.468 kern_sysctl.c
--- sys/kern/kern_sysctl.c 9 May 2025 14:53:22 -0000 1.468
+++ sys/kern/kern_sysctl.c 19 May 2025 17:43:01 -0000
@@ -848,7 +848,6 @@ hw_sysctl(int *name, u_int namelen, void
case HW_DISKSTATS:
case HW_CPUSPEED:
#ifndef SMALL_KERNEL
- case HW_SENSORS:
case HW_SETPERF:
case HW_PERFPOLICY:
case HW_BATTERY:
@@ -867,6 +866,11 @@ hw_sysctl(int *name, u_int namelen, void
sysctl_vsunlock(oldp, savelen);
return (err);
}
+#ifndef SMALL_KERNEL
+ case HW_SENSORS:
+ return (sysctl_sensors(name + 1, namelen - 1, oldp, oldlenp,
+ newp, newlen));
+#endif /* !SMALL_KERNEL */
case HW_VENDOR:
if (hw_vendor)
return (sysctl_rdstring(oldp, oldlenp, newp,
@@ -937,9 +941,6 @@ hw_sysctl_locked(int *name, u_int namele
return err;
return (sysctl_rdint(oldp, oldlenp, newp, cpuspeed));
#ifndef SMALL_KERNEL
- case HW_SENSORS:
- return (sysctl_sensors(name + 1, namelen - 1, oldp, oldlenp,
- newp, newlen));
case HW_SETPERF:
return (sysctl_hwsetperf(oldp, oldlenp, newp, newlen));
case HW_PERFPOLICY:
@@ -2786,10 +2787,15 @@ sysctl_sensors(int *name, u_int namelen,
/* Grab a copy, to clear the kernel pointers */
usd = malloc(sizeof(*usd), M_TEMP, M_WAITOK|M_ZERO);
+
+ mtx_enter(&ksd->mtx);
usd->num = ksd->num;
strlcpy(usd->xname, ksd->xname, sizeof(usd->xname));
memcpy(usd->maxnumt, ksd->maxnumt, sizeof(usd->maxnumt));
usd->sensors_count = ksd->sensors_count;
+ mtx_leave(&ksd->mtx);
+
+ sensordev_put(ksd);
ret = sysctl_rdstruct(oldp, oldlenp, newp, usd,
sizeof(struct sensordev));
@@ -2807,13 +2813,18 @@ sysctl_sensors(int *name, u_int namelen,
/* Grab a copy, to clear the kernel pointers */
us = malloc(sizeof(*us), M_TEMP, M_WAITOK|M_ZERO);
+
+ sensor_lock(ks);
memcpy(us->desc, ks->desc, sizeof(us->desc));
us->tv = ks->tv;
us->value = ks->value;
us->type = ks->type;
us->status = ks->status;
us->numt = ks->numt;
- us->flags = ks->flags;
+ us->flags = ks->flags & ~SENSOR_MPSAFE;
+ sensor_unlock(ks);
+
+ sensor_rele(ks);
ret = sysctl_rdstruct(oldp, oldlenp, newp, us,
sizeof(struct sensor));
Index: sys/sys/sensors.h
===================================================================
RCS file: /cvs/src/sys/sys/sensors.h,v
retrieving revision 1.37
diff -u -p -r1.37 sensors.h
--- sys/sys/sensors.h 15 Jul 2020 07:13:57 -0000 1.37
+++ sys/sys/sensors.h 19 May 2025 17:43:01 -0000
@@ -119,6 +119,7 @@ struct sensor {
int flags; /* sensor flags */
#define SENSOR_FINVALID 0x0001 /* sensor is invalid */
#define SENSOR_FUNKNOWN 0x0002 /* sensor value is unknown */
+#define SENSOR_MPSAFE 0x0004 /* sensor is mpsafe */
};
/* Sensor device data:
@@ -133,13 +134,29 @@ struct sensordev {
#ifdef _KERNEL
+#include <sys/refcnt.h>
+
+/*
+ * Locks used to protect data:
+ * I immutable after creation
+ * D sensordev_mtx
+ * d ksensordev's mtx
+ */
+
+struct ksensordev;
+
+extern struct mutex sensordev_mtx;
+
/* Sensor data */
struct ksensor {
- SLIST_ENTRY(ksensor) list; /* device-scope list */
- char desc[32]; /* sensor description, may be empty */
+ struct mutex mtx;
+ struct ksensordev *parent; /* [I] parent ksensordev */
+ SLIST_ENTRY(ksensor) list; /* [d] device-scope list */
+ char desc[32]; /* [I] sensor description, may be
+ empty */
struct timeval tv; /* sensor value last change time */
int64_t value; /* current value */
- enum sensor_type type; /* sensor type */
+ enum sensor_type type; /* [I] sensor type */
enum sensor_status status; /* sensor status */
int numt; /* sensor number of .type type */
int flags; /* sensor flags, ie. SENSOR_FINVALID */
@@ -148,23 +165,31 @@ SLIST_HEAD(ksensors_head, ksensor);
/* Sensor device data */
struct ksensordev {
- SLIST_ENTRY(ksensordev) list;
- int num; /* sensordev number */
- char xname[16]; /* unix device name */
- int maxnumt[SENSOR_MAX_TYPES];
- int sensors_count;
- struct ksensors_head sensors_list;
+ struct refcnt refcnt;
+ struct mutex mtx;
+ SLIST_ENTRY(ksensordev) list; /* [D] */
+ int num; /* [I] sensordev number */
+ char xname[16]; /* [I] unix device name */
+ int maxnumt[SENSOR_MAX_TYPES]; /* [d] */
+ int sensors_count; /* [d] */
+ struct ksensors_head sensors_list; /* [d] */
};
/* struct ksensordev */
void sensordev_install(struct ksensordev *);
void sensordev_deinstall(struct ksensordev *);
+int sensordev_get_locked(int, struct ksensordev **);
int sensordev_get(int, struct ksensordev **);
+void sensordev_put(struct ksensordev *);
+void sensor_lock(struct ksensor *);
+void sensor_unlock(struct ksensor *);
/* struct ksensor */
void sensor_attach(struct ksensordev *, struct ksensor *);
void sensor_detach(struct ksensordev *, struct ksensor *);
-int sensor_find(int, enum sensor_type, int, struct ksensor **);
+int sensor_find(int, enum sensor_type, int,
+ struct ksensor **);
+void sensor_rele(struct ksensor *);
/* task scheduling */
struct sensor_task;
Please test: mpsafe sensors