Re: [Xen-devel] [PATCH 2/3] xen/x86: add dom0 memory sizing variants

[Juergen Gross <jgross@xxxxxxxx>]

On 28/11/2018 16:17, Jan Beulich wrote:
>>>> On 22.11.18 at 17:40, <jgross@xxxxxxxx> wrote:
>> @@ -777,13 +777,20 @@ The amount of memory will be at least the minimum but 
>> never more than
>>  the maximum (i.e., `max` overrides the `min` option).  If there isn't
>>  enough memory then as much as possible is allocated.
>>  
>> -`max:<size>` also sets the maximum reservation (the maximum amount of
>> +`max:<sz>` also sets the maximum reservation (the maximum amount of
>>  memory dom0 can balloon up to).  If this is omitted then the maximum
>>  reservation is unlimited.
>>  
>>  For example, to set dom0's initial memory allocation to 512MB but
>>  allow it to balloon up as far as 1GB use `dom0_mem=512M,max:1G`
>>  
>> +> `<sz>` is: `<size> | [<size>+]<frac>%`
>> +> `<frac>` is an integer < 100
> 
> It is probably fine to state it like this here, but the implementation
> should take care of not allowing <size>+<frac>% to be greater
> than available memory.

dom0_compute_nr_pages() is making sure the size is limited by the
available memory.

> 
>> @@ -50,7 +55,28 @@ static unsigned long __init parse_amt(const char *s, 
>> const char **ps,
>>                                        unsigned long avail)
>>  {
>>      unsigned int minus = (*s == '-') ? 1 : 0;
>> -    unsigned long pages = parse_size_and_unit(s + minus, ps) >> PAGE_SHIFT;
>> +    unsigned long val, pages = 0;
>> +
>> +    /* Avoid accessing s[-1] in case value starts with '%'. */
>> +    if ( *s == '%' )
>> +        return 0;
>> +
>> +    s += minus;
>> +    while ( isdigit(*s) )
>> +    {
>> +        val = parse_size_and_unit(s, ps);
>> +        s = *ps;
>> +        if ( *s == '%' && isdigit(*(s - 1)) && val < (100 << 10) )
> 
> In the comment above you talk about s[-1] - why do you use a more
> complicated (even if just slightly) expression here?

I dislike using s[-1] in code. In case you prefer it I can use it here.

> 
>> +        {
>> +            pages += (val >> 10) * avail / 100;
> 
> The scaling by 10 bits here and above make it rather desirable to
> slightly change parse_size_and_unit() instead: I think it could
> easily be made not do the scaling if it finds % as the next input
> character. Otherwise at least a brief comment should be put here.

I'm fine with either solution. In case nobody else speaks up I'll
modify parse_size_and_unit() to do no scaling in case of '%' following
the value.

> Despite the comments - I very much like the added flexibility.

Thanks. That was the main hurdle. Implementation details can be sorted
out easily. :-)


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel