On 09/01/15(Fri) 12:36, David Higgs wrote: > On Fri, Jan 9, 2015 at 9:00 AM, Martin Pieuchot <mpieuc...@nolizard.org> > wrote: > > As pointed out by David Higgs, uhidev report functions do a lot of > > memory allocations and copies between buffers. This is not uncommon > > in USB land due to way DMA buffers are handled. > > > > One way to reduce the number of copies is to submit a transfer with > > the USBD_NO_COPY flag. Without it the kernel does two to three copies: > > > > (stack/driver array <-->) malloc'ed array <--> DMA reacheable array > > > > This is not limited to uhidev functions. That's also done for every > > transfer sent through an interrupt pipe or for every control request > > with data. > > > > I'd like to slowly change the stack such that USBD_NO_COPY becomes the > > default behavior. This would first reduces the number of buffer copies > > and then allow umass(4) to directly use the DMA reachable buffers > > provided by the SCSI stack, which should *at least* reduce the CPU usage > > when using USB drives. > > > > Here's a diff about uhidev(4) to illustrate the changes I'm talking > > about. > > > > Comments, ok? > > > > See inline. Reviewed to the best of my ability/eyesight via > variable-width webmail. > [...] > > + > > + if (id > 0) > > + len++; > > + > > + buf = usbd_alloc_buffer(xfer, len); > > + if (buf == NULL) > > + return (-1); > > You forgot to free xfer in this error path.
Indeed! > > @@ -586,6 +585,7 @@ usbd_status > > usbd_clear_endpoint_stall_async(struct usbd_pipe *pipe) > > { > > struct usbd_device *dev = pipe->device; > > + struct usbd_xfer *xfer; > > usb_device_request_t req; > > usbd_status err; > > > > @@ -596,7 +596,12 @@ usbd_clear_endpoint_stall_async(struct u > > USETW(req.wValue, UF_ENDPOINT_HALT); > > USETW(req.wIndex, pipe->endpoint->edesc->bEndpointAddress); > > USETW(req.wLength, 0); > > - err = usbd_do_request_async(dev, &req, 0, 0, 0); > > + > > + xfer = usbd_alloc_xfer(dev); > > + if (xfer == NULL) > > + return (USBD_NOMEM); > > + > > + err = usbd_request_async(xfer, &req, NULL, NULL); > > Missing xfer cleanup here too, I think. Here it's fine, as soon as you call usbd_request_async() the xfer will be freed. Updated diff below. Index: uhidev.c =================================================================== RCS file: /cvs/src/sys/dev/usb/uhidev.c,v retrieving revision 1.68 diff -u -p -r1.68 uhidev.c --- uhidev.c 9 Jan 2015 12:09:51 -0000 1.68 +++ uhidev.c 9 Jan 2015 19:03:45 -0000 @@ -671,18 +671,30 @@ int uhidev_set_report_async(struct uhidev_softc *sc, int type, int id, void *data, int len) { + struct usbd_xfer *xfer; usb_device_request_t req; - char *buf = data; int actlen = len; + char *buf; + + xfer = usbd_alloc_xfer(sc->sc_udev); + if (xfer == NULL) + return (-1); + + if (id > 0) + len++; + + buf = usbd_alloc_buffer(xfer, len); + if (buf == NULL) { + usbd_free_xfer(xfer); + return (-1); + } /* Prepend the reportID. */ if (id > 0) { - len++; - buf = malloc(len, M_TEMP, M_NOWAIT); - if (buf == NULL) - return (-1); buf[0] = id; memcpy(buf + 1, data, len - 1); + } else { + memcpy(buf, data, len); } req.bmRequestType = UT_WRITE_CLASS_INTERFACE; @@ -691,17 +703,9 @@ uhidev_set_report_async(struct uhidev_so USETW(req.wIndex, sc->sc_ifaceno); USETW(req.wLength, len); - if (usbd_do_request_async(sc->sc_udev, &req, buf, NULL, NULL)) + if (usbd_request_async(xfer, &req, NULL, NULL)) actlen = -1; - /* - * Since report requests are write-only it is safe to free - * the buffer right after submitting the transfer because - * it won't be used afterward. - */ - if (id > 0) - free(buf, M_TEMP, len); - return (actlen); } @@ -750,11 +754,11 @@ uhidev_get_report_async_cb(struct usbd_x if (info->id > 0) { len--; memcpy(info->data, xfer->buffer + 1, len); + } else { + memcpy(info->data, xfer->buffer, len); } } info->callback(info->priv, info->id, info->data, len); - if (info->id > 0) - free(xfer->buffer, M_TEMP, xfer->length); free(info, M_TEMP, sizeof(*info)); usbd_free_xfer(xfer); } @@ -763,42 +767,47 @@ int uhidev_get_report_async(struct uhidev_softc *sc, int type, int id, void *data, int len, void *priv, void (*callback)(void *, int, void *, int)) { + struct usbd_xfer *xfer; usb_device_request_t req; struct uhidev_async_info *info; - char *buf = data; int actlen = len; + char *buf; + + xfer = usbd_alloc_xfer(sc->sc_udev); + if (xfer == NULL) + return (-1); + + if (id > 0) + len++; + + buf = usbd_alloc_buffer(xfer, len); + if (buf == NULL) { + usbd_free_xfer(xfer); + return (-1); + } info = malloc(sizeof(*info), M_TEMP, M_NOWAIT); - if (info == NULL) + if (info == NULL) { + usbd_free_xfer(xfer); return (-1); + } info->callback = callback; info->priv = priv; info->data = data; info->id = id; - if (id > 0) { - len++; - buf = malloc(len, M_TEMP, M_NOWAIT|M_ZERO); - if (buf == NULL) { - free(info, M_TEMP, sizeof(*info)); - return (-1); - } - } - req.bmRequestType = UT_READ_CLASS_INTERFACE; req.bRequest = UR_GET_REPORT; USETW2(req.wValue, type, id); USETW(req.wIndex, sc->sc_ifaceno); USETW(req.wLength, len); - if (usbd_do_request_async(sc->sc_udev, &req, buf, priv, - uhidev_get_report_async_cb)) { + if (usbd_request_async(xfer, &req, priv, uhidev_get_report_async_cb)) { free(info, M_TEMP, sizeof(*info)); - if (id > 0) - free(buf, M_TEMP, len); actlen = -1; } + return (actlen); } Index: usbdi.c =================================================================== RCS file: /cvs/src/sys/dev/usb/usbdi.c,v retrieving revision 1.77 diff -u -p -r1.77 usbdi.c --- usbdi.c 9 Jan 2015 12:15:48 -0000 1.77 +++ usbdi.c 9 Jan 2015 13:22:02 -0000 @@ -55,8 +55,7 @@ extern int usbdebug; #define DPRINTFN(n,x) #endif -void usbd_do_request_async_cb(struct usbd_xfer *, void *, - usbd_status); +void usbd_request_async_cb(struct usbd_xfer *, void *, usbd_status); void usbd_start_next(struct usbd_pipe *pipe); usbd_status usbd_open_pipe_ival(struct usbd_interface *, u_int8_t, u_int8_t, struct usbd_pipe **, int); @@ -586,6 +585,7 @@ usbd_status usbd_clear_endpoint_stall_async(struct usbd_pipe *pipe) { struct usbd_device *dev = pipe->device; + struct usbd_xfer *xfer; usb_device_request_t req; usbd_status err; @@ -596,7 +596,12 @@ usbd_clear_endpoint_stall_async(struct u USETW(req.wValue, UF_ENDPOINT_HALT); USETW(req.wIndex, pipe->endpoint->edesc->bEndpointAddress); USETW(req.wLength, 0); - err = usbd_do_request_async(dev, &req, 0, 0, 0); + + xfer = usbd_alloc_xfer(dev); + if (xfer == NULL) + return (USBD_NOMEM); + + err = usbd_request_async(xfer, &req, NULL, NULL); return (err); } @@ -949,8 +954,7 @@ usbd_do_request_flags(struct usbd_device } void -usbd_do_request_async_cb(struct usbd_xfer *xfer, void *priv, - usbd_status status) +usbd_request_async_cb(struct usbd_xfer *xfer, void *priv, usbd_status status) { usbd_free_xfer(xfer); } @@ -960,19 +964,17 @@ usbd_do_request_async_cb(struct usbd_xfe * Can be used from interrupt context. */ usbd_status -usbd_do_request_async(struct usbd_device *dev, usb_device_request_t *req, - void *data, void *priv, usbd_callback callback) +usbd_request_async(struct usbd_xfer *xfer, usb_device_request_t *req, + void *priv, usbd_callback callback) { - struct usbd_xfer *xfer; usbd_status err; - xfer = usbd_alloc_xfer(dev); - if (xfer == NULL) - return (USBD_NOMEM); if (callback == NULL) - callback = usbd_do_request_async_cb; - usbd_setup_default_xfer(xfer, dev, priv, USBD_DEFAULT_TIMEOUT, req, - data, UGETW(req->wLength), 0, callback); + callback = usbd_request_async_cb; + + usbd_setup_default_xfer(xfer, xfer->device, priv, + USBD_DEFAULT_TIMEOUT, req, NULL, UGETW(req->wLength), + USBD_NO_COPY, callback); err = usbd_transfer(xfer); if (err != USBD_IN_PROGRESS) { usbd_free_xfer(xfer); Index: usbdi.h =================================================================== RCS file: /cvs/src/sys/dev/usb/usbdi.h,v retrieving revision 1.64 diff -u -p -r1.64 usbdi.h --- usbdi.h 9 Jan 2015 12:07:50 -0000 1.64 +++ usbdi.h 9 Jan 2015 13:14:25 -0000 @@ -121,8 +121,8 @@ usbd_status usbd_open_pipe_intr(struct u void *buffer, u_int32_t length, usbd_callback, int); usbd_status usbd_do_request(struct usbd_device *pipe, usb_device_request_t *req, void *data); -usbd_status usbd_do_request_async(struct usbd_device *pipe, - usb_device_request_t *req, void *data, void *priv, usbd_callback callback); +usbd_status usbd_request_async(struct usbd_xfer *xfer, + usb_device_request_t *req, void *priv, usbd_callback callback); usbd_status usbd_do_request_flags(struct usbd_device *pipe, usb_device_request_t *req, void *data, u_int16_t flags, int*, u_int32_t); usb_interface_descriptor_t *usbd_get_interface_descriptor(