| Message ID | d2397cd4-040e-3cc0-22d8-3f65d01f9326@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] gnttab: defer allocation of status frame tracking array | expand |
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; > } > >
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,
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,
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
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,
--- 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; }
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.