diff mbox series

[v2,3/4] acpi: numa: Add setting of generic port system locality attributes

Message ID 168451347867.3465146.10428399827479313906.stgit@djiang5-mobl3
State New, archived
Headers show
Series acpi: numa: Add target support for generic port to HMAT parsing | expand

Commit Message

Dave Jiang May 19, 2023, 4:24 p.m. UTC
Add generic port support for the parsing of HMAT system locality sub-table.
The attributes will be added to the third array member of the access
coordinates in order to not mix with the existing memory attributes. It only
provides the system locality attributes from initator to the generic port
targets and is missing the rest of the data to the actual memory device.

The complete attributes will be updated when a memory device is
attached and the system locality information is calculated end to end.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>

v2:
- Fix commit log runon sentence. (Jonathan)
- Add a check for memory type for skipping other access levels. (Jonathan)
- NODE_ACCESS_CLASS_GENPORT to NODE_ACCESS_CLASS_GENPORT_SINK. (Jonathan)
---
 drivers/acpi/numa/hmat.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Jonathan Cameron June 1, 2023, 2:38 p.m. UTC | #1
On Fri, 19 May 2023 09:24:38 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Add generic port support for the parsing of HMAT system locality sub-table.
> The attributes will be added to the third array member of the access
> coordinates in order to not mix with the existing memory attributes. It only
> provides the system locality attributes from initator to the generic port
> targets and is missing the rest of the data to the actual memory device.
> 
> The complete attributes will be updated when a memory device is
> attached and the system locality information is calculated end to end.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Missing 
---
here

As a passing comment, hmat_parse_locality() is awfully deeply nested
- maybe worth looking to see if some of the deeply nested stuff can be
factored out...  That would be a new patch however


