Re: [Xen-devel] [PATCH v5 3/8] xen: delay allocation of grant table sub structures

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.09.17 at 11:03, <jgross@xxxxxxxx> wrote:
> On 08/09/17 17:28, Jan Beulich wrote:
>>>>> On 08.09.17 at 08:56, <jgross@xxxxxxxx> wrote:
>> And if you special case Dom0,
>> wouldn't it be better to (also) special case the hardware domain
>> (in case that's not Dom0)?
> 
> As a hardware domain not being dom0 would need to be created via the
> appropriate domctl hypercalls I don't see why the measures for all
> other domains wouldn't be enough for that case.

Yes, that's true especially when making the domctl mandatory to be
used. Whether suitable default values for that case wouldn't better
live in a single place (the hypervisor) is a point to be considered
here, though (by default values I mean ones to be used when the
config file doesn't specify any, not ones to be used by the domctl
handler if the passed in values are zero or some other "use
defaults" indicator, as you had elsewhere).

>>> @@ -1029,8 +1033,17 @@ int domain_unpause_by_systemcontroller(struct domain 
>>> *d)
>>>       * Creation is considered finished when the controller reference count
>>>       * first drops to 0.
>>>       */
>>> -    if ( new == 0 )
>>> +    if ( new == 0 && !d->creation_finished )
>>> +    {
>>> +        int ret = grant_table_init(d);
>>> +
>>> +        if ( ret )
>>> +        {
>>> +            __domain_pause_by_systemcontroller(d, NULL);
>>> +            return ret;
>>> +        }
>>>          d->creation_finished = true;
>>> +    }
>> 
>> Adding a grant table call here looks rather arbitrary, if not hackish.
> 
> Would it still be hackish if I'd add a generic function doing last
> init calls just before the domain starts to run for the first time?
> The call to grant_table_init() would be just the first such late init
> calls for the domain.

Generalizing this would make things look better, yes, but that
would then still not deal with the bad error reporting which
results.

>> Why can't you call it from the domctl you're going to add in a later
>> patch, requiring the tool stack to issue that domctl in all cases, just
>> like e.g. a max_vcpus one is always necessary? That would also
>> avoid a possibly confusing error (from the unpause, i.e. not
>> obviously related to grant table setup failure). Of course that will
>> require merging this patch with the other one to avoid an
>> intermediate state in which the call wouldn't be made at all.
> 
> This would be another possibility, yes.
> 
> Instead of merging the patches I'd just move patches 6-8 before this one
> to have everything in place, including the needed tools side.

Right, later I had realized too that simple re-ordering would be
sufficient.

>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -1655,6 +1655,78 @@ gnttab_unpopulate_status_frames(struct domain *d, 
>>> struct grant_table *gt)
>>>      gt->nr_status_frames = 0;
>>>  }
>>>  
>>> +int
>>> +grant_table_init(struct domain *d)
>>> +{
>>> +    struct grant_table *gt = d->grant_table;
>>> +    unsigned int i, j;
>>> +
>>> +    if ( gt->nr_grant_frames )
>>> +        return 0;
>>> +
>>> +    gt->nr_grant_frames = INITIAL_NR_GRANT_FRAMES;
>>> +
>>> +    /* Active grant table. */
>>> +    if ( (gt->active = xzalloc_array(struct active_grant_entry *,
>>> +                                     max_nr_active_grant_frames)) == NULL )
>>> +        goto no_mem_1;
>>> +    for ( i = 0;
>>> +          i < num_act_frames_from_sha_frames(INITIAL_NR_GRANT_FRAMES); i++ 
>>> )
>>> +    {
>>> +        if ( (gt->active[i] = alloc_xenheap_page()) == NULL )
>>> +            goto no_mem_2;
>>> +        clear_page(gt->active[i]);
>>> +        for ( j = 0; j < ACGNT_PER_PAGE; j++ )
>>> +            spin_lock_init(&gt->active[i][j].lock);
>>> +    }
>>> +
>>> +    /* Tracking of mapped foreign frames table */
>>> +    gt->maptrack = vzalloc(max_maptrack_frames * sizeof(*gt->maptrack));
>>> +    if ( gt->maptrack == NULL )
>>> +        goto no_mem_2;
>>> +
>>> +    /* Shared grant table. */
>>> +    if ( (gt->shared_raw = xzalloc_array(void *, max_grant_frames)) == 
>>> NULL )
>>> +        goto no_mem_3;
>>> +    for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
>>> +    {
>>> +        if ( (gt->shared_raw[i] = alloc_xenheap_page()) == NULL )
>>> +            goto no_mem_4;
>>> +        clear_page(gt->shared_raw[i]);
>>> +    }
>>> +
>>> +    /* Status pages for grant table - for version 2 */
>>> +    gt->status = xzalloc_array(grant_status_t *,
>>> +                               grant_to_status_frames(max_grant_frames));
>>> +    if ( gt->status == NULL )
>>> +        goto no_mem_4;
>>> +
>>> +    for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
>>> +        gnttab_create_shared_page(d, gt, i);
>>> +
>>> +    gt->nr_status_frames = 0;
>>> +
>>> +    return 0;
>>> +
>>> + no_mem_4:
>>> +    for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
>>> +        free_xenheap_page(gt->shared_raw[i]);
>>> +    xfree(gt->shared_raw);
>>> +    gt->shared_raw = NULL;
>>> + no_mem_3:
>>> +    vfree(gt->maptrack);
>>> +    gt->maptrack = NULL;
>>> + no_mem_2:
>>> +    for ( i = 0;
>>> +          i < num_act_frames_from_sha_frames(INITIAL_NR_GRANT_FRAMES); i++ 
>>> )
>>> +        free_xenheap_page(gt->active[i]);
>>> +    xfree(gt->active);
>>> +    gt->active = NULL;
>>> + no_mem_1:
>>> +    gt->nr_grant_frames = 0;
>>> +    return -ENOMEM;
>>> +}
>> 
>> The redundancy between this code and gnttab_grow_table() has
>> always bothered me, and now would seem to be a good occasion
>> to do away with it. Why don't you defer to gnttab_grow_table()
>> anything that function already does (keeping the respective limits
>> at zero in here)?
> 
> Just to be sure I understand you correctly: you want to get rid of
> INITIAL_NR_GRANT_FRAMES and just grow from 0 on instead of starting at
> the current value 4, right?

Yes, the use of INITIAL_NR_GRANT_FRAMES would move to that
first "grow" invocation.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel