On Fri, Jan 9, 2015 at 2:07 PM, Martin Pieuchot <[email protected]> wrote: > On 09/01/15(Fri) 12:36, David Higgs wrote: >> 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. >> [...] >> > + >> > + 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. >
The updated diff appears to works for me and I don't see any further issues upon review. No behavior changes when applied to a snapshot + HEAD kernel as of this evening. Tested via repeated sysctl reads of upd(4) sensor info and a few device detach/attach events. --david
