On Fri, Jan 9, 2015 at 9:00 AM, Martin Pieuchot <[email protected]> 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.
> 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 13:31:36 -0000
> @@ -671,18 +671,28 @@ 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)
> + return (-1);
You forgot to free xfer in this error path.
>
> /* 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 +701,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 +752,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 +765,48 @@ 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;
> + void *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);
> + usbd_free_xfer(xfer);
> 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);
Missing xfer cleanup here too, I think.
> 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(
