On Sun, 10 Jul 2022 17:32:26 -0700
Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
> Jonathan Cameron wrote:
> > On Thu, 23 Jun 2022 21:19:40 -0700
> > Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
> >
> > > From: Ben Widawsky <bwidawsk@xxxxxxxxxx>
> > >
> > > Add an ABI to allow the number of devices that comprise a region to be
> > > set.
> >
> > Should at least mention interleave_granularity is being added as well!
>
> Added.
>
> >
> > >
> > > Signed-off-by: Ben Widawsky <bwidawsk@xxxxxxxxxx>
> > > [djbw: reword changelog]
> > > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> >
> > Random diversion inline...
> >
> > > ---
> > > Documentation/ABI/testing/sysfs-bus-cxl | 21 ++++
> > > drivers/cxl/core/region.c | 128 ++++++++++++++++++++++++
> > > drivers/cxl/cxl.h | 33 ++++++
> > > 3 files changed, 182 insertions(+)
> >
> > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > > index f75978f846b9..78af42454760 100644
> > > --- a/drivers/cxl/core/region.c
> > > +++ b/drivers/cxl/core/region.c
> > > @@ -7,6 +7,7 @@
> >
> >
> > > +static ssize_t interleave_granularity_store(struct device *dev,
> > > + struct device_attribute *attr,
> > > + const char *buf, size_t len)
> > > +{
> > > + struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev->parent);
> > > + struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
> > > + struct cxl_region *cxlr = to_cxl_region(dev);
> > > + struct cxl_region_params *p = &cxlr->params;
> > > + int rc, val;
> > > + u16 ig;
> > > +
> > > + rc = kstrtoint(buf, 0, &val);
> > > + if (rc)
> > > + return rc;
> > > +
> > > + rc = granularity_to_cxl(val, &ig);
> > > + if (rc)
> > > + return rc;
> > > +
> > > + /* region granularity must be >= root granularity */
> >
> > In general I think that's an implementation choice. Sure today
> > we only support it this way, but it's perfectly possible to build
> > setups where that's not the case.
>
> If the region granularity is smaller than the host bridge interleave
> granularity it means that multiple devices per host bridge are needed to
> satsify a single "slot" in the interleave. Valid? Yes. Useful for Linux
> to support, not clear.
True. Wait and see on this one makes sense to me. I only noticed because
my older test scripts (against hacks on top of Ben's code) were broken as
I did it the silly way :)
>
> > Maybe the comment should say that this code goes with an
> > implementation choice inline with the software guide (that argues you
> > will always prefer small ig for interleaving at the host to make best
> > use of bandwidth etc).
>
> No, I would prefer that as far as the Linux implementation is concerned
> the software-guide does not exist. In the sense that the Linux
> implementation choices supersede and are otherwise a superset of what
> the guide recommends.
ah. I phrased that badly. I just meant lift the argument as a comment rather
than a cross reference.
>
> Also, for the same reason that the code does not anticipate future
> implementation possibilities, neither should the comments. It is
> sufficient to just change this comment when / if the implemetation stops
> expecting region granularity >= root granularity.
>
> > Interestingly the code I was previously testing QEMU with
> > allowed that option (might have been only option that worked).
> > That code was a mixture of Ben's earlier version and my own hacks.
> > It probably doesn't make sense to support other ways of picking
> > the interleaving granularity until / if we ever get a request
> > to do so.
> >
> > I think it results in a different device ordering.
> >
> > Ordering with this
> >
> > Host
> > | 4k
> > / \
> > / \
> > HB HB 8k
> > | |
> > / \ / \
> > 0 2 1 3
> >
> > Ordering with Larger granularity CFMWS over finer granularity HB
> >
> > Host
> > | 8k
> > / \
> > / \
> > HB HB 4k
> > | |
> > / \ / \
> > 0 1 2 3
> >
> > Not clear why you'd do the second one though :) So can ignore for now.
>
> All I can think of is "ZOMG! My platform failed and the only one I have
> to recover my data has HB interleaves with larger granularity than my
> failed system!". Otherwise, I expect cross-platform CXL persistent
> memory recovery to be so rare as to not need to spend time too much time
> worrying about it now. It seems a straightforward constraint to lift at
> a later date without any risk to breaking the ABI.
It was cross platform that I was thinking but you make a fair point that
it is unlikely to occur that often. + If another OS want's to do it wrong
that's their problem :)
J
