Xen devel - Jul 2013 - [PATCH] 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.

Move the preemption check to the end of the loop, and adjust it so as to not
cause a continuation if we have completed the l2 table.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Keir <keir@xen.org>
Cc: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/mm.c |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 77dcafc..245838b 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1278,13 +1278,6 @@ 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() )
-        {
-            page->nr_validated_ptes = i;
-            rc = -EAGAIN;
-            break;
-        }
-
         if ( !is_guest_l2_slot(d, type, i) ||
              (rc = get_page_from_l2e(pl2e[i], pfn, d)) > 0 )
             continue;
@@ -1299,6 +1292,14 @@ static int alloc_l2_table(struct page_info *page,
unsigned long type,
         }
 
         adjust_guest_l2e(pl2e[i], d);
+
+        if ( preemptible && i != L2_PAGETABLE_ENTRIES
+             && hypercall_preempt_check() )
+        {
+            page->nr_validated_ptes = i + 1;
+            rc = -EAGAIN;
+            break;
+        }
     }
 
     if ( rc >= 0 && (type & PGT_pae_xen_l2) )
-- 
1.7.10.4

>>> On 03.07.13 at 13:37, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -1278,13 +1278,6 @@ 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()
)
> -        {
> -            page->nr_validated_ptes = i;
> -            rc = -EAGAIN;
> -            break;
> -        }
> -
>          if ( !is_guest_l2_slot(d, type, i) ||
>               (rc = get_page_from_l2e(pl2e[i], pfn, d)) > 0 )
>              continue;
> @@ -1299,6 +1292,14 @@ static int alloc_l2_table(struct page_info *page,
unsigned long type,
>          }
>  
>          adjust_guest_l2e(pl2e[i], d);
> +
> +        if ( preemptible && i != L2_PAGETABLE_ENTRIES
        if ( preemptible && i != L2_PAGETABLE_ENTRIES - 1

But an even smaller change would be to change the if() you remove 
above to

        if ( preemptible && i > page->nr_validated_ptes &&
            hypercall_preempt_check() )

Jan
> +             && hypercall_preempt_check() )
> +        {
> +            page->nr_validated_ptes = i + 1;
> +            rc = -EAGAIN;
> +            break;
> +        }
>      }
>  
>      if ( rc >= 0 && (type & PGT_pae_xen_l2) )

On 03/07/13 13:16, Jan Beulich wrote:
>>>> On 03.07.13 at 13:37, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -1278,13 +1278,6 @@ 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() )
>> -        {
>> -            page->nr_validated_ptes = i;
>> -            rc = -EAGAIN;
>> -            break;
>> -        }
>> -
>>          if ( !is_guest_l2_slot(d, type, i) ||
>>               (rc = get_page_from_l2e(pl2e[i], pfn, d)) > 0 )
>>              continue;
>> @@ -1299,6 +1292,14 @@ static int alloc_l2_table(struct page_info
*page, unsigned long type,
>>          }
>>  
>>          adjust_guest_l2e(pl2e[i], d);
>> +
>> +        if ( preemptible && i != L2_PAGETABLE_ENTRIES
>         if ( preemptible && i != L2_PAGETABLE_ENTRIES - 1
>
> But an even smaller change would be to change the if() you remove 
> above to
>
>         if ( preemptible && i > page->nr_validated_ptes
&&
>             hypercall_preempt_check() )
>
> Jan
So it will - I shall resend v2

~Andrew
>
>> +             && hypercall_preempt_check() )
>> +        {
>> +            page->nr_validated_ptes = i + 1;
>> +            rc = -EAGAIN;
>> +            break;
>> +        }
>>      }
>>  
>>      if ( rc >= 0 && (type & PGT_pae_xen_l2) )
>