Re: [PATCH v2] mm/sparse: set section nid for hot-add memory

[David Hildenbrand <david@redhat.com>]

On 19.06.19 11:01, Michal Hocko wrote:
> On Wed 19-06-19 10:54:08, David Hildenbrand wrote:
>> On 19.06.19 08:10, Michal Hocko wrote:
>>> On Tue 18-06-19 10:40:06, David Hildenbrand wrote:
>>>> On 18.06.19 10:32, Wei Yang wrote:
>>>>> On Tue, Jun 18, 2019 at 09:49:48AM +0200, Oscar Salvador wrote:
>>>>>> On Tue, Jun 18, 2019 at 08:55:37AM +0800, Wei Yang wrote:
>>>>>>> In case of NODE_NOT_IN_PAGE_FLAGS is set, we store section's node id in
>>>>>>> section_to_node_table[]. While for hot-add memory, this is missed.
>>>>>>> Without this information, page_to_nid() may not give the right node id.
>>>>>>>
>>>>>>> BTW, current online_pages works because it leverages nid in memory_block.
>>>>>>> But the granularity of node id should be mem_section wide.
>>>>>>
>>>>>> I forgot to ask this before, but why do you mention online_pages here?
>>>>>> IMHO, it does not add any value to the changelog, and it does not have much
>>>>>> to do with the matter.
>>>>>>
>>>>>
>>>>> Since to me it is a little confused why we don't set the node info but still
>>>>> could online memory to the correct node. It turns out we leverage the
>>>>> information in memblock.
>>>>
>>>> I'd also drop the comment here.
>>>>
>>>>>
>>>>>> online_pages() works with memblock granularity and not section granularity.
>>>>>> That memblock is just a hot-added range of memory, worth of either 1 section or multiple
>>>>>> sections, depending on the arch or on the size of the current memory.
>>>>>> And we assume that each hot-added memory all belongs to the same node.
>>>>>>
>>>>>
>>>>> So I am not clear about the granularity of node id. section based or memblock
>>>>> based. Or we have two cases:
>>>>>
>>>>> * for initial memory, section wide
>>>>> * for hot-add memory, mem_block wide
>>>>
>>>> It's all a big mess. Right now, you can offline initial memory with
>>>> mixed nodes. Also on my list of many ugly things to clean up.
>>>>
>>>> (I even remember that we can have mixed nodes within a section, but I
>>>> haven't figured out yet how that is supposed to work in some scenarios)
>>>
>>> Yes, that is indeed the case. See 4aa9fc2a435abe95a1e8d7f8c7b3d6356514b37a.
>>> How to fix this? Well, I do not think we can. Section based granularity
>>> simply doesn't agree with the reality and so we have to live with that.
>>> There is a long way to remove all those section size assumptions from
>>> the code though.
>>>
>>
>> Trying to remove NODE_NOT_IN_PAGE_FLAGS could work, but we would have to
>> identify how exactly needs that. For memory blocks, we need a different
>> approach (I have in my head to make ->nid indicate if we are dealing
>> with mixed nodes. If mixed, disallow onlining/offlining).
> 
> Well, I am not sure we really have to care about mutli-nodes memblocks
> much. The API is clumsy but does anybody actually care? The vast
> majority of hotplug usecases simply do not do that in the first place
> right?

Yes, AFAIK it could be done, resulting in unpredictable outcome.

And if they do need a smaller granularity to describe their
> memory topology then we need a different user API rather the fiddle with
> implementation details I would argue.
> 

It is not about supporting it, it is about properly blocking it.

-- 

Thanks,

David / dhildenb