It is generally preferable to not hold locks around allocation
functions. And indeed in the hwdom case there's no point at all to hold
the paging lock. Reduce the locked regions to the non-hwdom case, while
at the same time arranging for p2m_alloc_page() to have just a single
return point.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -48,7 +48,6 @@ static struct page_info *p2m_alloc_page(
 {
     struct page_info *pg;
 
-    spin_lock(&d->arch.paging.lock);
     /*
      * For hardware domain, there should be no limit in the number of pages that
      * can be allocated, so that the kernel may take advantage of the extended
@@ -58,34 +57,28 @@ static struct page_info *p2m_alloc_page(
     {
         pg = alloc_domheap_page(NULL, 0);
         if ( pg == NULL )
-        {
             printk(XENLOG_G_ERR "Failed to allocate P2M pages for hwdom.\n");
-            spin_unlock(&d->arch.paging.lock);
-            return NULL;
-        }
     }
     else
     {
+        spin_lock(&d->arch.paging.lock);
         pg = page_list_remove_head(&d->arch.paging.p2m_freelist);
-        if ( unlikely(!pg) )
-        {
-            spin_unlock(&d->arch.paging.lock);
-            return NULL;
-        }
+        spin_unlock(&d->arch.paging.lock);
     }
-    spin_unlock(&d->arch.paging.lock);
 
     return pg;
 }
 
 static void p2m_free_page(struct domain *d, struct page_info *pg)
 {
-    spin_lock(&d->arch.paging.lock);
     if ( is_hardware_domain(d) )
         free_domheap_page(pg);
     else
+    {
+        spin_lock(&d->arch.paging.lock);
         page_list_add_tail(pg, &d->arch.paging.p2m_freelist);
-    spin_unlock(&d->arch.paging.lock);
+        spin_unlock(&d->arch.paging.lock);
+    }
 }
 
 /* Return the size of the pool, in bytes. */

Hi Jan,

On 29/11/2022 15:39, Jan Beulich wrote:
> It is generally preferable to not hold locks around allocation
> functions. And indeed in the hwdom case there's no point at all to hold
> the paging lock. Reduce the locked regions to the non-hwdom case, while
> at the same time arranging for p2m_alloc_page() to have just a single
> return point.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

> 
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -48,7 +48,6 @@ static struct page_info *p2m_alloc_page(
>   {
>       struct page_info *pg;
>   
> -    spin_lock(&d->arch.paging.lock);
>       /*
>        * For hardware domain, there should be no limit in the number of pages that
>        * can be allocated, so that the kernel may take advantage of the extended
> @@ -58,34 +57,28 @@ static struct page_info *p2m_alloc_page(
>       {
>           pg = alloc_domheap_page(NULL, 0);
>           if ( pg == NULL )
> -        {
>               printk(XENLOG_G_ERR "Failed to allocate P2M pages for hwdom.\n");
> -            spin_unlock(&d->arch.paging.lock);
> -            return NULL;
> -        }
>       }
>       else
>       {
> +        spin_lock(&d->arch.paging.lock);
>           pg = page_list_remove_head(&d->arch.paging.p2m_freelist);
> -        if ( unlikely(!pg) )
> -        {
> -            spin_unlock(&d->arch.paging.lock);
> -            return NULL;
> -        }
> +        spin_unlock(&d->arch.paging.lock);
>       }
> -    spin_unlock(&d->arch.paging.lock);
>   
>       return pg;
>   }
>   
>   static void p2m_free_page(struct domain *d, struct page_info *pg)
>   {
> -    spin_lock(&d->arch.paging.lock);
>       if ( is_hardware_domain(d) )
>           free_domheap_page(pg);
>       else
> +    {
> +        spin_lock(&d->arch.paging.lock);
>           page_list_add_tail(pg, &d->arch.paging.p2m_freelist);
> -    spin_unlock(&d->arch.paging.lock);
> +        spin_unlock(&d->arch.paging.lock);
> +    }
>   }
>   
>   /* Return the size of the pool, in bytes. */

-- 
Julien Grall

On 29/11/2022 15:53, Julien Grall wrote:
> Hi Jan,
> 
> On 29/11/2022 15:39, Jan Beulich wrote:
>> It is generally preferable to not hold locks around allocation
>> functions. And indeed in the hwdom case there's no point at all to hold
>> the paging lock. Reduce the locked regions to the non-hwdom case, while
>> at the same time arranging for p2m_alloc_page() to have just a single
>> return point.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Julien Grall <jgrall@amazon.com>

It is now committed.

Cheers,

-- 
Julien Grall