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

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

Apr 15, 2021, 2:41 AM

Post #1 of 4 (13 views)

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

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

julien at xen

Apr 29, 2021, 6:15 AM

Post #2 of 4 (13 views)

Permalink

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 [ In reply to ]

jbeulich at suse

Apr 29, 2021, 6:40 AM

Post #3 of 4 (13 views)

Permalink

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 [ In reply to ]

julien at xen

Apr 30, 2021, 1:09 AM

Post #4 of 4 (13 views)

Permalink

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