Re: [Xen-devel] [PATCH v5 6/8] xen: add new domctl hypercall to set grant table resource limits

[Juergen Gross <jgross@xxxxxxxx>]

On 08/09/17 17:55, Jan Beulich wrote:
>>>> On 08.09.17 at 08:56, <jgross@xxxxxxxx> wrote:
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -3667,6 +3667,32 @@ void grant_table_init_vcpu(struct vcpu *v)
>>      v->maptrack_tail = MAPTRACK_TAIL;
>>  }
>>  
>> +int grant_table_set_limits(struct domain *d, unsigned int grant_frames,
>> +                           unsigned int maptrack_frames)
>> +{
>> +    struct grant_table *gt = d->grant_table;
>> +    int ret = -EBUSY;
>> +
>> +    if ( !gt )
>> +        return -EEXIST;
> 
> How does EEXIST represent the error condition?

Yes, this was a bad choice. What about ENOENT?

> 
>> +    grant_write_lock(gt);
>> +
>> +    if ( gt->nr_grant_frames )
>> +        goto unlock;
> 
> I think you can do without goto here with no risk of lowered
> readability.

Okay.

> 
>> +    ret = 0;
>> +    if ( grant_frames )
>> +        gt->max_grant_frames = grant_frames;
>> +    if ( maptrack_frames )
>> +        gt->max_maptrack_frames = maptrack_frames;
> 
> Together with what I have said regarding making the invocation
> of this domctl mandatory, I think these two shouldn't be conditional.
> In particular for maptrack I also don't see why a domain couldn't
> do with a limit of zero, as long as it's not serving as a backend for
> another guest.

Okay, then I'd need to specify a "take hypervisor default" value (e.g.
~0) in order to handle the case where no value was specified in the
domain's config file.

The question would be then: do we want to set maptrack to 0 as default
and require it to be specified for backend domains (driver domains,
stubdoms)?

>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -1163,6 +1163,13 @@ struct xen_domctl_psr_cat_op {
>>  typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t;
>>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t);
>>  
>> +struct xen_domctl_set_gnttab_limits {
>> +    uint32_t grant_frames;     /* IN: if 0, dont change */
>> +    uint32_t maptrack_frames;  /* IN: if 0, dont change */
>> +};
>> +typedef struct xen_domctl_set_gnttab_limits xen_domctl_set_gnttab_limits_t;
>> +DEFINE_XEN_GUEST_HANDLE(xen_domctl_set_gnttab_limits_t);
> 
> In another context I had already recently requested to stop the
> bad habit of adding typedef and handle for all domctl-s. I don't
> see what they're needed for, they just clutter the name space.

I'm happy to remove it from the patch.


Juergen

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