Xen devel - Sep 2011 - locking in drivers/xen/xen-pciback/passthrough.c:__xen_pcibk_publish_pci_roots()

Konrad,

this function, before calling the passed in callback, drops the lock it
acquired at the beginning of the function, and re-acquires it after the
callback returned. It also uses list_for_each_entry_safe() in an
apparent attempt to deal with races here. However, I''m getting the
impression that this wouldn''t really work (as the construct really
isn''t
meant for this case): If in the meantime the successor element got
removed from the list, the loop continuation would access data that
may already have got freed.

I see two possible solutions: Either after re-acquiring the lock the
function checks whether the current object is sill on the list, starting
over if not found (xen_pcibk_publish_pci_root() makes sure a
device doesn''t get published twice), or (if so possible) the lock gets
converted to a mutex (which should be safe to be held across the
callback invocation).

What are your thoughts here?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Fri, Sep 16, 2011 at 03:08:07PM +0100, Jan Beulich
wrote:
> Konrad,
> 
> this function, before calling the passed in callback, drops the lock it
> acquired at the beginning of the function, and re-acquires it after the
> callback returned. It also uses list_for_each_entry_safe() in an
> apparent attempt to deal with races here. However, I''m getting the
> impression that this wouldn''t really work (as the construct really
isn''t
> meant for this case): If in the meantime the successor element got
> removed from the list, the loop continuation would access data that
> may already have got freed.
> 
> I see two possible solutions: Either after re-acquiring the lock the
> function checks whether the current object is sill on the list, starting
> over if not found (xen_pcibk_publish_pci_root() makes sure a
> device doesn''t get published twice), or (if so possible) the lock
gets
> converted to a mutex (which should be safe to be held across the
> callback invocation).
> 
> What are your thoughts here?
I think mutexs would work here best. We don''t any of these operations
in
an IRQ interrupt context, so we are OK to be put to sleep.

I think there is some room in improvement in some of the other Xen code
in this regard - where we use spinlocks and could use a mechanism
that is OK to go to sleep.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel