Re: [PATCH v4] Documentation, dt, numa: Add note to empty NUMA node

[Gavin Shan <gshan@redhat.com>]

On 6/28/21 6:34 PM, Andrew Jones wrote:
> On Fri, Jun 25, 2021 at 09:14:49PM +1000, Gavin Shan wrote:
>> On 6/25/21 8:18 PM, Andrew Jones wrote:
>>> On Fri, Jun 25, 2021 at 06:36:48PM +1000, Gavin Shan wrote:
>>>> On 6/25/21 5:06 PM, Andrew Jones wrote:
>>>>> On Fri, Jun 25, 2021 at 09:02:17AM +0200, Andrew Jones wrote:
>>>>>> On Fri, Jun 25, 2021 at 01:23:38PM +0800, Gavin Shan wrote:
>>>>>>> The empty memory nodes, where no memory resides in, are allowed.
>>>>>>> For these empty memory nodes, the 'len' of 'reg' property is zero.
>>>>>>> The NUMA node IDs are still valid and parsed, but memory can be
>>>>>>> added to them through hotplug afterwards. I finds difficulty to
>>>>>>> get where it's properly documented.
>>>>>>>
>>>>>>> So lets add note to empty memory nodes in the NUMA binding doc.
>>>>>>>
>>>>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>>>>> ---
>>>>>>>     Documentation/devicetree/bindings/numa.txt | 4 ++++
>>>>>>>     1 file changed, 4 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/numa.txt b/Documentation/devicetree/bindings/numa.txt
>>>>>>> index 21b35053ca5a..0fc882e44270 100644
>>>>>>> --- a/Documentation/devicetree/bindings/numa.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/numa.txt
>>>>>>> @@ -109,6 +109,10 @@ Example:
>>>>>>>     Dual socket system consists of 2 boards connected through ccn bus and
>>>>>>>     each board having one socket/soc of 8 cpus, memory and pci bus.
>>>>>>> +Note that empty memory nodes, which no memory resides in, are allowed.
>>>>>>> +The NUMA node IDs in these empty memory nodes are still valid, but
>>>>>>> +memory can be added into them through hotplug afterwards.
>>>>>>
>>>>>> Please change the second sentence to:
>>>>>>
>>>>>>      The NUMA node IDs in these empty memory nodes are still valid and
>>>>>>      memory may be added into them through hotplug afterwards.
>>>>>>
>>>>>> But, this doesn't look like the right place for this paragraph. You're
>>>>>> adding the paragraph to the example section, but the example doesn't have
>>>>>> any empty memory nodes.
>>>>>>
>>>>>> I think the paragraph should be added to section "2 - numa-node-id" and an
>>>>>
>>>>> Or maybe even create a new section for it.
>>>>>
>>>>>> example empty memory node should be provided. Also, the commit message
>>>>>> talks about the length of 'reg' being zero, which is an important
>>>>>> distinction which should also be documented.
>>>>>>
>>>>
>>>> Drew, thanks for your comments. Yeah, it sounds sensible to create
>>>> a new section for it and an example would be more helpful. Please
>>>> check if below changes are fine to you. I probably need Randy's review
>>>> again.
>>>>
>>>> I'm trying to avoid too many revisions for this sort of trivial patch,
>>>> even though I already had. However, it's time frame for v5.14 and I'm
>>>> pushing this to be merged during the cycle.
>>>
>>> We must revise until we're satisfied... Also, I wouldn't call writing
>>> specifications trivial.
>>>
>>
>> [...]
>>
>>> I'll just rewrite it:
>>>
>>>    Empty memory nodes, which no memory resides in, are allowed. The 'length'
>>>    field of the 'reg' property is zero, but the 'base-address' is a dummy
>>>    address and invalid. However, the NUMA node IDs and distance maps are
>>>    still valid and memory may be added into them through hotplug afterwards.
>>>
>>
>> [...]
>>
>>>
>>> You should write how you've selected the dummy addresses for the empty
>>> memory nodes, perhaps here in the /* Empty memory node */ comments or
>>> above in the paragraph or both.
>>>
>>>> +	memory@0x100000000 {
>>>> +		device_type = "memory";
>>>> +		reg = <0x1 0x0 0x0 0x0>;
>>>> +		numa-node-id = <2>;
>>>> +	};
>>>> +
>>>> +	/* Empty memory node */
>>>> +	memory@0x180000000 {
>>>> +		device_type = "memory";
>>>> +		reg = <0x1 0x80000000 0x0 0x0>;
>>>> +		numa-node-id = <3>;
>>>> +	};
>>>> +
>>
>> [...]
>>
>> Thanks, Drew. As 'base-address' is dummy address and invalid. So we
>> would note it as suggestion. With your suggestion integrated, how about
>> something like this?
>>
>> ==============================================================================
>> 4 - Empty memory nodes
>> ==============================================================================
>>
>> Empty memory nodes, which no memory resides in, are allowed. The 'length'
>> field of the 'reg' property is zero, but the 'base-address' is a dummy
>> address and invalid. The 'base-address' could be the summation of highest
>> memory address plus the NUMA node ID. However, the NUMA node IDs and
>> distance maps are still valid and memory may be added into them through
>> hotplug afterwards.
>>
>>         :
>>
>>         memory@0 {
>>                  device_type = "memory";
>>                  reg = <0x0 0x0 0x0 0x80000000>;
>>                  numa-node-id = <0>;
>>          };
>>
>>          memory@0x80000000 {
>>                  device_type = "memory";
>>                  reg = <0x0 0x80000000 0x0 0x80000000>;
>>                  numa-node-id = <1>;
>>          };
>>
>>          /* Empty memory node */
>>          memory@0x100000002 {
>>                  device_type = "memory";
>>                  reg = <0x1 0x2 0x0 0x0>;
>>                  numa-node-id = <2>;
>>          };
>>
>>          /* Empty memory node */
>>          memory@0x100000003 {
>>                  device_type = "memory";
>>                  reg = <0x1 0x3 0x0 0x0>;
>>                  numa-node-id = <3>;
>>          };
>>
>>          :
>>
>> Thanks,
>> Gavin
>>
>>
> 
> Looks good to me.
> 

Thanks, Drew. v5 was posted to reflect changes, but I don't
include Randy's r-b because too much changes introduced.
Hopefully, Randy can take a look on v5 when getting a chance.

Thanks,
Gavin