On 29/07/2009 09:52, "Juergen Gross" <juergen.gross@xxxxxxxxxxxxxx> wrote:
>>> I can add an explicit check not to unassign borrowed cpus, if you like.
>> 
>> Your new interface ought to be responsible for its own synchronisation
>> needs. And if it's not you should implement the appropriate assertions
>> regarding e.g., spin_is_locked(), plus a code comment. It's simple
>> negligence to do neither.
>
> You are right.
> I will add a check to ensure borrowed cpus are not allowed to change the pool.
A couple more comments.
It is not safe to domain_pause() while you hold locks. It can deadlock, as
domain_pause() waits for the domain to be descheduled, but it could be
spinning on a lock you hold. Also it looks like a domain can be moved away
from a pool while the pool is paused, and then you would leak a pause
refcount.
Secondly, I think that the cpupool_borrow/return calls should be embedded
within vcpu_{lock,unlock,locked_change}_affinity(); also I see no need to
have cpupool_return_cpu() return anything as you should be able to make a
decision to move onto another CPU on the next scheduling round anyway (which
can always be forced by setting SCHEDULE_SOFTIRQ).
Really I dislike this patch greatly, as you can tell. ;-) The patchset as a
whole is *ginormous*, the Xen patch by itself is pretty big and complicated
and I believe full of races and deadlocks. I've just picked up on a few
obvious ones from a very brief read.
 -- Keir
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel