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