Re: [PATCH v3 3/3] cxl: Add memory hotplug notifier for cxl region

[Dave Jiang <dave.jiang@intel.com>]

On 1/8/24 19:15, Huang, Ying wrote:
> Dave Jiang <dave.jiang@intel.com> writes:
> 
>> On 1/8/24 05:15, Jonathan Cameron wrote:
>>> On Mon, 08 Jan 2024 14:49:03 +0800
>>> "Huang, Ying" <ying.huang@intel.com> wrote:
>>>
>>>> Dave Jiang <dave.jiang@intel.com> writes:

<snip>

>>>>> +
>>>>> +	node_set_perf_attrs(nid, &cxlr->coord, 0);
>>>>> +	node_set_perf_attrs(nid, &cxlr->coord, 1);  
>>>>
>>>> And this.
>>>>
>>>> But I don't think it's good to remove access level 1.  According to
>>>> commit b9fffe47212c ("node: Add access1 class to represent CPU to memory
>>>> characteristics").  Access level 1 is for performance from CPU to
>>>> memory.  So, we should keep access level 1.  For CXL memory device,
>>>> access level 0 and access level 1 should be equivalent.  Will the code
>>>> be used for something like GPU connected via CXL?  Where the access
>>>> level 0 may be for the performance from GPU to the memory.
>>>>
>>> I disagree. They are no more equivalent than they are on any other complex system.
>>>
>>> e.g. A CXL root port being described using generic Port infrastructure may be
>>> on a different die (IO dies are a common architecture) in the package
>>> than the CPU cores and that IO die may well have generic initiators that
>>> are much nearer than the CPU cores.
>>>
>>> In those cases access0 will cover initators on the IO die but access1 will
>>> cover the nearest CPU cores (initiators).
>>>
>>> Both should arguably be there for CXL memory as both are as relevant as
>>> they are for any other memory.
>>>
>>> If / when we get some GPUs etc on CXL that are initiators this will all
>>> get a lot more fun but for now we can kick that into the long grass.
>>
>>
>> With the current way of storing HMAT targets information, only the
>> best performance data is stored (access0). The existing HMAT handling
>> code also sets the access1 if the associated initiator node contains a
>> CPU for conventional memory. The current calculated full CXL path is
>> the access0 data. I think what's missing is the check to see if the
>> associated initiator node is also a CPU node and sets access1
>> conditionally based on that. Maybe if that conditional gets added then
>> that is ok for what we have now?
> 
> We do need access1 to put NUMA nodes into appropriate memory tiers, just
> like we have done in hmat.c.  Because default memory tiers are defined
> with performance from CPU to the device.  Is it possible to get access1
> always?

Let me take a look and see how to get this done. I think we'll need to define 2 access classes for the generic target numbers (instead of currently only 1) so we can store the access0 for generic port and access1 for generic port.