Re: [Xen-devel] [PATCH v9 09/10] xen: make grant resource limits per domain

[Juergen Gross <jgross@xxxxxxxx>]

On 22/09/17 17:15, Jan Beulich wrote:
>>>> On 22.09.17 at 13:41, <jgross@xxxxxxxx> wrote:
>> Instead of using the same global resource limits of grant tables (max.
>> number of grant frames, max. number of maptrack frames) for all domains
>> make these limits per domain. Set those per-domain limits in
>> grant_table_set_limits(). The global settings are serving as an upper
>> boundary now which must not be exceeded by a per-domain value. The
>> default of max_grant_frames is set to the maximum default xl will use.
>>
>> While updating the semantics of the boot parameters remove the
>> documentation of the no longer existing gnttab_max_nr_frames.
> 
> "... and correct the default gnttab_max_maptrack_frames uses" (or
> some such).

Okay.

> 
>> @@ -1672,8 +1670,8 @@ gnttab_grow_table(struct domain *d, unsigned int 
>> req_nr_frames)
>>      ASSERT(gt->active);
>>  
>>      if ( req_nr_frames < INITIAL_NR_GRANT_FRAMES )
>> -        req_nr_frames = INITIAL_NR_GRANT_FRAMES;
>> -    ASSERT(req_nr_frames <= max_grant_frames);
>> +        req_nr_frames = min(INITIAL_NR_GRANT_FRAMES, gt->max_grant_frames);
> 
> I'm not convinced of this: You effectively allowing a zero size grant
> table this way. I'd prefer if the "initial" constant stayed the lower
> bound. I'm open to lowering that initial value, though.

Okay. What about the value "1" for it? Should be enough for e.g.
stubdoms, dom0, ...

> 
>> @@ -1824,6 +1818,21 @@ gnttab_setup_table(
>>      gt = d->grant_table;
>>      grant_write_lock(gt);
>>  
>> +    if ( unlikely(op.nr_frames > gt->max_grant_frames) )
>> +    {
>> +        gdprintk(XENLOG_INFO, "d%u is limited to %u grant-table frames.\n",
> 
> You've switched to %u one too many times - domain IDs want
> printing with %d (also below).

Okay.

> 
>> @@ -2970,14 +2983,14 @@ 
>> gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
>>  
>>  static long
>>  gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_status_frames_t) 
>> uop,
>> -                         int count)
>> +                         unsigned int count, unsigned int limit_max)
>> {
>>       gnttab_get_status_frames_t op;
>>      struct domain *d;
>>      struct grant_table *gt;
>>      uint64_t       gmfn;
>>      int i;
>> -    int rc;
>> +    int rc, ret = 0;
> 
> This variable doesn't look to be necessary anymore (also in
> gnttab_setup_table(), as I notice only now).

Indeed.

> 
>> @@ -3010,9 +3023,19 @@ 
>> gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_status_frames_t) 
>> uop,
>>  
>>      if ( unlikely(op.nr_frames > nr_status_frames(gt)) )
>>      {
>> -        gdprintk(XENLOG_INFO, "Guest requested addresses for %d grant 
>> status "
>> -                 "frames, but only %d are available.\n",
>> -                 op.nr_frames, nr_status_frames(gt));
>> +        gdprintk(XENLOG_INFO, "Guest requested addresses of d%u for %u 
>> grant "
>> +                 "status frames, but only %u are available.\n",
> 
> Drop "Guest" and make the end ", has only %u\n"?

Okay.

> 
>> @@ -3665,7 +3694,11 @@ int grant_table_set_limits(struct domain *d, unsigned 
>> int grant_frames,
>>  
>>      /* Set limits. */
>>      if ( !gt->active )
>> +    {
>> +        gt->max_grant_frames = grant_frames;
> 
> As per above I think you want to silently apply a lower bound here.

I already have. It is 1 (note the test for !grant_frames some lines
higher).

> 
>> @@ -3769,6 +3802,12 @@ static void gnttab_usage_print(struct domain *rd)
>>  
>>      grant_read_lock(gt);
>>  
>> +    printk("grant-table for remote domain:%5d (v%d)\n"
> 
> "grant table for d%d (v%u)\n"?

Okay.

> 
>> +           "  %d frames (%d max), %d maptrack frames (%d max)\n",
> 
> %u (four times)

Okay.

One final note: I have detected another problem  in the ARM part of
this patch: gnttab_size is set wrong. Will correct this.


Juergen

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