Am Mittwoch, 6. Juni 2007 06:18 schrieb Pete Zaitcev:
> 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?
I figured the URB will end up in the same buckets for SLAB anyway due to
granularity.
> The naive approach is to have anchor elements out of line... In a slab,
> since they are fixed-sized. What do you think?
Balanced against a possible cache miss in interrupt. I want to keep
the runtime penalty small.
> Now, the details...
>
> #1. Why are the new fields public? I don't see a reason to let drivers
> to poke at them.
True. I'll change that.
> #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.
Then fix the primitive. The list handling primitives have to work as
documented.
Secondly, you must stop adding URBs to an anchor if you want to
kill its URBs, or there's a race that cannot be easily solved, unless
you want usb_anchor_urb() to return error codes.
Regards
Oliver
-------------------------------------------------------------------------
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/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
