[PATCH v3] gnttab: defer allocation of status frame tracking array

[PATCH v3] gnttab: defer allocation of status frame tracking array

[Jan Beulich]

This array can be large when many grant frames are permitted; avoid
allocating it when it's not going to be used anyway, by doing this only
in gnttab_populate_status_frames().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: Drop smp_wmb(). Re-base.
v2: Defer allocation to when a domain actually switches to the v2 grant
    API.

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1747,6 +1747,17 @@ gnttab_populate_status_frames(struct dom
     /* Make sure, prior version checks are architectural visible */
     block_speculation();
 
+    if ( gt->status == ZERO_BLOCK_PTR )
+    {
+        gt->status = xzalloc_array(grant_status_t *,
+                                   grant_to_status_frames(gt->max_grant_frames));
+        if ( !gt->status )
+        {
+            gt->status = ZERO_BLOCK_PTR;
+            return -ENOMEM;
+        }
+    }
+
     for ( i = nr_status_frames(gt); i < req_status_frames; i++ )
     {
         if ( (gt->status[i] = alloc_xenheap_page()) == NULL )
@@ -1767,18 +1778,23 @@ status_alloc_failed:
         free_xenheap_page(gt->status[i]);
         gt->status[i] = NULL;
     }
+    if ( !nr_status_frames(gt) )
+    {
+        xfree(gt->status);
+        gt->status = ZERO_BLOCK_PTR;
+    }
     return -ENOMEM;
 }
 
 static int
 gnttab_unpopulate_status_frames(struct domain *d, struct grant_table *gt)
 {
-    unsigned int i;
+    unsigned int i, n = nr_status_frames(gt);
 
     /* Make sure, prior version checks are architectural visible */
     block_speculation();
 
-    for ( i = 0; i < nr_status_frames(gt); i++ )
+    for ( i = 0; i < n; i++ )
     {
         struct page_info *pg = virt_to_page(gt->status[i]);
         gfn_t gfn = gnttab_get_frame_gfn(gt, true, i);
@@ -1833,12 +1849,11 @@ gnttab_unpopulate_status_frames(struct d
         page_set_owner(pg, NULL);
     }
 
-    for ( i = 0; i < nr_status_frames(gt); i++ )
-    {
-        free_xenheap_page(gt->status[i]);
-        gt->status[i] = NULL;
-    }
     gt->nr_status_frames = 0;
+    for ( i = 0; i < n; i++ )
+        free_xenheap_page(gt->status[i]);
+    xfree(gt->status);
+    gt->status = ZERO_BLOCK_PTR;
 
     return 0;
 }
@@ -1969,11 +1984,11 @@ int grant_table_init(struct domain *d, i
     if ( gt->shared_raw == NULL )
         goto out;
 
-    /* Status pages for grant table - for version 2 */
-    gt->status = xzalloc_array(grant_status_t *,
-                               grant_to_status_frames(gt->max_grant_frames));
-    if ( gt->status == NULL )
-        goto out;
+    /*
+     * Status page tracking array for v2 gets allocated on demand. But don't
+     * leave a NULL pointer there.
+     */
+    gt->status = ZERO_BLOCK_PTR;
 
     grant_write_lock(gt);
 
@@ -4047,11 +4062,12 @@ int gnttab_acquire_resource(
         if ( gt->gt_version != 2 )
             break;
 
+        rc = gnttab_get_status_frame_mfn(d, final_frame, &tmp);
+
         /* Check that void ** is a suitable representation for gt->status. */
         BUILD_BUG_ON(!__builtin_types_compatible_p(
                          typeof(gt->status), grant_status_t **));
         vaddrs = (void **)gt->status;
-        rc = gnttab_get_status_frame_mfn(d, final_frame, &tmp);
         break;
     }
 

Ping: [PATCH v3] gnttab: defer allocation of status frame tracking array

[Jan Beulich]

On 15.04.2021 11:41, Jan Beulich wrote:
> This array can be large when many grant frames are permitted; avoid
> allocating it when it's not going to be used anyway, by doing this only
> in gnttab_populate_status_frames().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I know there has been controversy here. Julien - you seemed to agree,
and iirc you partly drove how the patch is looking now. May I ask for
an ack? Andrew - you disagreed for reasons that neither Julien nor I
could really understand. Would you firmly nack the change and suggest
a way out, or would you allow this to go in with someone else's ack?

Thanks, Jan

> ---
> v3: Drop smp_wmb(). Re-base.
> v2: Defer allocation to when a domain actually switches to the v2 grant
>     API.
> 
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1747,6 +1747,17 @@ gnttab_populate_status_frames(struct dom
>      /* Make sure, prior version checks are architectural visible */
>      block_speculation();
>  
> +    if ( gt->status == ZERO_BLOCK_PTR )
> +    {
> +        gt->status = xzalloc_array(grant_status_t *,
> +                                   grant_to_status_frames(gt->max_grant_frames));
> +        if ( !gt->status )
> +        {
> +            gt->status = ZERO_BLOCK_PTR;
> +            return -ENOMEM;
> +        }
> +    }
> +
>      for ( i = nr_status_frames(gt); i < req_status_frames; i++ )
>      {
>          if ( (gt->status[i] = alloc_xenheap_page()) == NULL )
> @@ -1767,18 +1778,23 @@ status_alloc_failed:
>          free_xenheap_page(gt->status[i]);
>          gt->status[i] = NULL;
>      }
> +    if ( !nr_status_frames(gt) )
> +    {
> +        xfree(gt->status);
> +        gt->status = ZERO_BLOCK_PTR;
> +    }
>      return -ENOMEM;
>  }
>  
>  static int
>  gnttab_unpopulate_status_frames(struct domain *d, struct grant_table *gt)
>  {
> -    unsigned int i;
> +    unsigned int i, n = nr_status_frames(gt);
>  
>      /* Make sure, prior version checks are architectural visible */
>      block_speculation();
>  
> -    for ( i = 0; i < nr_status_frames(gt); i++ )
> +    for ( i = 0; i < n; i++ )
>      {
>          struct page_info *pg = virt_to_page(gt->status[i]);
>          gfn_t gfn = gnttab_get_frame_gfn(gt, true, i);
> @@ -1833,12 +1849,11 @@ gnttab_unpopulate_status_frames(struct d
>          page_set_owner(pg, NULL);
>      }
>  
> -    for ( i = 0; i < nr_status_frames(gt); i++ )
> -    {
> -        free_xenheap_page(gt->status[i]);
> -        gt->status[i] = NULL;
> -    }
>      gt->nr_status_frames = 0;
> +    for ( i = 0; i < n; i++ )
> +        free_xenheap_page(gt->status[i]);
> +    xfree(gt->status);
> +    gt->status = ZERO_BLOCK_PTR;
>  
>      return 0;
>  }
> @@ -1969,11 +1984,11 @@ int grant_table_init(struct domain *d, i
>      if ( gt->shared_raw == NULL )
>          goto out;
>  
> -    /* Status pages for grant table - for version 2 */
> -    gt->status = xzalloc_array(grant_status_t *,
> -                               grant_to_status_frames(gt->max_grant_frames));
> -    if ( gt->status == NULL )
> -        goto out;
> +    /*
> +     * Status page tracking array for v2 gets allocated on demand. But don't
> +     * leave a NULL pointer there.
> +     */
> +    gt->status = ZERO_BLOCK_PTR;
>  
>      grant_write_lock(gt);
>  
> @@ -4047,11 +4062,12 @@ int gnttab_acquire_resource(
>          if ( gt->gt_version != 2 )
>              break;
>  
> +        rc = gnttab_get_status_frame_mfn(d, final_frame, &tmp);
> +
>          /* Check that void ** is a suitable representation for gt->status. */
>          BUILD_BUG_ON(!__builtin_types_compatible_p(
>                           typeof(gt->status), grant_status_t **));
>          vaddrs = (void **)gt->status;
> -        rc = gnttab_get_status_frame_mfn(d, final_frame, &tmp);
>          break;
>      }
>  
> 

Re: Ping: [PATCH v3] gnttab: defer allocation of status frame tracking array

[Julien Grall]

Hi Jan,

On 29/04/2021 10:31, Jan Beulich wrote:
> On 15.04.2021 11:41, Jan Beulich wrote:
>> This array can be large when many grant frames are permitted; avoid
>> allocating it when it's not going to be used anyway, by doing this only
>> in gnttab_populate_status_frames().
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> I know there has been controversy here. Julien - you seemed to agree,
> and iirc you partly drove how the patch is looking now. May I ask for
> an ack? Andrew - you disagreed for reasons that neither Julien nor I
> could really understand. Would you firmly nack the change and suggest
> a way out, or would you allow this to go in with someone else's ack?

I was mostly waiting on the discussion with Andrew to settle before 
reviewing. I can have a look now.

Cheers,

-- 
Julien Grall

Re: [PATCH v3] gnttab: defer allocation of status frame tracking array

[Julien Grall]

Hi Jan,

On 15/04/2021 10:41, Jan Beulich wrote:
> This array can be large when many grant frames are permitted; avoid
> allocating it when it's not going to be used anyway, by doing this only
> in gnttab_populate_status_frames().

Given the controversy of the change, I would suggest to summarize why 
this approach is considered to be ok in the commit message.

> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v3: Drop smp_wmb(). Re-base.
> v2: Defer allocation to when a domain actually switches to the v2 grant
>      API.
> 
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1747,6 +1747,17 @@ gnttab_populate_status_frames(struct dom
>       /* Make sure, prior version checks are architectural visible */
>       block_speculation();
>   
> +    if ( gt->status == ZERO_BLOCK_PTR )
> +    {
> +        gt->status = xzalloc_array(grant_status_t *,
> +                                   grant_to_status_frames(gt->max_grant_frames));
> +        if ( !gt->status )
> +        {
> +            gt->status = ZERO_BLOCK_PTR;
> +            return -ENOMEM;
> +        }
> +    }
> +
>       for ( i = nr_status_frames(gt); i < req_status_frames; i++ )
>       {
>           if ( (gt->status[i] = alloc_xenheap_page()) == NULL )
> @@ -1767,18 +1778,23 @@ status_alloc_failed:
>           free_xenheap_page(gt->status[i]);
>           gt->status[i] = NULL;
>       }

NIT: can you add a newline here and...

> +    if ( !nr_status_frames(gt) )
> +    {
> +        xfree(gt->status);
> +        gt->status = ZERO_BLOCK_PTR;
> +    }

... here for readability.

>       return -ENOMEM;
>   }
>   
>   static int
>   gnttab_unpopulate_status_frames(struct domain *d, struct grant_table *gt)
>   {
> -    unsigned int i;
> +    unsigned int i, n = nr_status_frames(gt);
>   
>       /* Make sure, prior version checks are architectural visible */
>       block_speculation();
>   
> -    for ( i = 0; i < nr_status_frames(gt); i++ )
> +    for ( i = 0; i < n; i++ )
>       {
>           struct page_info *pg = virt_to_page(gt->status[i]);
>           gfn_t gfn = gnttab_get_frame_gfn(gt, true, i);
> @@ -1833,12 +1849,11 @@ gnttab_unpopulate_status_frames(struct d
>           page_set_owner(pg, NULL);
>       }
>   
> -    for ( i = 0; i < nr_status_frames(gt); i++ )
> -    {
> -        free_xenheap_page(gt->status[i]);
> -        gt->status[i] = NULL;
> -    }
>       gt->nr_status_frames = 0;
> +    for ( i = 0; i < n; i++ )
> +        free_xenheap_page(gt->status[i]);
> +    xfree(gt->status);
> +    gt->status = ZERO_BLOCK_PTR;
The new position of the for loop seems unrelated to the purpose of the 
patch. May I ask why this was done?

>   
>       return 0;
>   }
> @@ -1969,11 +1984,11 @@ int grant_table_init(struct domain *d, i
>       if ( gt->shared_raw == NULL )
>           goto out;
>   
> -    /* Status pages for grant table - for version 2 */
> -    gt->status = xzalloc_array(grant_status_t *,
> -                               grant_to_status_frames(gt->max_grant_frames));
> -    if ( gt->status == NULL )
> -        goto out;
> +    /*
> +     * Status page tracking array for v2 gets allocated on demand. But don't
> +     * leave a NULL pointer there.
> +     */
> +    gt->status = ZERO_BLOCK_PTR;
>   
>       grant_write_lock(gt);
>   
> @@ -4047,11 +4062,12 @@ int gnttab_acquire_resource(
>           if ( gt->gt_version != 2 )
>               break;
>   
> +        rc = gnttab_get_status_frame_mfn(d, final_frame, &tmp);

NIT: It wasn't obvious to me why gnttab_get_status_frame_mfn() is moved 
before gt->status. May I suggest to add a in-code comment abouve the 
ordering?

> +
>           /* Check that void ** is a suitable representation for gt->status. */
>           BUILD_BUG_ON(!__builtin_types_compatible_p(
>                            typeof(gt->status), grant_status_t **));
>           vaddrs = (void **)gt->status;
> -        rc = gnttab_get_status_frame_mfn(d, final_frame, &tmp);
>           break;
>       }
>   
> 

Cheers,

-- 
Julien Grall

Re: [PATCH v3] gnttab: defer allocation of status frame tracking array

[Jan Beulich]

On 29.04.2021 15:15, Julien Grall wrote:
> On 15/04/2021 10:41, Jan Beulich wrote:
>> This array can be large when many grant frames are permitted; avoid
>> allocating it when it's not going to be used anyway, by doing this only
>> in gnttab_populate_status_frames().
> 
> Given the controversy of the change, I would suggest to summarize why 
> this approach is considered to be ok in the commit message.

I've added "While the delaying of the respective memory allocation adds
possible reasons for failure of the respective enclosing operations,
there are other memory allocations there already, so callers can't
expect these operations to always succeed anyway."

>> @@ -1767,18 +1778,23 @@ status_alloc_failed:
>>           free_xenheap_page(gt->status[i]);
>>           gt->status[i] = NULL;
>>       }
> 
> NIT: can you add a newline here and...
> 
>> +    if ( !nr_status_frames(gt) )
>> +    {
>> +        xfree(gt->status);
>> +        gt->status = ZERO_BLOCK_PTR;
>> +    }
> 
> ... here for readability.

Can do.

>> @@ -1833,12 +1849,11 @@ gnttab_unpopulate_status_frames(struct d
>>           page_set_owner(pg, NULL);
>>       }
>>   
>> -    for ( i = 0; i < nr_status_frames(gt); i++ )
>> -    {
>> -        free_xenheap_page(gt->status[i]);
>> -        gt->status[i] = NULL;
>> -    }
>>       gt->nr_status_frames = 0;
>> +    for ( i = 0; i < n; i++ )
>> +        free_xenheap_page(gt->status[i]);
>> +    xfree(gt->status);
>> +    gt->status = ZERO_BLOCK_PTR;
> The new position of the for loop seems unrelated to the purpose of the 
> patch. May I ask why this was done?

Since I was touching this anyway, I thought I could also bring it
into "canonical" order: Up-ing of an array's size should always
first populate the higher entries, then bump the upper bound.
Shrinking of an array's size should always first shrink the upper
bound, then un-populate the higher entries. This may not strictly
be needed here, but I think code we have would better not set bad
precedents (which may otherwise propagate elsewhere).

>> @@ -4047,11 +4062,12 @@ int gnttab_acquire_resource(
>>           if ( gt->gt_version != 2 )
>>               break;
>>   
>> +        rc = gnttab_get_status_frame_mfn(d, final_frame, &tmp);
> 
> NIT: It wasn't obvious to me why gnttab_get_status_frame_mfn() is moved 
> before gt->status. May I suggest to add a in-code comment abouve the 
> ordering?

I've added

        /* This may change gt->status, so has to happen before setting vaddrs. */ 

Jan

Re: [PATCH v3] gnttab: defer allocation of status frame tracking array

[Julien Grall]

Hi Jan,

On 29/04/2021 14:40, Jan Beulich wrote:
> On 29.04.2021 15:15, Julien Grall wrote:
>> On 15/04/2021 10:41, Jan Beulich wrote:
>>> This array can be large when many grant frames are permitted; avoid
>>> allocating it when it's not going to be used anyway, by doing this only
>>> in gnttab_populate_status_frames().
>>
>> Given the controversy of the change, I would suggest to summarize why
>> this approach is considered to be ok in the commit message.
> 
> I've added "While the delaying of the respective memory allocation adds
> possible reasons for failure of the respective enclosing operations,
> there are other memory allocations there already, so callers can't
> expect these operations to always succeed anyway."

Looks good to me, thanks!

> 
>>> @@ -1767,18 +1778,23 @@ status_alloc_failed:
>>>            free_xenheap_page(gt->status[i]);
>>>            gt->status[i] = NULL;
>>>        }
>>
>> NIT: can you add a newline here and...
>>
>>> +    if ( !nr_status_frames(gt) )
>>> +    {
>>> +        xfree(gt->status);
>>> +        gt->status = ZERO_BLOCK_PTR;
>>> +    }
>>
>> ... here for readability.
> 
> Can do.
> 
>>> @@ -1833,12 +1849,11 @@ gnttab_unpopulate_status_frames(struct d
>>>            page_set_owner(pg, NULL);
>>>        }
>>>    
>>> -    for ( i = 0; i < nr_status_frames(gt); i++ )
>>> -    {
>>> -        free_xenheap_page(gt->status[i]);
>>> -        gt->status[i] = NULL;
>>> -    }
>>>        gt->nr_status_frames = 0;
>>> +    for ( i = 0; i < n; i++ )
>>> +        free_xenheap_page(gt->status[i]);
>>> +    xfree(gt->status);
>>> +    gt->status = ZERO_BLOCK_PTR;
>> The new position of the for loop seems unrelated to the purpose of the
>> patch. May I ask why this was done?
> 
> Since I was touching this anyway, I thought I could also bring it
> into "canonical" order: Up-ing of an array's size should always
> first populate the higher entries, then bump the upper bound.
> Shrinking of an array's size should always first shrink the upper
> bound, then un-populate the higher entries. This may not strictly
> be needed here, but I think code we have would better not set bad
> precedents (which may otherwise propagate elsewhere).

I am assuming the concern here would be concurrent access. In which 
case, neither of the two versions would be actually be safe.

Anyway, I can see the theory so I am OK with it. However, this is more a 
clean-up than something strictly necessary for this patch. I can live 
with the code beeing modified here, but this at least ought to be 
explained in the commit message.

>>> @@ -4047,11 +4062,12 @@ int gnttab_acquire_resource(
>>>            if ( gt->gt_version != 2 )
>>>                break;
>>>    
>>> +        rc = gnttab_get_status_frame_mfn(d, final_frame, &tmp);
>>
>> NIT: It wasn't obvious to me why gnttab_get_status_frame_mfn() is moved
>> before gt->status. May I suggest to add a in-code comment abouve the
>> ordering?
> 
> I've added
> 
>          /* This may change gt->status, so has to happen before setting vaddrs. */

Sounds good to me!

Cheers,

-- 
Julien Grall