Message-ID: <67915ce296030_20fa29457@dwillia2-xfh.jf.intel.com.notmuch>
Date: Wed, 22 Jan 2025 13:02:26 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>, Dan Williams
	<dan.j.williams@...el.com>
CC: Ira Weiny <ira.weiny@...el.com>, Dave Jiang <dave.jiang@...el.com>, Fan Ni
	<fan.ni@...sung.com>, Jonathan Corbet <corbet@....net>, Andrew Morton
	<akpm@...ux-foundation.org>, Kees Cook <kees@...nel.org>, "Gustavo A. R.
 Silva" <gustavoars@...nel.org>, Davidlohr Bueso <dave@...olabs.net>, "Alison
 Schofield" <alison.schofield@...el.com>, Vishal Verma
	<vishal.l.verma@...el.com>, <linux-cxl@...r.kernel.org>,
	<linux-doc@...r.kernel.org>, <nvdimm@...ts.linux.dev>,
	<linux-kernel@...r.kernel.org>, <linux-hardening@...r.kernel.org>, Li Ming
	<ming.li@...omail.com>
Subject: Re: [PATCH v8 02/21] cxl/mem: Read dynamic capacity configuration
 from the device

Jonathan Cameron wrote:
> On Wed, 15 Jan 2025 14:34:36 -0800
> Dan Williams <dan.j.williams@...el.com> wrote:
> 
> > Ira Weiny wrote:
> > > Dan Williams wrote:  
> > > > Ira Weiny wrote:  
> > > 
> > > [snip]
> > >   
> > > > > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > > > > index e8907c403edbd83c8a36b8d013c6bc3391207ee6..05a0718aea73b3b2a02c608bae198eac7c462523 100644
> > > > > --- a/drivers/cxl/cxlmem.h
> > > > > +++ b/drivers/cxl/cxlmem.h
> > > > > @@ -403,6 +403,7 @@ enum cxl_devtype {
> > > > >  	CXL_DEVTYPE_CLASSMEM,
> > > > >  };
> > > > >  
> > > > > +#define CXL_MAX_DC_REGION 8  
> > > > 
> > > > Please no, lets not sign up to have the "which cxl 'region' concept are
> > > > you referring to?" debate in perpetuity. "DPA partition", "DPA
> > > > resource", "DPA capacity" anything but "region".
> > > >   
> > > 
> > > I'm inclined to agree with Alejandro on this one.  I've walked this
> > > tightrope quite a bit with this series.  But there are other places where
> > > we have chosen to change the verbiage from the spec and it has made it
> > > difficult for new comers to correlate the spec with the code.
> > > 
> > > So I like Alejandro's idea of adding "HW" to the name to indicate that we
> > > are talking about a spec or hardware defined thing.  
> > 
> > See below, the only people that could potentially be bothered by the
> > lack of spec terminology matching are the very same people that are
> > sophisticated enough to have read the spec to know its a problem.
> 
> It's confusing me.  :)  I know the confusion source exists but
> that doesn't mean I remember how all the terms match up.

CXL 3.1 Figure 9-24 DCD DPA Space Example

In that one diagram it uses "space", "capacity", "partition", and
"region". Linux is free to say "let's just pick one term and stick to
it". "Region" is already oversubscribed.

I agree with Alejandro that a glossary of Linux terms added to the
Documentation is overdue and would help people orient to what maps
where. That would be needed even if the "continue to oversubscribe
'region'" proposal went through to explain "oh, no, not that 'region'
*this* 'region'".

[..]
> > Actually these buffers provide a buffer for the (struct
> > > resource)dc_res[x].name pointers to point to.  
> > 
> > I missed that specific detail, but I still challenge whether this
> > precision is needed especially since it makes the data structure
> > messier. Given these names are for debug only and multi-partition DCD
> > devices seem unlikely to ever exist, just use a static shared name for
> > adding to ->dpa_res.
> 
> Given the read only shared concept relies on multiple hardware dc regions
> (I think they map to partitions) then we are very likely to see
> multiples.  (maybe I'm lost in terminology as well).

Ah, good point. I was focusing on "devices with DPA partitions of
different performance characteristics within the same operation mode" as
being unlikely, but "devices with both shared and non-shared capacity"
indeed seems more likely.

Now, part of the code smell that made me fall out of love with 'enum
cxl_decoder_mode' was this continued confusion between mode names and
partition ids, where printing "dc%d" to the resource name was part of
that smell.

The proposal for what goes into the "name" field of partition resources
in the "DPA metadata is a mess..." series is to disconnect operation
modes from partition indices. A natural consequence of allowing "pmem"
to be partition 0, is that a dynamic device may also have 0 static
capacity, or other arrangements that make the partition-id less
meaningful to userspace.

So instead of needing to print "dc%d" into the resource name field, the
resource name is simply the operation mode: ram, pmem, dynamic ram,
dynamic pmem*, shared ram, shared pmem*.

The implication is that userspace does not need to care about partition
ids, unless and until a device shows up that ships multiple partitions
with the same operation mode, but different performance characteristics.
If that happens userspace would need a knob to disambiguate partitions
with the same operation mode. That does not feel like something that is
threatening to become real in the near term, and partition ids can
continue to be hidden from userspace.

* I doubt we will see dynamic pmem or shared pmem.

[..]
> > > > Linux is not obligated to follow the questionable naming decisions of
> > > > specifications.  
> > > 
> > > We are not.  But as Alejandro says it can be confusing if we don't make
> > > some association to the spec.
> > > 
> > > What do you think about the HW/SW line I propose above?  
> > 
> > Rename to cxl_dc_partition_info and drop the region_ prefixes, sure.
> > 
> > Otherwise, for this init-time only concern I would much rather deal with
> > the confusion of:
> > 
> > "why does Linux call this partition when the spec calls it region?":
> > which only trips up people that already know the difference because they read the
> > spec. In that case the comment will answer their confusion.
> > 
> > ...versus:
> > 
> > "why are there multiple region concepts in the CXL subsystem": which
> > trips up everyone that greps through the CXL subsystem especially those
> > that have no intention of ever reading the spec.
> 
> versus one time rename of all internal infrastructure to align to the spec
> and only keep the confusion at the boundaries where we have ABI.

That's just it, to date 'region' has always meant 'struct cxl_region' in
drivers/cxl/, so there is no one time rename to be had. The decision is
whether to decline new claimers of the 'region' moniker and create a
document to explain that term, or play the "dc region" ambiguity game
for the duration.

I vote "diverge from spec and document".

> Horrible option but how often are those diving in the code that bothered
> about the userspace /kernel interaction terminology?
> 
> Anyhow, they are all horrible choices.

Agree!

Powered by blists - more mailing lists