Xen devel - Jul 2013 - [PATCH v2] x86/mm: Ensure useful progress in alloc_l2_table()

While debugging the issue which turned out to be XSA-58, a printk in this loop
showed that it was quite easy to never make useful progress, because of
consistently failing the preemption check.

One single l2 entry is a reasonable amount of work to do, even if an action is
pending, and also assures forwards progress across repeat continuations.

Tweak the continuation criteria to fail on the first iteration of the loop.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Keir <keir@xen.org>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Tim Deegan <tim@xen.org>

---
Changes since v1:
 * Dont move the continuation block to the end
---
 xen/arch/x86/mm.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 77dcafc..f151dec 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1278,7 +1278,8 @@ static int alloc_l2_table(struct page_info *page, unsigned
long type,
 
     for ( i = page->nr_validated_ptes; i < L2_PAGETABLE_ENTRIES; i++ )
     {
-        if ( preemptible && i && hypercall_preempt_check() )
+        if ( preemptible && i > page->nr_validated_ptes
+             && hypercall_preempt_check() )
         {
             page->nr_validated_ptes = i;
             rc = -EAGAIN;
-- 
1.7.10.4

On 03/07/2013 13:28, "Andrew Cooper" <andrew.cooper3@citrix.com>
wrote:
> While debugging the issue which turned out to be XSA-58, a printk in this
loop
> showed that it was quite easy to never make useful progress, because of
> consistently failing the preemption check.
> 
> One single l2 entry is a reasonable amount of work to do, even if an action
is
> pending, and also assures forwards progress across repeat continuations.
> 
> Tweak the continuation criteria to fail on the first iteration of the loop.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Keir <keir@xen.org>
Acked-by: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Tim Deegan <tim@xen.org>
> 
> ---
> Changes since v1:
>  * Dont move the continuation block to the end
> ---
>  xen/arch/x86/mm.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 77dcafc..f151dec 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -1278,7 +1278,8 @@ static int alloc_l2_table(struct page_info *page,
> unsigned long type,
>  
>      for ( i = page->nr_validated_ptes; i < L2_PAGETABLE_ENTRIES; i++
)
>      {
> -        if ( preemptible && i && hypercall_preempt_check()
)
> +        if ( preemptible && i > page->nr_validated_ptes
> +             && hypercall_preempt_check() )
>          {
>              page->nr_validated_ptes = i;
>              rc = -EAGAIN;