Re: [Xen-devel] [PATCH 1/2] page-alloc/x86: don't restrict DMA heap to node 0

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.08.16 at 14:18, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 10/08/16 11:21, Jan Beulich wrote:
>>>>> On 10.08.16 at 11:58, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 10/08/16 10:23, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/numa.c
>>>> +++ b/xen/arch/x86/numa.c
>>>> @@ -355,11 +355,21 @@ void __init init_cpu_to_node(void)
>>>>      }
>>>>  }
>>>>  
>>>> -EXPORT_SYMBOL(cpu_to_node);
>>>> -EXPORT_SYMBOL(node_to_cpumask);
>>>> -EXPORT_SYMBOL(memnode_shift);
>>>> -EXPORT_SYMBOL(memnodemap);
>>>> -EXPORT_SYMBOL(node_data);
>>>> +unsigned int __init arch_get_dma_bitsize(void)
>>>> +{
>>>> +    unsigned int node;
>>>> +
>>>> +    for_each_online_node(node)
>>>> +        if ( node_spanned_pages(node) &&
>>>> +             !(node_start_pfn(node) >> (32 - PAGE_SHIFT)) )
>>>> +            break;
>>>> +    if ( node >= MAX_NUMNODES )
>>>> +        panic("No node with memory below 4Gb");
>>>> +
>>>> +    return min_t(unsigned int,
>>>> +                 flsl(node_start_pfn(node) + node_spanned_pages(node) / 4 
>>>> - 
> 1)
>>>> +                 + PAGE_SHIFT, 32);
>>> You have moved the -1 and -2 inside the flsl() call, which alters its
>>> behaviour quite a bit.  Is this intentional or accidental?
>> This is intentional, and their original placement was only not too
>> wrong because of the effective use of zero in place of what is
>> now node_start_pfn(node). (Obviously the division by 4 alone
>> could have gone in either place, but the "- 1" should have been
>> inside the flsl() even before imo.)
> 
> In which case it should be at least mentioned in the commit message.

"Also adjust the original calculation: I think the subtraction of 1
 should have been part of the flsl() argument rather than getting
 applied to its result. And while previously the division by 4 was valid
 to be done on the flsl() result, this now also needs to be converted,
 as is should only be applied to the spanned pages value."

> Finally, do you really mean to only divide the spanned pages by 4? 

Hmm, this question reads ambiguous to me: Do you mean the divisor
should be larger than 4? I don't think so, or else the area could end
up rather small. Or do you mean to divide the sum of start address
and spanned pages? That would surely be wrong, as the result could
end up below start address, and hence the area could continue to
remain empty.

> Either way, it could do with further bracketing.

Bracketing of what? Even elementary school maths give precedence
to multiplication and division over addition and subtraction. I know
you like to parenthesize basically every binary expression that's an
operand of another expression, but I think you meanwhile also know
that I think this goes too far.

Jan


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