The anchor patch critique

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