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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>

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;
>      }
>  
>