Author: imp
Date: Sat Aug 29 04:29:53 2020
New Revision: 364944
URL: https://svnweb.freebsd.org/changeset/base/364944
Log:
devctl: move to using a uma zone
Convert the memory management of devctl. Rewrite if to make better
use of memory. This eliminates several mallocs (5? worse case) needed
to send a message. It's now possible to always send a message, though
if things are really backed up the oldest message will be dropped to
free up space for the newest.
Add a static bus_child_{location,pnpinfo}_sb to start migrating to
sbuf instead of buffer + length. Use it in the new code. Other code
will be converted later (bus_child_*_str is only used inside of
subr_bus.c, though implemented in ~100 places in the tree).
Reviewed by: markj@
Differential Revision: https://reviews.freebsd.org/D26140
Modified:
head/sys/kern/subr_bus.c
Modified: head/sys/kern/subr_bus.c
==============================================================================
--- head/sys/kern/subr_bus.c Sat Aug 29 02:46:25 2020 (r364943)
+++ head/sys/kern/subr_bus.c Sat Aug 29 04:29:53 2020 (r364944)
@@ -156,6 +156,8 @@ EVENTHANDLER_LIST_DEFINE(device_attach);
EVENTHANDLER_LIST_DEFINE(device_detach);
EVENTHANDLER_LIST_DEFINE(dev_lookup);
+static int bus_child_location_sb(device_t child, struct sbuf *sb);
+static int bus_child_pnpinfo_sb(device_t child, struct sbuf *sb);
static void devctl2_init(void);
static bool device_frozen;
@@ -392,9 +394,10 @@ static struct cdevsw dev_cdevsw = {
.d_name = "devctl",
};
+#define DEVCTL_BUFFER (1024 - sizeof(void *))
struct dev_event_info {
- char *dei_data;
STAILQ_ENTRY(dev_event_info) dei_link;
+ char dei_data[DEVCTL_BUFFER];
};
STAILQ_HEAD(devq, dev_event_info);
@@ -409,6 +412,7 @@ static struct dev_softc {
struct selinfo sel;
struct devq devq;
struct sigio *sigio;
+ uma_zone_t zone;
} devsoftc;
static void filt_devctl_detach(struct knote *kn);
@@ -431,6 +435,11 @@ devinit(void)
cv_init(&devsoftc.cv, "dev cv");
STAILQ_INIT(&devsoftc.devq);
knlist_init_mtx(&devsoftc.sel.si_note, &devsoftc.mtx);
+ if (devctl_queue_length > 0) {
+ devsoftc.zone = uma_zcreate("DEVCTL", sizeof(struct
dev_event_info),
+ NULL, NULL, NULL, NULL, UMA_ALIGN_PTR, UMA_ZONE_NOFREE);
+ uma_prealloc(devsoftc.zone, devctl_queue_length);
+ }
devctl2_init();
}
@@ -495,8 +504,7 @@ devread(struct cdev *dev, struct uio *uio, int ioflag)
devsoftc.queued--;
mtx_unlock(&devsoftc.mtx);
rv = uiomove(n1->dei_data, strlen(n1->dei_data), uio);
- free(n1->dei_data, M_BUS);
- free(n1, M_BUS);
+ uma_zfree(devsoftc.zone, n1);
return (rv);
}
@@ -585,42 +593,51 @@ devctl_process_running(void)
return (devsoftc.inuse == 1);
}
-/**
- * @brief Queue data to be read from the devctl device
- *
- * Generic interface to queue data to the devctl device. It is
- * assumed that @p data is properly formatted. It is further assumed
- * that @p data is allocated using the M_BUS malloc type.
- */
-static void
-devctl_queue_data_f(char *data, int flags)
+static struct dev_event_info *
+devctl_alloc_dei(void)
{
- struct dev_event_info *n1 = NULL, *n2 = NULL;
+ struct dev_event_info *dei = NULL;
- if (strlen(data) == 0)
- goto out;
+ mtx_lock(&devsoftc.mtx);
if (devctl_queue_length == 0)
goto out;
- n1 = malloc(sizeof(*n1), M_BUS, flags);
- if (n1 == NULL)
- goto out;
- n1->dei_data = data;
- mtx_lock(&devsoftc.mtx);
- if (devctl_queue_length == 0) {
- mtx_unlock(&devsoftc.mtx);
- free(n1->dei_data, M_BUS);
- free(n1, M_BUS);
- return;
- }
- /* Leave at least one spot in the queue... */
- while (devsoftc.queued > devctl_queue_length - 1) {
- n2 = STAILQ_FIRST(&devsoftc.devq);
+ if (devctl_queue_length == devsoftc.queued) {
+ dei = STAILQ_FIRST(&devsoftc.devq);
STAILQ_REMOVE_HEAD(&devsoftc.devq, dei_link);
- free(n2->dei_data, M_BUS);
- free(n2, M_BUS);
devsoftc.queued--;
+ } else {
+ /* dei can't be NULL -- we know we have at least one in the
zone */
+ dei = uma_zalloc(devsoftc.zone, M_NOWAIT);
+ MPASS(dei != NULL);
}
- STAILQ_INSERT_TAIL(&devsoftc.devq, n1, dei_link);
+ *dei->dei_data = '\0';
+out:
+ mtx_unlock(&devsoftc.mtx);
+ return (dei);
+}
+
+static struct dev_event_info *
+devctl_alloc_dei_sb(struct sbuf *sb)
+{
+ struct dev_event_info *dei;
+
+ dei = devctl_alloc_dei();
+ if (dei != NULL)
+ sbuf_new(sb, dei->dei_data, sizeof(dei->dei_data),
SBUF_FIXEDLEN);
+ return (dei);
+}
+
+static void
+devctl_free_dei(struct dev_event_info *dei)
+{
+ uma_zfree(devsoftc.zone, dei);
+}
+
+static void
+devctl_queue(struct dev_event_info *dei)
+{
+ mtx_lock(&devsoftc.mtx);
+ STAILQ_INSERT_TAIL(&devsoftc.devq, dei, dei_link);
devsoftc.queued++;
cv_broadcast(&devsoftc.cv);
KNOTE_LOCKED(&devsoftc.sel.si_note, 0);
@@ -628,55 +645,38 @@ devctl_queue_data_f(char *data, int flags)
selwakeup(&devsoftc.sel);
if (devsoftc.async && devsoftc.sigio != NULL)
pgsigio(&devsoftc.sigio, SIGIO, 0);
- return;
-out:
- /*
- * We have to free data on all error paths since the caller
- * assumes it will be free'd when this item is dequeued.
- */
- free(data, M_BUS);
- return;
}
-static void
-devctl_queue_data(char *data)
-{
- devctl_queue_data_f(data, M_NOWAIT);
-}
-
/**
* @brief Send a 'notification' to userland, using standard ways
*/
void
devctl_notify_f(const char *system, const char *subsystem, const char *type,
- const char *data, int flags)
+ const char *data, int flags __unused)
{
- int len = 0;
- char *msg;
+ struct dev_event_info *dei;
+ struct sbuf sb;
- if (system == NULL)
- return; /* BOGUS! Must specify system. */
- if (subsystem == NULL)
- return; /* BOGUS! Must specify subsystem. */
- if (type == NULL)
- return; /* BOGUS! Must specify type. */
- len += strlen(" system=") + strlen(system);
- len += strlen(" subsystem=") + strlen(subsystem);
- len += strlen(" type=") + strlen(type);
- /* add in the data message plus newline. */
- if (data != NULL)
- len += strlen(data);
- len += 3; /* '!', '\n', and NUL */
- msg = malloc(len, M_BUS, flags);
- if (msg == NULL)
- return; /* Drop it on the floor */
- if (data != NULL)
- snprintf(msg, len, "!system=%s subsystem=%s type=%s %s\n",
- system, subsystem, type, data);
+ if (system == NULL || subsystem == NULL || type == NULL)
+ return;
+ dei = devctl_alloc_dei_sb(&sb);
+ if (dei == NULL)
+ return;
+ sbuf_cpy(&sb, "!system=");
+ sbuf_cat(&sb, system);
+ sbuf_cat(&sb, " subsystem=");
+ sbuf_cat(&sb, subsystem);
+ sbuf_cat(&sb, " type=");
+ sbuf_cat(&sb, type);
+ if (data != NULL) {
+ sbuf_putc(&sb, ' ');
+ sbuf_cat(&sb, data);
+ }
+ sbuf_putc(&sb, '\n');
+ if (sbuf_finish(&sb) != 0)
+ devctl_free_dei(dei); /* overflow -> drop it */
else
- snprintf(msg, len, "!system=%s subsystem=%s type=%s\n",
- system, subsystem, type);
- devctl_queue_data_f(msg, flags);
+ devctl_queue(dei);
}
void
@@ -698,53 +698,46 @@ devctl_notify(const char *system, const char *subsyste
* object of that event, plus the plug and play info and location info
* for that event. This is likely most useful for devices, but less
* useful for other consumers of this interface. Those should use
- * the devctl_queue_data() interface instead.
+ * the devctl_notify() interface instead.
+ *
+ * Output:
+ * ${type}${what} at $(location dev) $(pnp-info dev) on $(parent dev)
*/
static void
devaddq(const char *type, const char *what, device_t dev)
{
- char *data = NULL;
- char *loc = NULL;
- char *pnp = NULL;
+ struct dev_event_info *dei;
const char *parstr;
+ struct sbuf sb;
- if (!devctl_queue_length)/* Rare race, but lost races safely discard */
+ dei = devctl_alloc_dei_sb(&sb);
+ if (dei == NULL)
return;
- data = malloc(1024, M_BUS, M_NOWAIT);
- if (data == NULL)
- goto bad;
+ sbuf_cpy(&sb, type);
+ sbuf_cat(&sb, what);
+ sbuf_cat(&sb, " at ");
- /* get the bus specific location of this device */
- loc = malloc(1024, M_BUS, M_NOWAIT);
- if (loc == NULL)
- goto bad;
- *loc = '\0';
- bus_child_location_str(dev, loc, 1024);
+ /* Add in the location */
+ bus_child_location_sb(dev, &sb);
+ sbuf_putc(&sb, ' ');
- /* Get the bus specific pnp info of this device */
- pnp = malloc(1024, M_BUS, M_NOWAIT);
- if (pnp == NULL)
- goto bad;
- *pnp = '\0';
- bus_child_pnpinfo_str(dev, pnp, 1024);
+ /* Add in pnpinfo */
+ bus_child_pnpinfo_sb(dev, &sb);
/* Get the parent of this device, or / if high enough in the tree. */
if (device_get_parent(dev) == NULL)
parstr = "."; /* Or '/' ? */
else
parstr = device_get_nameunit(device_get_parent(dev));
- /* String it all together. */
- snprintf(data, 1024, "%s%s at %s %s on %s\n", type, what, loc, pnp,
- parstr);
- free(loc, M_BUS);
- free(pnp, M_BUS);
- devctl_queue_data(data);
+ sbuf_cat(&sb, " on ");
+ sbuf_cat(&sb, parstr);
+ sbuf_putc(&sb, '\n');
+ if (sbuf_finish(&sb) != 0)
+ goto bad;
+ devctl_queue(dei);
return;
bad:
- free(pnp, M_BUS);
- free(loc, M_BUS);
- free(data, M_BUS);
- return;
+ devctl_free_dei(dei);
}
/*
@@ -786,7 +779,6 @@ devnomatch(device_t dev)
static int
sysctl_devctl_queue(SYSCTL_HANDLER_ARGS)
{
- struct dev_event_info *n1;
int q, error;
q = devctl_queue_length;
@@ -795,18 +787,32 @@ sysctl_devctl_queue(SYSCTL_HANDLER_ARGS)
return (error);
if (q < 0)
return (EINVAL);
- if (mtx_initialized(&devsoftc.mtx))
- mtx_lock(&devsoftc.mtx);
- devctl_queue_length = q;
- while (devsoftc.queued > devctl_queue_length) {
- n1 = STAILQ_FIRST(&devsoftc.devq);
- STAILQ_REMOVE_HEAD(&devsoftc.devq, dei_link);
- free(n1->dei_data, M_BUS);
- free(n1, M_BUS);
- devsoftc.queued--;
+
+ /*
+ * When set as a tunable, we've not yet initialized the mutex.
+ * It is safe to just assign to devctl_queue_length and return
+ * as we're racing no one. We'll use whatever value set in
+ * devinit.
+ */
+ if (!mtx_initialized(&devsoftc.mtx)) {
+ devctl_queue_length = q;
+ return (0);
}
- if (mtx_initialized(&devsoftc.mtx))
- mtx_unlock(&devsoftc.mtx);
+
+ /*
+ * XXX It's hard to grow or shrink the UMA zone. Only allow
+ * disabling the queue size for the moment until underlying
+ * UMA issues can be sorted out.
+ */
+ if (q != 0)
+ return (EINVAL);
+ if (q == devctl_queue_length)
+ return (0);
+ mtx_lock(&devsoftc.mtx);
+ devctl_queue_length = 0;
+ uma_zdestroy(devsoftc.zone);
+ devsoftc.zone = 0;
+ mtx_unlock(&devsoftc.mtx);
return (0);
}
@@ -4918,6 +4924,70 @@ bus_child_location_str(device_t child, char *buf, size
}
return (BUS_CHILD_LOCATION_STR(parent, child, buf, buflen));
}
+
+/**
+ * @brief Wrapper function for bus_child_pnpinfo_str using sbuf
+ *
+ * A convenient wrapper frunction for bus_child_pnpinfo_str that allows
+ * us to splat that into an sbuf. It uses unholy knowledge of sbuf to
+ * accomplish this, however. It is an interim function until we can convert
+ * this interface more fully.
+ */
+/* Note: we reach inside of sbuf because it's API isn't rich enough to do this
*/
+#define SPACE(s) ((s)->s_size - (s)->s_len)
+#define EOB(s) ((s)->s_buf + (s)->s_len)
+
+static int
+bus_child_pnpinfo_sb(device_t dev, struct sbuf *sb)
+{
+ char *p;
+ size_t space;
+
+ MPASS((sb->s_flags & SBUF_INCLUDENUL) == 0);
+ if (sb->s_error != 0)
+ return (-1);
+ p = EOB(sb);
+ *p = '\0'; /* sbuf buffer isn't NUL terminated until sbuf_finish()
*/
+ space = SPACE(sb);
+ if (space <= 1) {
+ sb->s_error = ENOMEM;
+ return (-1);
+ }
+ bus_child_pnpinfo_str(dev, p, space);
+ sb->s_len += strlen(p);
+ return (0);
+}
+
+/**
+ * @brief Wrapper function for bus_child_pnpinfo_str using sbuf
+ *
+ * A convenient wrapper frunction for bus_child_pnpinfo_str that allows
+ * us to splat that into an sbuf. It uses unholy knowledge of sbuf to
+ * accomplish this, however. It is an interim function until we can convert
+ * this interface more fully.
+ */
+static int
+bus_child_location_sb(device_t dev, struct sbuf *sb)
+{
+ char *p;
+ size_t space;
+
+ MPASS((sb->s_flags & SBUF_INCLUDENUL) == 0);
+ if (sb->s_error != 0)
+ return (-1);
+ p = EOB(sb);
+ *p = '\0'; /* sbuf buffer isn't NUL terminated until sbuf_finish()
*/
+ space = SPACE(sb);
+ if (space <= 1) {
+ sb->s_error = ENOMEM;
+ return (-1);
+ }
+ bus_child_location_str(dev, p, space);
+ sb->s_len += strlen(p);
+ return (0);
+}
+#undef SPACE
+#undef EOB
/**
* @brief Wrapper function for BUS_GET_CPUS().
_______________________________________________
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
