Anthony Liguori
2005-Jul-15 14:40 UTC
[Xen-devel] Re: [Xen-changelog] Simpler domid allocation.
Hi Keir, This change worries me a bit because I believe it increases the likelihood of subtle race conditions in the tools. It''s now likely that if one destroys a domain and immediately creates a new domain, when the tools finally see the VIRQ, they''ll be very confused since the domid appears to be valid. The situation is worse for a destroyed domain. A destroy does not generate a VIRQ in which case if the destroy and create happen within whatever the tools polling interval is, it will appear to the tool that the destroy just didn''t work. The solution would be to have a completely serialized tool chain although that prevents having multiple simulatenous tools running at the same time or a distribute tool chain where most things don''t require a central daemon. The old domid allocation was odd but it kept life easier by making race conditions difficult to create. Regards, Anthony Liguori Xen patchbot -unstable wrote:># HG changeset patch ># User kaf24@firebug.cl.cam.ac.uk ># Node ID 8d04aa7b42805d6d15c651abe13249c4d2eefaf7 ># Parent 1d026c7023d28aad1a409638d4f1db518a80bdfd > >Simpler domid allocation. >Signed-off-by: Keir Fraser <keir@xensource.com> > >diff -r 1d026c7023d2 -r 8d04aa7b4280 xen/common/dom0_ops.c >--- a/xen/common/dom0_ops.c Thu Jul 14 23:48:06 2005 >+++ b/xen/common/dom0_ops.c Fri Jul 15 07:53:46 2005 >@@ -37,55 +37,6 @@ > > put_domain(d); > return 0; >-} >- >-/* >- * Allocate a free domain id. We try to reuse domain ids in a fairly low range, >- * only expanding the range when there are no free domain ids. This is to keep >- * domain ids in a range depending on the number that exist simultaneously, >- * rather than incrementing domain ids in the full 32-bit range. >- */ >-static int allocate_domid(domid_t *pdom) >-{ >- static spinlock_t domid_lock = SPIN_LOCK_UNLOCKED; >- static domid_t curdom = 0; >- static domid_t topdom = 101; >- int err = 0; >- domid_t dom; >- >- spin_lock(&domid_lock); >- >- /* Try to use a domain id in the range 0..topdom, starting at curdom. */ >- for ( dom = curdom + 1; dom != curdom; dom++ ) >- { >- if ( dom == topdom ) >- dom = 1; >- if ( is_free_domid(dom) ) >- goto exit; >- } >- >- /* Couldn''t find a free domain id in 0..topdom, try higher. */ >- for ( dom = topdom; dom < DOMID_FIRST_RESERVED; dom++ ) >- { >- if ( is_free_domid(dom) ) >- { >- topdom = dom + 1; >- goto exit; >- } >- } >- >- /* No free domain ids. */ >- err = -ENOMEM; >- >- exit: >- if ( err == 0 ) >- { >- curdom = dom; >- *pdom = dom; >- } >- >- spin_unlock(&domid_lock); >- return err; > } > > static void getdomaininfo(struct domain *d, dom0_getdomaininfo_t *info) >@@ -217,18 +168,33 @@ > domid_t dom; > struct vcpu *v; > unsigned int i, cnt[NR_CPUS] = { 0 }; >- >+ static spinlock_t alloc_lock = SPIN_LOCK_UNLOCKED; >+ static domid_t rover = 0; >+ >+ spin_lock(&alloc_lock); > > dom = op->u.createdomain.domain; > if ( (dom > 0) && (dom < DOMID_FIRST_RESERVED) ) > { > ret = -EINVAL; > if ( !is_free_domid(dom) ) >- break; >- } >- else if ( (ret = allocate_domid(&dom)) != 0 ) >- { >- break; >+ goto alloc_out; >+ } >+ else >+ { >+ for ( dom = rover + 1; dom != rover; dom++ ) >+ { >+ if ( dom == DOMID_FIRST_RESERVED ) >+ dom = 0; >+ if ( is_free_domid(dom) ) >+ break; >+ } >+ >+ ret = -ENOMEM; >+ if ( dom == rover ) >+ goto alloc_out; >+ >+ rover = dom; > } > > /* Do an initial CPU placement. Pick the least-populated CPU. */ >@@ -249,11 +215,12 @@ > pro = i; > > ret = -ENOMEM; >- if ( (d = do_createdomain(dom, pro)) == NULL ) >- break; >- >- ret = 0; >- >+ if ( (d = do_createdomain(dom, pro)) != NULL ) >+ ret = 0; >+ >+ alloc_out: >+ spin_unlock(&alloc_lock); >+ > op->u.createdomain.domain = d->domain_id; > copy_to_user(u_dom0_op, op, sizeof(*op)); > } > >_______________________________________________ >Xen-changelog mailing list >Xen-changelog@lists.xensource.com >http://lists.xensource.com/xen-changelog > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2005-Jul-15 15:17 UTC
[Xen-devel] Re: [Xen-changelog] Simpler domid allocation.
I think you misread the allocation scheme -- it still allocates increasing domid''s in turn but just doesn;t arbitrarily decide to wrap at e.g., domid==100. So your fears over immediate domid reuse are unfounded. I understand your fears w.r.t. decentralised tools. One thing I think we will end up doing is adding the domain uuid (big unique domain identifier) to Xen. Xen will still work in terms of the short 16-bit domids, but control tools will be able to read out this extra unique key value which they can use to protect themselves against domid reuse in the absence of some other trusted authority. Invaluable for debugging screwed-up machine states too. :-) -- Keir On 15 Jul 2005, at 15:40, Anthony Liguori wrote:> This change worries me a bit because I believe it increases the > likelihood of subtle race conditions in the tools. It''s now likely > that if one destroys a domain and immediately creates a new domain, > when the tools finally see the VIRQ, they''ll be very confused since > the domid appears to be valid. > > The situation is worse for a destroyed domain. A destroy does not > generate a VIRQ in which case if the destroy and create happen within > whatever the tools polling interval is, it will appear to the tool > that the destroy just didn''t work. > > The solution would be to have a completely serialized tool chain > although that prevents having multiple simulatenous tools running at > the same time or a distribute tool chain where most things don''t > require a central daemon. > > The old domid allocation was odd but it kept life easier by making > race conditions difficult to create._______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Anthony Liguori
2005-Jul-15 15:40 UTC
[Xen-devel] Re: [Xen-changelog] Simpler domid allocation.
Keir Fraser wrote:> > I think you misread the allocation scheme -- it still allocates > increasing domid''s in turn but just doesn;t arbitrarily decide to wrap > at e.g., domid==100. So your fears over immediate domid reuse are > unfounded.Indeed. I read the removed comment as the new functionality. Should not read patches before having any caffeine :-)> I understand your fears w.r.t. decentralised tools. One thing I think > we will end up doing is adding the domain uuid (big unique domain > identifier) to Xen. Xen will still work in terms of the short 16-bit > domids, but control tools will be able to read out this extra unique > key value which they can use to protect themselves against domid reuse > in the absence of some other trusted authority. Invaluable for > debugging screwed-up machine states too. :-)Excellent. The UUID would be part of the struct domain in Xen? Would Xen create it and the tools just have an interface to get it? I''d be happy to submit a patch to add this as it would be very useful for the tools (since it effectively eliminates the possibility of race conditions). Thanks, Anthony Liguori> -- Keir > > On 15 Jul 2005, at 15:40, Anthony Liguori wrote: > >> This change worries me a bit because I believe it increases the >> likelihood of subtle race conditions in the tools. It''s now likely >> that if one destroys a domain and immediately creates a new domain, >> when the tools finally see the VIRQ, they''ll be very confused since >> the domid appears to be valid. >> >> The situation is worse for a destroyed domain. A destroy does not >> generate a VIRQ in which case if the destroy and create happen within >> whatever the tools polling interval is, it will appear to the tool >> that the destroy just didn''t work. >> >> The solution would be to have a completely serialized tool chain >> although that prevents having multiple simulatenous tools running at >> the same time or a distribute tool chain where most things don''t >> require a central daemon. >> >> The old domid allocation was odd but it kept life easier by making >> race conditions difficult to create. > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2005-Jul-15 15:48 UTC
[Xen-devel] Re: [Xen-changelog] Simpler domid allocation.
On 15 Jul 2005, at 16:40, Anthony Liguori wrote:>> I understand your fears w.r.t. decentralised tools. One thing I think >> we will end up doing is adding the domain uuid (big unique domain >> identifier) to Xen. Xen will still work in terms of the short 16-bit >> domids, but control tools will be able to read out this extra unique >> key value which they can use to protect themselves against domid >> reuse in the absence of some other trusted authority. Invaluable for >> debugging screwed-up machine states too. :-) > > Excellent. The UUID would be part of the struct domain in Xen? Would > Xen create it and the tools just have an interface to get it? I''d be > happy to submit a patch to add this as it would be very useful for the > tools (since it effectively eliminates the possibility of race > conditions).No, it would just be a 16-byte array settable via a dom0_op and returned by getdomaininfo. No reason for Xen to be responsible for creating it, and it has downsides: 1. It would stop us from using an allocation strategy that 100% guarantees cluster-wide uniqueness 2. We''d like domains to keep their uuids across reboots. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel