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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

Finally, do you really mean to only divide the spanned pages by 4? 
Either way, it could do with further bracketing.

~Andrew

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