Re: [Xen-devel] [PATCH v5 4/8] xen: make grant resource limits per domain

[Juergen Gross <jgross@xxxxxxxx>]

On 08/09/17 17:44, Jan Beulich wrote:
>>>> On 08.09.17 at 08:56, <jgross@xxxxxxxx> wrote:
>> @@ -1843,6 +1838,14 @@ gnttab_setup_table(
>>      gt = d->grant_table;
>>      grant_write_lock(gt);
>>  
>> +    if ( unlikely(op.nr_frames > gt->max_grant_frames) )
>> +    {
>> +        gdprintk(XENLOG_INFO, "Domain is limited to %d grant-table 
>> frames.\n",
>> +                gt->max_grant_frames);
> 
> %u please

Okay.

> 
>> @@ -3465,6 +3471,8 @@ grant_table_create(
>>      /* Simple stuff. */
>>      percpu_rwlock_resource_init(&t->lock, grant_rwlock);
>>      spin_lock_init(&t->maptrack_lock);
>> +    t->max_grant_frames = max_grant_frames;
>> +    t->max_maptrack_frames = max_maptrack_frames;
> 
> Am I mistaken or are these the only uses of the two static variables
> now? If so (also to prove that's the case) their definitions would
> probably better be moved into this function, together with their
> integer_param() invocations. The adjustments done by
> gnttab_usage_init() could also go here afaict.

Okay.

> 
>> @@ -3755,6 +3763,12 @@ static void gnttab_usage_print(struct domain *rd)
>>  
>>      grant_read_lock(gt);
>>  
>> +    printk("grant-table for remote domain:%5d (v%d)\n"
>> +           "  %d frames (%d max), %d maptrack frames (%d max)\n",
>> +           rd->domain_id, gt->gt_version,
>> +           nr_grant_frames(gt), gt->max_grant_frames,
>> +           nr_maptrack_frames(gt), gt->max_maptrack_frames);
> 
> Various %u instances again, and Dom%d please. Also you put this
> after the table header, corrupting intended output.

The position where the domain header is printed didn't change. It is
just unconditional now and contains some more information.

> 
>> @@ -3782,12 +3796,7 @@ static void gnttab_usage_print(struct domain *rd)
>>              status = status_entry(gt, ref);
>>          }
>>  
>> -        if ( first )
>> -        {
>> -            printk("grant-table for remote domain:%5d (v%d)\n",
>> -                   rd->domain_id, gt->gt_version);
>> -            first = 0;
>> -        }
>> +        first = 0;
> 
> Is it useful to print the per-table information when there are no
> entries at all for a domain? I think it would be better to move
> what you add as well as the table header into the if() that you
> delete.

Hmm, I think the per-domain limits are valuable even without any
grant entries being present. In the end I don't expect lots of domains
without any grant entry other than dom0, as the default entries for
xenstore and console are being created by the tools already. And having
dom0 information especially for the maptrack entries will be
interesting.


Juergen

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