> 
> v2:
> - Fix commit log runon sentence. (Jonathan)
> - Add a check for memory type for skipping other access levels. (Jonathan)
> - NODE_ACCESS_CLASS_GENPORT to NODE_ACCESS_CLASS_GENPORT_SINK. (Jonathan)
> ---
>  drivers/acpi/numa/hmat.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> index e2ab1cce0add..82320c92abed 100644
> --- a/drivers/acpi/numa/hmat.c
> +++ b/drivers/acpi/numa/hmat.c
> @@ -60,6 +60,7 @@ struct target_cache {
>  enum {
>  	NODE_ACCESS_CLASS_0 = 0,
>  	NODE_ACCESS_CLASS_1,
> +	NODE_ACCESS_CLASS_GENPORT_SINK,
>  	NODE_ACCESS_CLASS_MAX,
>  };
>  
> @@ -368,6 +369,15 @@ static __init int hmat_parse_locality(union acpi_subtable_headers *header,
>  			if (mem_hier == ACPI_HMAT_MEMORY) {
>  				target = find_mem_target(targs[targ]);
>  				if (target && target->processor_pxm == inits[init]) {
> +					if (*target->device_handle) {
> +						hmat_update_target_access(target, type, value,
> +								NODE_ACCESS_CLASS_GENPORT_SINK);
> +						if ((hmat_loc->flags &
> +						     ACPI_HMAT_MEMORY_HIERARCHY) ==
> +						    ACPI_HMAT_MEMORY)
> +							continue;

I'm confused.  Isn't this already what was checked for with
if (mem_heir == ACPI_HMAT_MEMORY)?

> +					}
> +
>  					hmat_update_target_access(target, type, value,
>  								  NODE_ACCESS_CLASS_0);
>  					/* If the node has a CPU, update access 1 */
> 
>
Dave Jiang June 1, 2023, 4:04 p.m. UTC | #2
On 6/1/23 07:38, Jonathan Cameron wrote:
> On Fri, 19 May 2023 09:24:38 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
>
>> Add generic port support for the parsing of HMAT system locality sub-table.
>> The attributes will be added to the third array member of the access
>> coordinates in order to not mix with the existing memory attributes. It only
>> provides the system locality attributes from initator to the generic port
>> targets and is missing the rest of the data to the actual memory device.
>>
>> The complete attributes will be updated when a memory device is
>> attached and the system locality information is calculated end to end.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Missing
> ---
> here
>
> As a passing comment, hmat_parse_locality() is awfully deeply nested
> - maybe worth looking to see if some of the deeply nested stuff can be
> factored out...  That would be a new patch however


I'll take a look

>
>
>> v2:
>> - Fix commit log runon sentence. (Jonathan)
>> - Add a check for memory type for skipping other access levels. (Jonathan)
>> - NODE_ACCESS_CLASS_GENPORT to NODE_ACCESS_CLASS_GENPORT_SINK. (Jonathan)
>> ---
>>   drivers/acpi/numa/hmat.c |   10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
>> index e2ab1cce0add..82320c92abed 100644
>> --- a/drivers/acpi/numa/hmat.c
>> +++ b/drivers/acpi/numa/hmat.c
>> @@ -60,6 +60,7 @@ struct target_cache {
>>   enum {
>>   	NODE_ACCESS_CLASS_0 = 0,
>>   	NODE_ACCESS_CLASS_1,
>> +	NODE_ACCESS_CLASS_GENPORT_SINK,
>>   	NODE_ACCESS_CLASS_MAX,
>>   };
>>   
>> @@ -368,6 +369,15 @@ static __init int hmat_parse_locality(union acpi_subtable_headers *header,
>>   			if (mem_hier == ACPI_HMAT_MEMORY) {
>>   				target = find_mem_target(targs[targ]);
>>   				if (target && target->processor_pxm == inits[init]) {
>> +					if (*target->device_handle) {
>> +						hmat_update_target_access(target, type, value,
>> +								NODE_ACCESS_CLASS_GENPORT_SINK);
>> +						if ((hmat_loc->flags &
>> +						     ACPI_HMAT_MEMORY_HIERARCHY) ==
>> +						    ACPI_HMAT_MEMORY)
>> +							continue;
> I'm confused.  Isn't this already what was checked for with
> if (mem_heir == ACPI_HMAT_MEMORY)?

Yes. Do gen target show up as not memory? I couldn't tell from the ACPI 
spec. I wonder if I need to move the setup outside of the (mem_hier == 
ACPI_HMAT_MEMORY)?


>> +					}
>> +
>>   					hmat_update_target_access(target, type, value,
>>   								  NODE_ACCESS_CLASS_0);
>>   					/* If the node has a CPU, update access 1 */
>>
>>
Jonathan Cameron June 1, 2023, 4:48 p.m. UTC | #3
On Thu, 1 Jun 2023 09:04:34 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> On 6/1/23 07:38, Jonathan Cameron wrote:
> > On Fri, 19 May 2023 09:24:38 -0700
> > Dave Jiang <dave.jiang@intel.com> wrote:
> >  
> >> Add generic port support for the parsing of HMAT system locality sub-table.
> >> The attributes will be added to the third array member of the access
> >> coordinates in order to not mix with the existing memory attributes. It only
> >> provides the system locality attributes from initator to the generic port
> >> targets and is missing the rest of the data to the actual memory device.
> >>
> >> The complete attributes will be updated when a memory device is
> >> attached and the system locality information is calculated end to end.
> >>
> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com>  
> > Missing
> > ---
> > here
> >
> > As a passing comment, hmat_parse_locality() is awfully deeply nested
> > - maybe worth looking to see if some of the deeply nested stuff can be
> > factored out...  That would be a new patch however  
> 
> 
> I'll take a look
> 
> >
> >  
> >> v2:
> >> - Fix commit log runon sentence. (Jonathan)
> >> - Add a check for memory type for skipping other access levels. (Jonathan)
> >> - NODE_ACCESS_CLASS_GENPORT to NODE_ACCESS_CLASS_GENPORT_SINK. (Jonathan)
> >> ---
> >>   drivers/acpi/numa/hmat.c |   10 ++++++++++
> >>   1 file changed, 10 insertions(+)
> >>
> >> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> >> index e2ab1cce0add..82320c92abed 100644
> >> --- a/drivers/acpi/numa/hmat.c
> >> +++ b/drivers/acpi/numa/hmat.c
> >> @@ -60,6 +60,7 @@ struct target_cache {
> >>   enum {
> >>   	NODE_ACCESS_CLASS_0 = 0,
> >>   	NODE_ACCESS_CLASS_1,
> >> +	NODE_ACCESS_CLASS_GENPORT_SINK,
> >>   	NODE_ACCESS_CLASS_MAX,
> >>   };
> >>   
> >> @@ -368,6 +369,15 @@ static __init int hmat_parse_locality(union acpi_subtable_headers *header,
> >>   			if (mem_hier == ACPI_HMAT_MEMORY) {
> >>   				target = find_mem_target(targs[targ]);
> >>   				if (target && target->processor_pxm == inits[init]) {
> >> +					if (*target->device_handle) {
> >> +						hmat_update_target_access(target, type, value,
> >> +								NODE_ACCESS_CLASS_GENPORT_SINK);
> >> +						if ((hmat_loc->flags &
> >> +						     ACPI_HMAT_MEMORY_HIERARCHY) ==
> >> +						    ACPI_HMAT_MEMORY)
> >> +							continue;  
> > I'm confused.  Isn't this already what was checked for with
> > if (mem_heir == ACPI_HMAT_MEMORY)?  
> 
> Yes. Do gen target show up as not memory? I couldn't tell from the ACPI 
> spec. I wonder if I need to move the setup outside of the (mem_hier == 
> ACPI_HMAT_MEMORY)?

I think it would be almost meaningless to target non memory.
Ultimately I guess someone might do cache stashing or similar
and care about targeting a CPU in a memoryless node?  Let's
ignore that however.



> 
> 
> >> +					}
> >> +
> >>   					hmat_update_target_access(target, type, value,
> >>   								  NODE_ACCESS_CLASS_0);
> >>   					/* If the node has a CPU, update access 1 */
> >>
> >>
diff mbox series

Patch

diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
index e2ab1cce0add..82320c92abed 100644
--- a/drivers/acpi/numa/hmat.c
+++ b/drivers/acpi/numa/hmat.c
@@ -60,6 +60,7 @@  struct target_cache {
 enum {
 	NODE_ACCESS_CLASS_0 = 0,
 	NODE_ACCESS_CLASS_1,
+	NODE_ACCESS_CLASS_GENPORT_SINK,
 	NODE_ACCESS_CLASS_MAX,
 };
 
@@ -368,6 +369,15 @@  static __init int hmat_parse_locality(union acpi_subtable_headers *header,
 			if (mem_hier == ACPI_HMAT_MEMORY) {
 				target = find_mem_target(targs[targ]);
 				if (target && target->processor_pxm == inits[init]) {
+					if (*target->device_handle) {
+						hmat_update_target_access(target, type, value,
+								NODE_ACCESS_CLASS_GENPORT_SINK);
+						if ((hmat_loc->flags &
+						     ACPI_HMAT_MEMORY_HIERARCHY) ==
+						    ACPI_HMAT_MEMORY)
+							continue;
+					}
+
 					hmat_update_target_access(target, type, value,
 								  NODE_ACCESS_CLASS_0);
 					/* If the node has a CPU, update access 1 */