Received-SPF: pass (google.com: domain of linux-kernel+bounces-212019-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 12 Jun 2024 10:38:05 -0700
From: Alison Schofield <alison.schofield@intel.com>
To: "Huang, Ying" <ying.huang@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>, linux-cxl@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Dave Jiang <dave.jiang@intel.com>, Bharata B Rao <bharata@amd.com>,
	Alistair Popple <apopple@nvidia.com>,
	"Aneesh Kumar K . V" <aneesh.kumar@linux.ibm.com>
Subject: Re: [PATCH -V2] cxl/region: Support to calculate memory tier
 abstract distance
Message-ID: <Zmnc/bIkAQp6dpxQ@aschofie-mobl2>
References: <20240611055423.470574-1-ying.huang@intel.com>
 <ZmjBfcaosIlOODFR@aschofie-mobl2>
 <87sexi7vzp.fsf@yhuang6-desk2.ccr.corp.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87sexi7vzp.fsf@yhuang6-desk2.ccr.corp.intel.com>

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
> >