paging_domctl() missing break statements?

paging_domctl() missing break statements?

[Jan Beulich]

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

Re: paging_domctl() missing break statements?

[Tim Deegan]

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:

RE: paging_domctl() missing break statements?

[Dan Magenheimer]

/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

Re: paging_domctl() missing break statements?

[Tim Deegan]

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));

Re: paging_domctl() missing break statements?

[Paolo Bonzini]

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

Re: paging_domctl() missing break statements?

[Keir Fraser]

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

Re: paging_domctl() missing break statements?

[Tim Deegan]

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)

Re: Re: paging_domctl() missing break statements?

[Patrick Colp]

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
>
>

Re: Re: paging_domctl() missing break statements?

[Jan Beulich]

>>> 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

[PATCH] Re: Re: paging_domctl() missing break statements?

[Tim Deegan]

[-- Attachment #1: Type: text/plain, Size: 941 bytes --]

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)

[-- Attachment #2: shadow-disable --]
[-- Type: text/plain, Size: 2874 bytes --]

diff -r 4001ab0d5785 xen/arch/x86/mm/paging.c
--- a/xen/arch/x86/mm/paging.c	Fri Jun 25 13:23:49 2010 +0100
+++ b/xen/arch/x86/mm/paging.c	Fri Jun 25 14:35:19 2010 +0100
@@ -700,23 +700,21 @@
      */
     switch ( sc->op )
     {
+
+    case XEN_DOMCTL_SHADOW_OP_ENABLE:
+        if ( !(sc->mode & XEN_DOMCTL_SHADOW_ENABLE_LOG_DIRTY) )
+            break;
+        /* Else fall through... */
     case XEN_DOMCTL_SHADOW_OP_ENABLE_LOGDIRTY:
         if ( hap_enabled(d) )
             hap_logdirty_init(d);
         return paging_log_dirty_enable(d);
 
-    case XEN_DOMCTL_SHADOW_OP_ENABLE:
-        if ( sc->mode & XEN_DOMCTL_SHADOW_ENABLE_LOG_DIRTY )
-        {
-            if ( hap_enabled(d) )
-                hap_logdirty_init(d);
-            return paging_log_dirty_enable(d);
-        }
-
     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:
diff -r 4001ab0d5785 xen/arch/x86/mm/shadow/common.c
--- a/xen/arch/x86/mm/shadow/common.c	Fri Jun 25 13:23:49 2010 +0100
+++ b/xen/arch/x86/mm/shadow/common.c	Fri Jun 25 14:35:19 2010 +0100
@@ -3241,9 +3241,12 @@
         {
             int i;
             mfn_t *oos_snapshot = v->arch.paging.shadow.oos_snapshot;
-            for(i = 0; i < SHADOW_OOS_PAGES; i++)
+            for ( i = 0; i < SHADOW_OOS_PAGES; i++ )
                 if ( mfn_valid(oos_snapshot[i]) )
+                {
                     shadow_free(d, oos_snapshot[i]);
+                    oos_snapshot[i] = _mfn(INVALID_MFN);
+                }
         }
 #endif /* OOS */
     }
@@ -3395,17 +3398,23 @@
 #endif
                 make_cr3(v, pagetable_get_pfn(v->arch.guest_table));
 
+#if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC) 
+            {
+                int i;
+                mfn_t *oos_snapshot = v->arch.paging.shadow.oos_snapshot;
+                for ( i = 0; i < SHADOW_OOS_PAGES; i++ )
+                    if ( mfn_valid(oos_snapshot[i]) )
+                    {
+                        shadow_free(d, oos_snapshot[i]);
+                        oos_snapshot[i] = _mfn(INVALID_MFN);
+                    }
+            }
+#endif /* OOS */
         }
 
         /* Pull down the memory allocation */
         if ( sh_set_allocation(d, 0, NULL) != 0 )
-        {
-            // XXX - How can this occur?
-            //       Seems like a bug to return an error now that we've
-            //       disabled the relevant shadow mode.
-            //
-            return -ENOMEM;
-        }
+            BUG(); /* In fact, we will have BUG()ed already */
         shadow_hash_teardown(d);
         SHADOW_PRINTK("un-shadowing of domain %u done."
                        "  Shadow pages total = %u, free = %u, p2m=%u\n",

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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