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

[Julien Grall <julien@xxxxxxx>]

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