Hi, Oliver:
I thought that the anchor would be very useful for the usblp cleanup,
so I looked at the patch. Short summary is, I like the general idea,
but not the cost of implementation.
> @@ -1161,6 +1176,8 @@ struct urb
> /* public: documented fields in the urb that can be used by drivers */
> struct list_head urb_list; /* list head for use by the urb's
> * current owner */
> + struct list_head anchor_list; /* the URB may be anchored by the
> driver */
> + struct usb_anchor *anchor;
> struct usb_device *dev; /* (in) pointer to associated device */
So, you add 24 bytes to all URBs, which are... not very thin, to be sure.
Last time I counted they were 152 bytes apiece. Still, a 15% increase.
I know you're a good algorithmist, are you sure you don't have any ideas?
The naive approach is to have anchor elements out of line... In a slab,
since they are fixed-sized. What do you think?
Now, the details...
#1. Why are the new fields public? I don't see a reason to let drivers
to poke at them.
#2. I'm not comfortable with all these list_empty() out of spinlocks:
> +void usb_unanchor_urb(struct urb *urb)
....
> + spin_unlock_irqrestore(&anchor->lock, flags);
> + usb_put_urb(urb);
> + if (list_empty(&anchor->urb_list))
> + wake_up(&anchor->wait);
> +int usb_wait_anchor_empty_timeout(struct usb_anchor *anchor,
> + unsigned int timeout)
> +{
> + return wait_event_timeout(anchor->wait, list_empty(&anchor->urb_list),
> + msecs_to_jiffies(timeout));
I understand that list_empty is just two loads and a compare, and at
worst we'll get a false negative. However, modern compilers optimize
so much that I don't trust anything anymore.
-- Pete
-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
