| Message ID | 169696721124.1190606.18028412676865061799.stgit@bgt-140510-bm03.eng.stellus.in |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl/region: Refactor logic around check_last_peer() | expand |
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).
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.
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.
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;
'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(-)