[PATCH] cxl/region: Refactor logic around check_last_peer()

[PATCH] cxl/region: Refactor logic around check_last_peer()

[Jim Harris]

'distance' is equivalent to the interleave_ways of the switch decoder
of the port we are setting up in cxl_port_setup_targets().

We can also eliminate the special case for passthrough decoders (i.e.
interleave_ways == 1) - check_last_peer() will just compare this
endpoint's dport with the dport of the immediately preceding endpoint
based on position.

Signed-off-by: Jim Harris <jim.harris@samsung.com>
---
 drivers/cxl/core/region.c |   25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 6d63b8798c29..0dc1c78772f1 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1043,10 +1043,10 @@ static void cxl_port_detach_region(struct cxl_port *port,
 }
 
 static int check_last_peer(struct cxl_endpoint_decoder *cxled,
-			   struct cxl_ep *ep, struct cxl_region_ref *cxl_rr,
-			   int distance)
+			   struct cxl_ep *ep, struct cxl_region_ref *cxl_rr)
 {
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
+	int switch_iw = cxl_rr->decoder->interleave_ways;
 	struct cxl_region *cxlr = cxl_rr->region;
 	struct cxl_region_params *p = &cxlr->params;
 	struct cxl_endpoint_decoder *cxled_peer;
@@ -1057,16 +1057,16 @@ static int check_last_peer(struct cxl_endpoint_decoder *cxled,
 
 	/*
 	 * If this position wants to share a dport with the last endpoint mapped
-	 * then that endpoint, at index 'position - distance', must also be
+	 * then that endpoint, at index 'position - switch_iw', must also be
 	 * mapped by this dport.
 	 */
-	if (pos < distance) {
+	if (pos < switch_iw) {
 		dev_dbg(&cxlr->dev, "%s:%s: cannot host %s:%s at %d\n",
 			dev_name(port->uport_dev), dev_name(&port->dev),
 			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), pos);
 		return -ENXIO;
 	}
-	cxled_peer = p->targets[pos - distance];
+	cxled_peer = p->targets[pos - switch_iw];
 	cxlmd_peer = cxled_to_memdev(cxled_peer);
 	ep_peer = cxl_ep_load(port, cxlmd_peer);
 	if (ep->dport != ep_peer->dport) {
@@ -1111,20 +1111,17 @@ static int cxl_port_setup_targets(struct cxl_port *port,
 
 	cxlsd = to_cxl_switch_decoder(&cxld->dev);
 	if (cxl_rr->nr_targets_set) {
-		int i, distance;
+		int i;
 
 		/*
-		 * Passthrough decoders impose no distance requirements between
-		 * peers
+		 * Check if this endpoint's dport is already in the
+		 * switch decoder's target list, and if so check that
+		 * it is positioned correctly based on the switch's
+		 * interleave.
 		 */
-		if (cxl_rr->nr_targets == 1)
-			distance = 0;
-		else
-			distance = p->nr_targets / cxl_rr->nr_targets;
 		for (i = 0; i < cxl_rr->nr_targets_set; i++)
 			if (ep->dport == cxlsd->target[i]) {
-				rc = check_last_peer(cxled, ep, cxl_rr,
-						     distance);
+				rc = check_last_peer(cxled, ep, cxl_rr);
 				if (rc)
 					return rc;
 				goto out_target_set;

RE: [PATCH] cxl/region: Refactor logic around check_last_peer()

[Dan Williams]

Jim Harris wrote:
> 'distance' is equivalent to the interleave_ways of the switch decoder
> of the port we are setting up in cxl_port_setup_targets().
> 
> We can also eliminate the special case for passthrough decoders (i.e.
> interleave_ways == 1) - check_last_peer() will just compare this
> endpoint's dport with the dport of the immediately preceding endpoint
> based on position.

It turns out this patch fails cxl-region-sysfs.sh.

    test/cxl-region-sysfs.sh: failed at line 94
    
    [   38.367581] check_last_peer: cxl region8: cxl_host_bridge.0:port4: mem5:decoder15.1 pos 4 mismatched peer mem7:decoder17.0
    
This patch looked so appetizing that I really wanted it to be a bug in
the test and not a bug in this patch, but I think it is the latter.

> @@ -1111,20 +1111,17 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>  
>       cxlsd = to_cxl_switch_decoder(&cxld->dev);
>       if (cxl_rr->nr_targets_set) {
> -             int i, distance;
> +             int i;
>  
>               /*
> -              * Passthrough decoders impose no distance requirements between
> -              * peers
> +              * Check if this endpoint's dport is already in the
> +              * switch decoder's target list, and if so check that
> +              * it is positioned correctly based on the switch's
> +              * interleave.
>                */
> -             if (cxl_rr->nr_targets == 1)
> -                     distance = 0;
> -             else
> -                     distance = p->nr_targets / cxl_rr->nr_targets;

This calculation is essentially doing the "top-down" version of the
"bottom-up" position calculation Alison introduced in her proposed region
assembly fixes:

    for_each_parent_port(...)
        pos = pos * parent_ways + parent_pos

So in a x8 region across 2x HBs with 2x switches per HB. The "distance"
of peers at the switch level is 4. This change makes that 2.

Maybe the right conceptual cleanup is to still ditch this distance
calculation based on "p->nr_targets / cxl_rr->nr_targets" and walk up
from this port and multiply the local ways by all the ancestral ways,
but as is this gets the answer with less steps (modulo all the work to
build up @ep and @cxl_rr).

Re: [PATCH] cxl/region: Refactor logic around check_last_peer()

[Jim Harris]

On Tue, Oct 24, 2023 at 07:49:47PM -0700, Dan Williams wrote:
> 
> It turns out this patch fails cxl-region-sysfs.sh.
> 
>     test/cxl-region-sysfs.sh: failed at line 94
>     
>     [   38.367581] check_last_peer: cxl region8: cxl_host_bridge.0:port4: mem5:decoder15.1 pos 4 mismatched peer mem7:decoder17.0
>     
> This patch looked so appetizing that I really wanted it to be a bug in
> the test and not a bug in this patch, but I think it is the latter.
> 
> > @@ -1111,20 +1111,17 @@ static int cxl_port_setup_targets(struct cxl_port *port,
> >  
> >       cxlsd = to_cxl_switch_decoder(&cxld->dev);
> >       if (cxl_rr->nr_targets_set) {
> > -             int i, distance;
> > +             int i;
> >  
> >               /*
> > -              * Passthrough decoders impose no distance requirements between
> > -              * peers
> > +              * Check if this endpoint's dport is already in the
> > +              * switch decoder's target list, and if so check that
> > +              * it is positioned correctly based on the switch's
> > +              * interleave.
> >                */
> > -             if (cxl_rr->nr_targets == 1)
> > -                     distance = 0;
> > -             else
> > -                     distance = p->nr_targets / cxl_rr->nr_targets;
> 
> This calculation is essentially doing the "top-down" version of the
> "bottom-up" position calculation Alison introduced in her proposed region
> assembly fixes:
> 
>     for_each_parent_port(...)
>         pos = pos * parent_ways + parent_pos
> 
> So in a x8 region across 2x HBs with 2x switches per HB. The "distance"
> of peers at the switch level is 4. This change makes that 2.
> 
> Maybe the right conceptual cleanup is to still ditch this distance
> calculation based on "p->nr_targets / cxl_rr->nr_targets" and walk up
> from this port and multiply the local ways by all the ancestral ways,
> but as is this gets the answer with less steps (modulo all the work to
> build up @ep and @cxl_rr).

Thanks Dan. I'll run with that and push out a v2.

Re: [PATCH] cxl/region: Refactor logic around check_last_peer()

[Jim Harris]

On Wed, Oct 25, 2023 at 10:37:40PM +0000, Jim Harris wrote:
> On Tue, Oct 24, 2023 at 07:49:47PM -0700, Dan Williams wrote:
> > 
> > It turns out this patch fails cxl-region-sysfs.sh.
> > 
> >     test/cxl-region-sysfs.sh: failed at line 94
> >     
> >     [   38.367581] check_last_peer: cxl region8: cxl_host_bridge.0:port4: mem5:decoder15.1 pos 4 mismatched peer mem7:decoder17.0
> >     
> > This patch looked so appetizing that I really wanted it to be a bug in
> > the test and not a bug in this patch, but I think it is the latter.
> > 
> > > @@ -1111,20 +1111,17 @@ static int cxl_port_setup_targets(struct cxl_port *port,
> > >  
> > >       cxlsd = to_cxl_switch_decoder(&cxld->dev);
> > >       if (cxl_rr->nr_targets_set) {
> > > -             int i, distance;
> > > +             int i;
> > >  
> > >               /*
> > > -              * Passthrough decoders impose no distance requirements between
> > > -              * peers
> > > +              * Check if this endpoint's dport is already in the
> > > +              * switch decoder's target list, and if so check that
> > > +              * it is positioned correctly based on the switch's
> > > +              * interleave.
> > >                */
> > > -             if (cxl_rr->nr_targets == 1)
> > > -                     distance = 0;
> > > -             else
> > > -                     distance = p->nr_targets / cxl_rr->nr_targets;
> > 
> > This calculation is essentially doing the "top-down" version of the
> > "bottom-up" position calculation Alison introduced in her proposed region
> > assembly fixes:
> > 
> >     for_each_parent_port(...)
> >         pos = pos * parent_ways + parent_pos
> > 
> > So in a x8 region across 2x HBs with 2x switches per HB. The "distance"
> > of peers at the switch level is 4. This change makes that 2.
> > 
> > Maybe the right conceptual cleanup is to still ditch this distance
> > calculation based on "p->nr_targets / cxl_rr->nr_targets" and walk up
> > from this port and multiply the local ways by all the ancestral ways,
> > but as is this gets the answer with less steps (modulo all the work to
> > build up @ep and @cxl_rr).
> 
> Thanks Dan. I'll run with that and push out a v2.

I played around with this idea. Walking up from this port is easy, but we
need the port's decoder to get the interleave_ways. That's easy too, we
can get the decoder by doing an xa_load() on the port's regions xarray
with the region pointer. Except that we don't attach region_refs to the
root decoder.

I have a locally-tested patch that attaches region_refs to the root decoder,
but it didn't seem warranted for just refactoring this function. So I pushed
out a v2 that just renames "distance" to "ancestral_ways" and moved all
of the related calculations into check_last_peer() itself.

[PATCH v2] cxl/region: Refactor logic around check_last_peer()

[Jim Harris]

'distance' is equivalent to the product of the interleave_ways of the
"ancestor" decoders of the port's decoder we are setting up in
cxl_port_setup_targets().

So use the term "ancestral_ways" instead of "distance" to better
clarify the meaning of this value. Also move all logic around this
value directly into check_last_peer() so that all necessary calculations
are in one place. It also allows eliminating a parameter to that
function.

Signed-off-by: Jim Harris <jim.harris@samsung.com>
---
 drivers/cxl/core/region.c |   33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index a1eac592c66a..2edfc02bb766 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1037,8 +1037,7 @@ static void cxl_port_detach_region(struct cxl_port *port,
 }
 
 static int check_last_peer(struct cxl_endpoint_decoder *cxled,
-			   struct cxl_ep *ep, struct cxl_region_ref *cxl_rr,
-			   int distance)
+			   struct cxl_ep *ep, struct cxl_region_ref *cxl_rr)
 {
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
 	struct cxl_region *cxlr = cxl_rr->region;
@@ -1048,19 +1047,30 @@ static int check_last_peer(struct cxl_endpoint_decoder *cxled,
 	struct cxl_memdev *cxlmd_peer;
 	struct cxl_ep *ep_peer;
 	int pos = cxled->pos;
+	int ancestral_ways;
+
+	if (cxl_rr->nr_targets == 1) {
+		/*
+		 * Passthrough decoders impose no positioning requirements
+		 * between peers.
+		 */
+		return 0;
+	}
+
+	ancestral_ways = p->nr_targets / cxl_rr->nr_targets;
 
 	/*
 	 * If this position wants to share a dport with the last endpoint mapped
-	 * then that endpoint, at index 'position - distance', must also be
+	 * then that endpoint, at index 'position - ancestral_ways', must also be
 	 * mapped by this dport.
 	 */
-	if (pos < distance) {
+	if (pos < ancestral_ways) {
 		dev_dbg(&cxlr->dev, "%s:%s: cannot host %s:%s at %d\n",
 			dev_name(port->uport_dev), dev_name(&port->dev),
 			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), pos);
 		return -ENXIO;
 	}
-	cxled_peer = p->targets[pos - distance];
+	cxled_peer = p->targets[pos - ancestral_ways];
 	cxlmd_peer = cxled_to_memdev(cxled_peer);
 	ep_peer = cxl_ep_load(port, cxlmd_peer);
 	if (ep->dport != ep_peer->dport) {
@@ -1105,20 +1115,15 @@ static int cxl_port_setup_targets(struct cxl_port *port,
 
 	cxlsd = to_cxl_switch_decoder(&cxld->dev);
 	if (cxl_rr->nr_targets_set) {
-		int i, distance;
+		int i;
 
 		/*
-		 * Passthrough decoders impose no distance requirements between
-		 * peers
+		 * Check if this ep is already in the switch target list and
+		 * if so ensure it meets relative positioning requirements.
 		 */
-		if (cxl_rr->nr_targets == 1)
-			distance = 0;
-		else
-			distance = p->nr_targets / cxl_rr->nr_targets;
 		for (i = 0; i < cxl_rr->nr_targets_set; i++)
 			if (ep->dport == cxlsd->target[i]) {
-				rc = check_last_peer(cxled, ep, cxl_rr,
-						     distance);
+				rc = check_last_peer(cxled, ep, cxl_rr);
 				if (rc)
 					return rc;
 				goto out_target_set;

Re: [PATCH v2] cxl/region: Refactor logic around check_last_peer()

[Alison Schofield]

On Wed, Nov 01, 2023 at 05:02:18AM +0000, Jim Harris wrote:
> 'distance' is equivalent to the product of the interleave_ways of the
> "ancestor" decoders of the port's decoder we are setting up in
> cxl_port_setup_targets().
> 
> So use the term "ancestral_ways" instead of "distance" to better
> clarify the meaning of this value. Also move all logic around this
> value directly into check_last_peer() so that all necessary calculations
> are in one place. It also allows eliminating a parameter to that
> function.

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

A couple of housekeeping notes:
- Use get_maintainers script to find the To: List
- It's customary to send revised patches as a new thread, not as a
  reply to previous version.

Thanks,
Alison


> 
> Signed-off-by: Jim Harris <jim.harris@samsung.com>
> ---
>  drivers/cxl/core/region.c |   33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index a1eac592c66a..2edfc02bb766 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1037,8 +1037,7 @@ static void cxl_port_detach_region(struct cxl_port *port,
>  }
>  
>  static int check_last_peer(struct cxl_endpoint_decoder *cxled,
> -			   struct cxl_ep *ep, struct cxl_region_ref *cxl_rr,
> -			   int distance)
> +			   struct cxl_ep *ep, struct cxl_region_ref *cxl_rr)
>  {
>  	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
>  	struct cxl_region *cxlr = cxl_rr->region;
> @@ -1048,19 +1047,30 @@ static int check_last_peer(struct cxl_endpoint_decoder *cxled,
>  	struct cxl_memdev *cxlmd_peer;
>  	struct cxl_ep *ep_peer;
>  	int pos = cxled->pos;
> +	int ancestral_ways;
> +
> +	if (cxl_rr->nr_targets == 1) {
> +		/*
> +		 * Passthrough decoders impose no positioning requirements
> +		 * between peers.
> +		 */
> +		return 0;
> +	}
> +
> +	ancestral_ways = p->nr_targets / cxl_rr->nr_targets;
>  
>  	/*
>  	 * If this position wants to share a dport with the last endpoint mapped
> -	 * then that endpoint, at index 'position - distance', must also be
> +	 * then that endpoint, at index 'position - ancestral_ways', must also be
>  	 * mapped by this dport.
>  	 */
> -	if (pos < distance) {
> +	if (pos < ancestral_ways) {
>  		dev_dbg(&cxlr->dev, "%s:%s: cannot host %s:%s at %d\n",
>  			dev_name(port->uport_dev), dev_name(&port->dev),
>  			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), pos);
>  		return -ENXIO;
>  	}
> -	cxled_peer = p->targets[pos - distance];
> +	cxled_peer = p->targets[pos - ancestral_ways];
>  	cxlmd_peer = cxled_to_memdev(cxled_peer);
>  	ep_peer = cxl_ep_load(port, cxlmd_peer);
>  	if (ep->dport != ep_peer->dport) {
> @@ -1105,20 +1115,15 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>  
>  	cxlsd = to_cxl_switch_decoder(&cxld->dev);
>  	if (cxl_rr->nr_targets_set) {
> -		int i, distance;
> +		int i;
>  
>  		/*
> -		 * Passthrough decoders impose no distance requirements between
> -		 * peers
> +		 * Check if this ep is already in the switch target list and
> +		 * if so ensure it meets relative positioning requirements.
>  		 */
> -		if (cxl_rr->nr_targets == 1)
> -			distance = 0;
> -		else
> -			distance = p->nr_targets / cxl_rr->nr_targets;
>  		for (i = 0; i < cxl_rr->nr_targets_set; i++)
>  			if (ep->dport == cxlsd->target[i]) {
> -				rc = check_last_peer(cxled, ep, cxl_rr,
> -						     distance);
> +				rc = check_last_peer(cxled, ep, cxl_rr);
>  				if (rc)
>  					return rc;
>  				goto out_target_set;
>