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

[Jim Harris <jim.harris@samsung.com>]

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.