On 11/09/17 11:23, Jan Beulich wrote: >>>> On 11.09.17 at 11:03, <jgr...@suse.com> wrote: >> On 08/09/17 17:28, Jan Beulich wrote: >>>>>> On 08.09.17 at 08:56, <jgr...@suse.com> 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).
But this is exactly what happens: the hypervisor defaults are being used in case nothing is specified in the domain's config file: a value of 0 for a value (grant table frame limit or maptrack frame limit) specified in the domctl will just use the default values. Or did I misunderstand you here? > >>>> @@ -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. In case nobody objects I'll go with making the new domctl mandatory then. > >>> 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(>->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. Hmm, shouldn't we just grow one frame after the other? Is it really true that most domains will need more than 1500 grants? A simple test domain with one disk and one NIC seems to be okay with a little bit more than 300 grants, so 1 grant table frame would be enough for that case. Juergen _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel