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

[Julien Grall <julien@xen.org>]

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