| Message ID | 20160623145943.GF410@mail-itl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 23/06/16 15:59, Marek Marczykowski-Górecki wrote: > On Thu, Jun 23, 2016 at 03:12:04PM +0100, Andrew Cooper wrote: >> On 23/06/16 14:25, Marek Marczykowski-Górecki wrote: >>> On Thu, Jun 23, 2016 at 03:46:46AM -0600, Jan Beulich wrote: >>>>>>> On 23.06.16 at 11:23, <marmarek@invisiblethingslab.com> wrote: >>>>> On Thu, Jun 23, 2016 at 11:18:24AM +0200, Marek Marczykowski-Górecki wrote: >>>>>> On Thu, Jun 23, 2016 at 03:12:47AM -0600, Jan Beulich wrote: >>>>>>>>>> On 23.06.16 at 10:57, <marmarek@invisiblethingslab.com> wrote: >>>>>>>> On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote: >>>>>>>>> I wonder what good the duplication of the returned domain ID does: I'm >>>>>>>>> tempted to remove the one in the command-specific structure. Does >>>>>>>>> anyone have insight into why it was done that way? >>>>>>>> Isn't XEN_DOMCTL_getdomaininfo supposed to return info about first >>>>>>>> existing domain with ID >= passed one? Reading various comments in code >>>>>>>> it looks to be used to domain enumeration. This patch changes this >>>>>>>> behaviour. >>>>>>> No, it doesn't. It adjusts the behavior only for the DM case (which >>>>>>> isn't supposed to get information on other than the target domain, >>>>>>> i.e. in this one specific case the very domain ID needs to be passed >>>>>>> in). >>>>>> int xc_domain_getinfo(xc_interface *xch, >>>>>> uint32_t first_domid, >>>>>> unsigned int max_doms, >>>>>> xc_dominfo_t *info) >>>>>> { >>>>>> unsigned int nr_doms; >>>>>> uint32_t next_domid = first_domid; >>>>>> DECLARE_DOMCTL; >>>>>> int rc = 0; >>>>>> >>>>>> memset(info, 0, max_doms*sizeof(xc_dominfo_t)); >>>>>> >>>>>> for ( nr_doms = 0; nr_doms < max_doms; nr_doms++ ) >>>>>> { >>>>>> domctl.cmd = XEN_DOMCTL_getdomaininfo; >>>>>> domctl.domain = (domid_t)next_domid; >>>>>> if ( (rc = do_domctl(xch, &domctl)) < 0 ) >>>>>> break; >>>>>> info->domid = (uint16_t)domctl.domain; >>>>>> (...) >>>>>> next_domid = (uint16_t)domctl.domain + 1; >>>>>> >>>>>> >>>>>> Looks like heavily dependent on XEN_DOMCTL_getdomaininfo returning next >>>>> valid >>>>>> domain. >>>>> Hmm, looks like I've misread you patch. Reading again... >>>>> >>>>> But now I see rcu_read_lock(&domlist_read_lock) is gets called only when >>>>> looping over domains, but rcu_read_unlock is called in any case. Is it >>>>> correct? >>>> How that? There is this third hunk: >>> Ok, after drawing a flowchart of the control in this function after your >>> change, on a piece of paper, this case looks fine. But depending on how >>> the domain was found (explicit loop or rcu_lock_domain_by_id), different >>> locks are held, which makes it harder to follow what is going on. >>> >>> Crazy idea: how about making the code easy/easier to read instead of >>> obfuscating it even more? XEN_DOMCTL_getdomaininfo semantic is >>> convolved enough. How about this version (2 patches): >>> >>> xen: move domain lookup for getdomaininfo to the same >>> >>> XEN_DOMCTL_getdomaininfo have different semantics than most of others >>> domctls - it returns information about first valid domain with ID >= >>> argument. But that's no excuse for having the lookup done in a different >>> place, which made handling different corner cases unnecessary complex. >>> Move the lookup to the first switch clause. And adjust locking to be the >>> same as for other cases. >>> >>> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> >> FWIW, I prefer this solution to the issue. >> >> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, with a few style >> nits. > Fixed patch according to your comments: > > xen: move domain lookup for getdomaininfo to the same > > XEN_DOMCTL_getdomaininfo have different semantics than most of others > domctls - it returns information about first valid domain with ID >= > argument. But that's no excuse for having the lookup code in a different > place, which made handling different corner cases unnecessary complex. > Move the lookup to the first switch clause. And adjust locking to be the > same as for other cases. > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > xen/common/domctl.c | 44 +++++++++++++++++++++++--------------------- > 1 file changed, 23 insertions(+), 21 deletions(-) > > diff --git a/xen/common/domctl.c b/xen/common/domctl.c > index e43904e..41de3e8 100644 > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -442,11 +442,32 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) > switch ( op->cmd ) > { > case XEN_DOMCTL_createdomain: > - case XEN_DOMCTL_getdomaininfo: > case XEN_DOMCTL_test_assign_device: > case XEN_DOMCTL_gdbsx_guestmemio: > d = NULL; > break; > + > + case XEN_DOMCTL_getdomaininfo: > + d = rcu_lock_domain_by_id(op->domain); > + > + if ( d == NULL ) > + { > + /* Search for the next available domain. */ > + rcu_read_lock(&domlist_read_lock); > + > + for_each_domain ( d ) > + if ( d->domain_id >= op->domain ) > + { > + rcu_lock_domain(d); > + break; > + } > + > + rcu_read_unlock(&domlist_read_lock); > + if ( d == NULL ) > + return -ESRCH; > + } > + break; > + > default: > d = rcu_lock_domain_by_id(op->domain); > if ( d == NULL ) > @@ -862,33 +883,14 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) > break; > > case XEN_DOMCTL_getdomaininfo: > - { > - domid_t dom = op->domain; > - > - rcu_read_lock(&domlist_read_lock); > - > - for_each_domain ( d ) > - if ( d->domain_id >= dom ) > - break; > - > - ret = -ESRCH; > - if ( d == NULL ) > - goto getdomaininfo_out; > - > ret = xsm_getdomaininfo(XSM_HOOK, d); > if ( ret ) > - goto getdomaininfo_out; > + break; > > getdomaininfo(d, &op->u.getdomaininfo); > - > op->domain = op->u.getdomaininfo.domain; > copyback = 1; > - > - getdomaininfo_out: > - rcu_read_unlock(&domlist_read_lock); > - d = NULL; > break; > - } > > case XEN_DOMCTL_getvcpucontext: > {
diff --git a/xen/common/domctl.c b/xen/common/domctl.c index e43904e..41de3e8 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -442,11 +442,32 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) switch ( op->cmd ) { case XEN_DOMCTL_createdomain: - case XEN_DOMCTL_getdomaininfo: case XEN_DOMCTL_test_assign_device: case XEN_DOMCTL_gdbsx_guestmemio: d = NULL; break; + + case XEN_DOMCTL_getdomaininfo: + d = rcu_lock_domain_by_id(op->domain); + + if ( d == NULL ) + { + /* Search for the next available domain. */ + rcu_read_lock(&domlist_read_lock); + + for_each_domain ( d ) + if ( d->domain_id >= op->domain ) + { + rcu_lock_domain(d); + break; + } + + rcu_read_unlock(&domlist_read_lock); + if ( d == NULL ) + return -ESRCH; + } + break; + default: d = rcu_lock_domain_by_id(op->domain); if ( d == NULL ) @@ -862,33 +883,14 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) break; case XEN_DOMCTL_getdomaininfo: - { - domid_t dom = op->domain; - - rcu_read_lock(&domlist_read_lock); - - for_each_domain ( d ) - if ( d->domain_id >= dom ) - break; - - ret = -ESRCH; - if ( d == NULL ) - goto getdomaininfo_out; - ret = xsm_getdomaininfo(XSM_HOOK, d); if ( ret ) - goto getdomaininfo_out; + break; getdomaininfo(d, &op->u.getdomaininfo); - op->domain = op->u.getdomaininfo.domain; copyback = 1; - - getdomaininfo_out: - rcu_read_unlock(&domlist_read_lock); - d = NULL; break; - } case XEN_DOMCTL_getvcpucontext: {