Re: [PATCH] cxl/region: Delay inserting iomem resource until auto region commit

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

Alison Schofield wrote:
> During auto region assembly the region driver inserts the region
> resource into the iomem tree when the first endpoint arrives and
> region assembly begins. If the region later fails to assemble, the
> resource can remain stranded in the iomem tree, making it appear like
> a DAX region is a child of the CXL region, when that is not true.
> 
> For example:
> 68e80000000-8d37fffffff : CXL Window 9
>   68e80000000-70e7fffffff : Soft Reserved
>     68e80000000-70e7fffffff : region9
>       68e80000000-70e7fffffff : dax19.0
>         68e80000000-70e7fffffff : System RAM (kmem)

...but it *is* telling the truth. The truth is that multiple objects
have laid a claim to that address range. The region9 object has reserved
part of the CXL window for its use. If the "Soft Reserved" and "dax19.0"
reservations are removed then the address space should still be reserved
for the region. The collision only occurs when drivers for those
competing objects try to mark the range IORESOURCE_BUSY. Competing
claims to the same address range are why IORESOURCE_BUSY exists, benign
overlaps are ok.

Note that CXL Window 9 is also reserving more address space than is
actually in use. Like region9 70e80000000-8d37fffffff is reserved
address space with no active decode.

> In the above case, region9 failed to assemble, yet proc/iomem shows
> the DAX region as being parented under a CXL region. In reality, the
> CXL region is in a disabled state and the DAX region is managed by the
> HMEM driver.

iomem resource parenting shows registration ordering, not actual
parenting. Most of the time the registration order and the parenting
lines up, but I would be surprised if any use case depends on this.

> Examining /proc/iomem is one way users inspect the memory topology,
> and with this patch that view remains accurate.

The ambiguity of iomem resource parenting can be resolved by looking the
device-path for dax19.0 ("daxctl list -RDu"). It is also resolved by
noticing that the CXL region and the DAX region are using separate
memregion_id values (9 vs 19).

The extra effort for incremental precision is not needed, the contents
of /proc/iomem are accurate.

> Delay insertion of the iomem resource until the auto region reaches
> the commit state. Introduce the res_want_insert field to track whether
> the region's resource should be inserted into the iomem tree.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
> 
> Putting this out for comments and I expect to rebase on 7.0-rc1 if 
> this is wanted.
> 
> Today it is built upon Smita's v6 Soft Reserved set [1] because it
> is with that set where the failover to DAX starts happening and the
> confusing /proc/iomem can appear. Without that set, the resource of 
> the failed region appears in /proc/iomem, but it's less confusing
> since it doesn't show any children.
> 
> There is an option for Smita's set to teardown the CXL regions when
> it takes over the resource for HMEM DAX, however latest revision, v6,
> has taken a gentler approach and leaves the regions intact.
> 
> 
> [1] https://lore.kernel.org/linux-cxl/20260210064501.157591-1-Smita.KoralahalliChannabasappa@amd.com/
> 
> 
>  drivers/cxl/core/region.c | 32 ++++++++++++++++++++------------
>  drivers/cxl/cxl.h         |  4 ++++
>  2 files changed, 24 insertions(+), 12 deletions(-)
> 
[..]
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index c796c3db36e0..2b977ab33af6 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -480,6 +480,9 @@ enum cxl_config_state {
>   * @interleave_ways: number of endpoints in the region
>   * @interleave_granularity: capacity each endpoint contributes to a stripe
>   * @res: allocated iomem capacity for this region
> + * @res_want_insert: true if the resource should be inserted into the iomem
> + *		tree. Set to false after the first attempt to insert or if
> + *		res originates from the iomem tree via alloc_free_mem_region()

This is too much "control flow in a data structure" for my taste, but is
moot given the comments above that this extra effort is not necessary.