Date: Sat, 29 Aug 2020 04:29:53 +0000 (UTC) From: Warner Losh <imp@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r364944 - head/sys/kern Message-ID: <202008290429.07T4TrbH007764@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
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().
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202008290429.07T4TrbH007764>