linux-kernel - Re: [PATCH -V2] cxl/region: Support to calculate memory tier abstract distance

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zmnc/bIkAQp6dpxQ@aschofie-mobl2>
Date: Wed, 12 Jun 2024 10:38:05 -0700
From: Alison Schofield <alison.schofield@...el.com>
To: "Huang, Ying" <ying.huang@...el.com>
Cc: Dan Williams <dan.j.williams@...el.com>, linux-cxl@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Jonathan Cameron <Jonathan.Cameron@...wei.com>,
	Dave Jiang <dave.jiang@...el.com>, Bharata B Rao <bharata@....com>,
	Alistair Popple <apopple@...dia.com>,
	"Aneesh Kumar K . V" <aneesh.kumar@...ux.ibm.com>
Subject: Re: [PATCH -V2] cxl/region: Support to calculate memory tier
 abstract distance

On Wed, Jun 12, 2024 at 10:09:14AM +0800, Ying Huang wrote:

snip

> >> ---
> >>  drivers/cxl/core/region.c | 40 +++++++++++++++++++++++++++++++++++----
> >>  drivers/cxl/cxl.h         |  2 ++
> >>  2 files changed, 38 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> >> index 3c2b6144be23..81d0910c0a02 100644
> >> --- a/drivers/cxl/core/region.c
> >> +++ b/drivers/cxl/core/region.c
> >> @@ -9,6 +9,7 @@
> >>  #include <linux/uuid.h>
> >>  #include <linux/sort.h>
> >>  #include <linux/idr.h>
> >> +#include <linux/memory-tiers.h>
> >>  #include <cxlmem.h>
> >>  #include <cxl.h>
> >>  #include "core.h"
> >> @@ -2304,14 +2305,20 @@ static bool cxl_region_update_coordinates(struct cxl_region *cxlr, int nid)
> >>  	return true;
> >>  }
> >>  
> >> +static int cxl_region_nid(struct cxl_region *cxlr)
> >> +{
> >> +	struct cxl_region_params *p = &cxlr->params;
> >> +	struct cxl_endpoint_decoder *cxled = p->targets[0];
> >> +	struct cxl_decoder *cxld = &cxled->cxld;
> >> +
> >> +	return phys_to_target_node(cxld->hpa_range.start);
> >> +}
> >> +
> >
> > I believe it's OK to send a resource_size_t to phys_to_target_node()
> > like this:
> >
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -2308,10 +2308,8 @@ static bool cxl_region_update_coordinates(struct cxl_region *cxlr, int nid)
> >  static int cxl_region_nid(struct cxl_region *cxlr)
> >  {
> >         struct cxl_region_params *p = &cxlr->params;
> > -       struct cxl_endpoint_decoder *cxled = p->targets[0];
> > -       struct cxl_decoder *cxld = &cxled->cxld;
> >
> > -       return phys_to_target_node(cxld->hpa_range.start);
> > +       return phys_to_target_node(p->res->start);
> >  }
> >
> 
> I believe this works.  But the original implementation is just a
> mechanical code movement from cxl_region_perf_attrs_callback().  So, I
> prefer to keep it stupid. Then, further optimization can be done on top
> of it.  Is it good for you?

I prefer to do it now while we are thinking about it.

How about a precursor patch:
Patch 1/2: cxl/region: Add a region to node id helper

--and then in that commit log you can say it's a simplified lookup 
and is being done in preparation for adding another caller.

-- Alison

> 
snip
> >