Re: [PATCH 3/4] cxl: Introduce 'struct cxl_dpa_partition' and 'struct cxl_range_info'

[Dan Williams <dan.j.williams@intel.com>]

Alejandro Lucero Palau wrote:
> 
> On 1/17/25 18:23, Dan Williams wrote:
> > Jonathan Cameron wrote:
> >> On Thu, 16 Jan 2025 22:10:44 -0800
> >> Dan Williams <dan.j.williams@intel.com> wrote:
> >>
> >>> The pending efforts to add CXL Accelerator (type-2) device [1], and
> >>> Dynamic Capacity (DCD) support [2], tripped on the
> >>> no-longer-fit-for-purpose design in the CXL subsystem for tracking
> >>> device-physical-address (DPA) metadata. Trip hazards include:
> >>>
> >>> - CXL Memory Devices need to consider a PMEM partition, but Accelerator
> >>>    devices with CXL.mem likely do not in the common case.
> >>>
> >>> - CXL Memory Devices enumerate DPA through Memory Device mailbox
> >>>    commands like Partition Info, Accelerators devices do not.
> >>>
> >>> - CXL Memory Devices that support DCD support more than 2 partitions.
> >>>    Some of the driver algorithms are awkward to expand to > 2 partition
> >>>    cases.
> >>>
> >>> - DPA performance data is a general capability that can be shared with
> >>>    accelerators, so tracking it in 'struct cxl_memdev_state' is no longer
> >>>    suitable.
> >>>
> >>> - 'enum cxl_decoder_mode' is sometimes a partition id and sometimes a
> >>>    memory property, it should be phased in favor of a partition id and
> >>>    the memory property comes from the partition info.
> >>>
> >>> Towards cleaning up those issues and allowing a smoother landing for the
> >>> aforementioned pending efforts, introduce a 'struct cxl_dpa_partition'
> >>> array to 'struct cxl_dev_state', and 'struct cxl_range_info' as a shared
> >>> way for Memory Devices and Accelerators to initialize the DPA information
> >>> in 'struct cxl_dev_state'.
> >>>
> >>> For now, split a new cxl_dpa_setup() from cxl_mem_create_range_info() to
> >>> get the new data structure initialized, and cleanup some qos_class init.
> >>> Follow on patches will go further to use the new data structure to
> >>> cleanup algorithms that are better suited to loop over all possible
> >>> partitions.
> >>>
> >>> cxl_dpa_setup() follows the locking expectations of mutating the device
> >>> DPA map, and is suitable for Accelerator drivers to use. Accelerators
> >>> likely only have one hardcoded 'ram' partition to convey to the
> >>> cxl_core.
> >>>
> >>> Link: http://lore.kernel.org/20241230214445.27602-1-alejandro.lucero-palau@amd.com [1]
> >>> Link: http://lore.kernel.org/20241210-dcd-type2-upstream-v8-0-812852504400@intel.com [2]
> >>> Cc: Dave Jiang <dave.jiang@intel.com>
> >>> Cc: Alejandro Lucero <alucerop@amd.com>
> >>> Cc: Ira Weiny <ira.weiny@intel.com>
> >>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> >> Hi Dan,
> >>
> >> In basic form this seems fine, but I find the nr_paritions variable usage very
> >> counter intuitive.  It's just how many we configured not how many there
> >> are, potentially with 0 size (so not a partition).  I'd be happier if we
> >> can avoid that by just prefilling the lot with zero size and filling in
> >> the ones we want.  So zero size means doesn't exist and use an iterator where
> >> appropriate to skip the zero size ones.
> > The PMEM-only device case did give me pause. Is that 2 partitions with a
> > zero-sized first partition, or is that just 1 partition?
> 
> 
> I was wrong about the code being broken for this case.
> 
> The code would create two partitions, at least for the case of partition 
> alignment being 0, with the first one having 0 size.

That was the feedback from Jonathan from v1 => v2. Drop zero-size
partitions from the tracking.

> This is all based on data/code based on mbox commands. Without mbox this 
> partition info needs to be hardcoded or created somehow by the accel 
> driver by its own means, so it is good to know the code expects such a 
> 0-size partition for the pmem-only case and the nr_partitions should be 
> set accordingly. Not what my type2 patchset needs now, but I bet this 
> will need to be set properly by a coming accel driver.

An accelerator does not need to worry about passing in a 0-sized pmem
partition, it can just register the one DPA range that it cares about.
nr_partitions means that the partittion array has entry
[0]..[nr_partitions-1] filled with non-zero data, each with a distinct
operation mode, and in contiguous order starting from zero.

The only place Ira and I identified where this potentially runs into
trouble is if a device places a gap between partitions, or if we want to
skip over "shared" capacity in the first round of DCD support. Might
just need to mandate that userspace skip over "shared" capacity for now.