The main switch statement in that function looks suspicious, and with no explicit comment saying that fall-through is intended it would seem like one or two break statements are actually missing. Comments? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2010-Feb-17 09:58 UTC
Re: [Xen-devel] paging_domctl() missing break statements?
At 09:48 +0000 on 17 Feb (1266400095), Jan Beulich wrote:> The main switch statement in that function looks suspicious, and with no > explicit comment saying that fall-through is intended it would seem like > one or two break statements are actually missing. Comments?Yep, looks like that was just working by blind luck. Tim. diff -r 560277d2fd20 xen/arch/x86/mm/paging.c --- a/xen/arch/x86/mm/paging.c Mon Feb 15 08:19:07 2010 +0000 +++ b/xen/arch/x86/mm/paging.c Wed Feb 17 09:56:43 2010 +0000 @@ -717,11 +717,13 @@ hap_logdirty_init(d); return paging_log_dirty_enable(d); } + break; case XEN_DOMCTL_SHADOW_OP_OFF: if ( paging_mode_log_dirty(d) ) if ( (rc = paging_log_dirty_disable(d)) != 0 ) return rc; + break; case XEN_DOMCTL_SHADOW_OP_CLEAN: case XEN_DOMCTL_SHADOW_OP_PEEK: _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Dan Magenheimer
2010-Feb-17 15:20 UTC
RE: [Xen-devel] paging_domctl() missing break statements?
/me wonders if this explains the periodic but apparently harmless messages I often see on the console like: (XEN) paging.c:170: paging_free_log_dirty_bitmap: used X pages for domain Y dirty logging which I''ve never reported. And, if not, is that message useful/meaningful to anyone or should it be removed?> -----Original Message----- > From: Tim Deegan [mailto:Tim.Deegan@citrix.com] > Sent: Wednesday, February 17, 2010 2:58 AM > To: Jan Beulich > Cc: xen-devel@lists.xensource.com > Subject: Re: [Xen-devel] paging_domctl() missing break statements? > > At 09:48 +0000 on 17 Feb (1266400095), Jan Beulich wrote: > > The main switch statement in that function looks suspicious, and with > no > > explicit comment saying that fall-through is intended it would seem > like > > one or two break statements are actually missing. Comments? > > Yep, looks like that was just working by blind luck. > > Tim. > > diff -r 560277d2fd20 xen/arch/x86/mm/paging.c > --- a/xen/arch/x86/mm/paging.c Mon Feb 15 08:19:07 2010 +0000 > +++ b/xen/arch/x86/mm/paging.c Wed Feb 17 09:56:43 2010 +0000 > @@ -717,11 +717,13 @@ > hap_logdirty_init(d); > return paging_log_dirty_enable(d); > } > + break; > > case XEN_DOMCTL_SHADOW_OP_OFF: > if ( paging_mode_log_dirty(d) ) > if ( (rc = paging_log_dirty_disable(d)) != 0 ) > return rc; > + break; > > case XEN_DOMCTL_SHADOW_OP_CLEAN: > case XEN_DOMCTL_SHADOW_OP_PEEK: > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2010-Feb-17 17:36 UTC
Re: [Xen-devel] paging_domctl() missing break statements?
At 15:20 +0000 on 17 Feb (1266420027), Dan Magenheimer wrote:> /me wonders if this explains the periodic but apparently harmless > messages I often see on the console like: > > (XEN) paging.c:170: paging_free_log_dirty_bitmap: used X pages for domain Y dirty logging > > which I''ve never reported.No, that''s just some unrelated noise; I think it went in when the log-dirty bitmaps were made sparse.> And, if not, is that message useful/meaningful to anyone or > should it be removed?It should be removed. Signed-off-by: Tim Deegan <Tim.Deegan@citrix.com> diff -r 560277d2fd20 xen/arch/x86/mm/paging.c --- a/xen/arch/x86/mm/paging.c Mon Feb 15 08:19:07 2010 +0000 +++ b/xen/arch/x86/mm/paging.c Wed Feb 17 17:32:02 2010 +0000 @@ -165,9 +165,6 @@ if ( !mfn_valid(d->arch.paging.log_dirty.top) ) return; - - dprintk(XENLOG_DEBUG, "%s: used %d pages for domain %d dirty logging\n", - __FUNCTION__, d->arch.paging.log_dirty.allocs, d->domain_id); l4 = map_domain_page(mfn_x(d->arch.paging.log_dirty.top)); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Paolo Bonzini
2010-Jun-23 12:27 UTC
[Xen-devel] Re: paging_domctl() missing break statements?
On 02/17/2010 10:58 AM, Tim Deegan wrote:> At 09:48 +0000 on 17 Feb (1266400095), Jan Beulich wrote: >> The main switch statement in that function looks suspicious, and with no >> explicit comment saying that fall-through is intended it would seem like >> one or two break statements are actually missing. Comments? > > Yep, looks like that was just working by blind luck. > > Tim. > > diff -r 560277d2fd20 xen/arch/x86/mm/paging.c > --- a/xen/arch/x86/mm/paging.c Mon Feb 15 08:19:07 2010 +0000 > +++ b/xen/arch/x86/mm/paging.c Wed Feb 17 09:56:43 2010 +0000 > @@ -717,11 +717,13 @@ > hap_logdirty_init(d); > return paging_log_dirty_enable(d); > } > + break; > > case XEN_DOMCTL_SHADOW_OP_OFF: > if ( paging_mode_log_dirty(d) ) > if ( (rc = paging_log_dirty_disable(d)) != 0 ) > return rc; > + break; > > case XEN_DOMCTL_SHADOW_OP_CLEAN: > case XEN_DOMCTL_SHADOW_OP_PEEK:This was never applied. Paolo _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2010-Jun-23 12:51 UTC
[Xen-devel] Re: paging_domctl() missing break statements?
On 23/06/2010 13:27, "Paolo Bonzini" <pbonzini@redhat.com> wrote:>>> The main switch statement in that function looks suspicious, and with no >>> explicit comment saying that fall-through is intended it would seem like >>> one or two break statements are actually missing. Comments? >> >> Yep, looks like that was just working by blind luck. >> >> Tim.> This was never applied.It was applied as 20954:b4041e7bbe1b but then reverted as 20987:c4301c2c727d: """ Revert 20954:b4041e7bbe1b "paging_domctl: Add missing breaks in switch stmt" This fixed a fairly innocuous bug (OP_ENABLE/OP_OFF both don''t work properly) but unmasked a much nastier one (turning off shadow mode on a PV guest crashes the hypervisor). So, for now, we pick the less of two evils. We don''t really much rely on OP_ENABLE/OP_OFF anyway, as it happens. Signed-off-by: Keir Fraser <keir.fraser@citrix.com> """ -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2010-Jun-23 12:51 UTC
[Xen-devel] Re: paging_domctl() missing break statements?
At 13:27 +0100 on 23 Jun (1277299651), Paolo Bonzini wrote:> This was never applied.It was applied and then reverted because it caused Xen crashes: http://xenbits.xensource.com/xen-unstable.hg?rev/c4301c2c727d and nobody''s got round to properly reworking it. Cheers, Tim. -- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, XenServer Engineering Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Patrick Colp
2010-Jun-23 16:27 UTC
Re: [Xen-devel] Re: paging_domctl() missing break statements?
The problem with the patch is that with the break statements in the
"else" cases of XEN_DOMCTL_SHADOW_OP_ENABLE and
XEN_DOMCTL_SHADOW_OP_OFF it currently falls through. Simply sticking
break in at the bottom changes these control flow paths.
So a (more) proper patch should replicate the code of
XEN_DOMCTL_SHADOW_OP_OFF (and of OP_CLEAN and OP_PEEK):
if ( paging_mode_log_dirty(d) )
if ( (rc = paging_log_dirty_disable(d)) != 0 )
return rc;
and
return paging_log_dirty_op(d, sc);
before the break statement. Same with the XEN_DOMCTL_SHADOW_OP_OFF
case statement... it should have:
return paging_log_dirty_op(d, sc);
after the initial if statement as its "else" case.
I can whip up a patch to do that, although it''s not clear to me that
this is entirely cleaner. Or that XEN_DOMCTL_SHADOW_OP_ENABLE needs
the XEN_DOMCTL_SHADOW_OP_OFF code. This would have to be analysed more
carefully (unless somebody knows the answer off the top of their
head). It could just be that in the "else" case for
XEN_DOMCTL_SHADOW_OP_ENABLE it should just do "return
paging_log_dirty_op(d, sc);". Any thoughts?
Patrick
On 23 June 2010 05:51, Tim Deegan <Tim.Deegan@citrix.com>
wrote:> At 13:27 +0100 on 23 Jun (1277299651), Paolo Bonzini wrote:
>> This was never applied.
>
> It was applied and then reverted because it caused Xen crashes:
> http://xenbits.xensource.com/xen-unstable.hg?rev/c4301c2c727d
> and nobody''s got round to properly reworking it.
>
> Cheers,
>
> Tim.
>
> --
> Tim Deegan <Tim.Deegan@citrix.com>
> Principal Software Engineer, XenServer Engineering
> Citrix Systems UK Ltd. (Company #02937203, SL9 0BG)
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Jan Beulich
2010-Jun-24 07:10 UTC
Re: [Xen-devel] Re: paging_domctl() missing break statements?
>>> On 23.06.10 at 18:27, Patrick Colp <pjcolp@cs.ubc.ca> wrote: > The problem with the patch is that with the break statements in the > "else" cases of XEN_DOMCTL_SHADOW_OP_ENABLE and > XEN_DOMCTL_SHADOW_OP_OFF it currently falls through. Simply sticking > break in at the bottom changes these control flow paths. > > So a (more) proper patch should replicate the code of > XEN_DOMCTL_SHADOW_OP_OFF (and of OP_CLEAN and OP_PEEK): > > if ( paging_mode_log_dirty(d) ) > if ( (rc = paging_log_dirty_disable(d)) != 0 ) > return rc; > > and > > return paging_log_dirty_op(d, sc); > > before the break statement. Same with the XEN_DOMCTL_SHADOW_OP_OFF > case statement... it should have: > > return paging_log_dirty_op(d, sc); > > after the initial if statement as its "else" case.That means you consider the current behavior right, i.e. the fall through being intentional. If that''s indeed the case, rather than replicating the code I''d suggest just annotating the code to state that the fall through is intentional in both places (as is common practice elsewhere). Jan Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2010-Jun-25 13:50 UTC
[PATCH] Re: [Xen-devel] Re: paging_domctl() missing break statements?
At 17:27 +0100 on 23 Jun (1277314052), Patrick Colp wrote:> The problem with the patch is that with the break statements in the > "else" cases of XEN_DOMCTL_SHADOW_OP_ENABLE and > XEN_DOMCTL_SHADOW_OP_OFF it currently falls through. Simply sticking > break in at the bottom changes these control flow paths.Yep - and that would be the right thing to do; the problem is that it unmasked a different bug that had crept into the shadow-disable code. Since this bug was harmless, the other bug was a hypervisor crash, and the release was imminent, we just reverted to the old code. The attached patch reinstates the breaks in the flow control (and folds two identical cases together) and fixes the memory leak that was causing the crash. Signed-off-by: Tim Deegan <Tim.Deegan@citrix.com> Tim. -- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, XenServer Engineering Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel