Re: [PATCH v6 8/9] dax/hmem, cxl: Defer and resolve ownership of Soft Reserved memory ranges

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

Smita Koralahalli wrote:
> The current probe time ownership check for Soft Reserved memory based
> solely on CXL window intersection is insufficient. dax_hmem probing is not
> always guaranteed to run after CXL enumeration and region assembly, which
> can lead to incorrect ownership decisions before the CXL stack has
> finished publishing windows and assembling committed regions.
> 
> Introduce deferred ownership handling for Soft Reserved ranges that
> intersect CXL windows. When such a range is encountered during dax_hmem
> probe, schedule deferred work and wait for the CXL stack to complete
> enumeration and region assembly before deciding ownership.
> 
> Evaluate ownership of Soft Reserved ranges based on CXL region
> containment.
> 
>    - If all Soft Reserved ranges are fully contained within committed CXL
>      regions, DROP handling Soft Reserved ranges from dax_hmem and allow
>      dax_cxl to bind.
> 
>    - If any Soft Reserved range is not fully claimed by committed CXL
>      region, REGISTER the Soft Reserved ranges with dax_hmem.
> 
> Use dax_cxl_mode to coordinate ownership decisions for Soft Reserved
> ranges. Once, ownership resolution is complete, flush the deferred work
> from dax_cxl before allowing dax_cxl to bind.
> 
> This enforces a strict ownership. Either CXL fully claims the Soft
> reserved ranges or it relinquishes it entirely.

We have had multiple suggestions during the course of developing this
state machine, but I can not see reading this changelog or the
implementation that the full / final state machine is laid out with all
the old ideas cleaned out of the implementation.

For example, I think this has my "untested!" suggestion from:

http://lore.kernel.org/697acf78acf70_3095100c@dwillia2-mobl4.notmuch

...but it does not have the explanation of why it turned out to be
suitable and fits the end goal state machine.

It also has the original definition of "enum dax_cxl_mode". However,
with the recent simplification proposal to stop doing total CXL unwind I
think it allows for a more straightforward state machine. For example,
the "drop" state is now automatic simply by losing the race with
dax_hmem, right?

I think we are close, just some final complexity shaving.

So, with the decision to stop tearing down CXL this state machine only
has 3 requirements.

1/ CXL enumeration needs to start before dax_hmem invokes
   wait_for_device_probe().

2/ dax_cxl driver registration needs to be postponed until after
   dax_hmem has dispositioned all its regions. 

3/ No probe path can flush the work because of the wait_for_device_probe().

Requirement 1/ is met by patch1. Requirement 2/ partially met, has a
proposal here around flushing the work from a separate workqueue
invocation, but I think you want the dependency directly on the dax_hmem
module (if enabled). Requirement 3/ not achieved.

For 3/ I think we can borrow what cxl_mem_probe() does and do:

if (work_pending(&dax_hmem_work))
	return -EBUSY;

...if for some reason someone really wants to rebind the dax_hmem driver
they need to flush the queue, and that can be achived by a flush_work()
in the module_exit() path.

This does mean that patch 7 in this series disappears because bus.c has
no role to play in this mess. It is just dax_hmem and dax_cxl getting
their ordering straight.

Some notes on the implications follow:

> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
> ---
>  drivers/dax/bus.c       |  3 ++
>  drivers/dax/bus.h       | 19 ++++++++++
>  drivers/dax/cxl.c       |  1 +
>  drivers/dax/hmem/hmem.c | 78 +++++++++++++++++++++++++++++++++++++++--
>  4 files changed, 99 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
> index 92b88952ede1..81985bcc70f9 100644
> --- a/drivers/dax/bus.c
> +++ b/drivers/dax/bus.c
> @@ -25,6 +25,9 @@ DECLARE_RWSEM(dax_region_rwsem);
>   */
>  DECLARE_RWSEM(dax_dev_rwsem);
>  
> +enum dax_cxl_mode dax_cxl_mode = DAX_CXL_MODE_DEFER;
> +EXPORT_SYMBOL_NS_GPL(dax_cxl_mode, "CXL");
> +
>  static DEFINE_MUTEX(dax_hmem_lock);
>  static dax_hmem_deferred_fn hmem_deferred_fn;
>  static void *dax_hmem_data;
> diff --git a/drivers/dax/bus.h b/drivers/dax/bus.h
> index b58a88e8089c..82616ff52fd1 100644
> --- a/drivers/dax/bus.h
> +++ b/drivers/dax/bus.h
> @@ -41,6 +41,25 @@ struct dax_device_driver {
>  	void (*remove)(struct dev_dax *dev);
>  };
>  
> +/*
> + * enum dax_cxl_mode - State machine to determine ownership for CXL
> + * tagged Soft Reserved memory ranges.
> + * @DAX_CXL_MODE_DEFER: Ownership resolution pending. Set while waiting
> + * for CXL enumeration and region assembly to complete.
> + * @DAX_CXL_MODE_REGISTER: CXL regions do not fully cover Soft Reserved
> + * ranges. Fall back to registering those ranges via dax_hmem.
> + * @DAX_CXL_MODE_DROP: All Soft Reserved ranges intersecting CXL windows
> + * are fully contained within committed CXL regions. Drop HMEM handling
> + * and allow dax_cxl to bind.

With the above, dax_cxl_mode disappears.

> diff --git a/drivers/dax/cxl.c b/drivers/dax/cxl.c
> index a2136adfa186..3ab39b77843d 100644
> --- a/drivers/dax/cxl.c
> +++ b/drivers/dax/cxl.c
> @@ -44,6 +44,7 @@ static struct cxl_driver cxl_dax_region_driver = {
>  
>  static void cxl_dax_region_driver_register(struct work_struct *work)
>  {
> +	dax_hmem_flush_work();

Looks ok, as long as that symbol is from dax_hmem.ko which gets you the
load dependency (requirement 2/).

Might also want to make sure that all this deferral mess disappears when
CONFIG_DEV_DAX_HMEM=n.

>  	cxl_driver_register(&cxl_dax_region_driver);
>  }
>  
> diff --git a/drivers/dax/hmem/hmem.c b/drivers/dax/hmem/hmem.c
> index 1e3424358490..85854e25254b 100644
> --- a/drivers/dax/hmem/hmem.c
> +++ b/drivers/dax/hmem/hmem.c
> @@ -3,6 +3,7 @@
>  #include <linux/memregion.h>
>  #include <linux/module.h>
>  #include <linux/dax.h>
> +#include <cxl/cxl.h>
>  #include "../bus.h"
>  
>  static bool region_idle;
> @@ -69,8 +70,18 @@ static int hmem_register_device(struct device *host, int target_nid,
>  	if (IS_ENABLED(CONFIG_DEV_DAX_CXL) &&
>  	    region_intersects(res->start, resource_size(res), IORESOURCE_MEM,
>  			      IORES_DESC_CXL) != REGION_DISJOINT) {
> -		dev_dbg(host, "deferring range to CXL: %pr\n", res);
> -		return 0;
> +		switch (dax_cxl_mode) {
> +		case DAX_CXL_MODE_DEFER:
> +			dev_dbg(host, "deferring range to CXL: %pr\n", res);
> +			dax_hmem_queue_work();

This case is just a flag that determines if the work queue has completed
its one run. So I expect this something like:

if (!dax_hmem_initial_probe) {
	queue_work()
	return;
}

Otherwise just go ahead and register because dax_cxl by this time has
had a chance to have a say and the system falls back to "first come /
first served" mode. In other words the simplification of not cleaning up
goes both ways. dax_hmem naturally fails if dax_cxl already claimed the
address range.

> +			return 0;
> +		case DAX_CXL_MODE_REGISTER:
> +			dev_dbg(host, "registering CXL range: %pr\n", res);
> +			break;
> +		case DAX_CXL_MODE_DROP:
> +			dev_dbg(host, "dropping CXL range: %pr\n", res);
> +			return 0;
> +		}
>  	}
>  
>  	rc = region_intersects_soft_reserve(res->start, resource_size(res));
> @@ -123,8 +134,70 @@ static int hmem_register_device(struct device *host, int target_nid,
>  	return rc;
>  }
>  
> +static int hmem_register_cxl_device(struct device *host, int target_nid,
> +				    const struct resource *res)
> +{
> +	if (region_intersects(res->start, resource_size(res), IORESOURCE_MEM,
> +			      IORES_DESC_CXL) != REGION_DISJOINT)
> +		return hmem_register_device(host, target_nid, res);
> +
> +	return 0;
> +}
> +
> +static int soft_reserve_has_cxl_match(struct device *host, int target_nid,
> +				      const struct resource *res)
> +{
> +	if (region_intersects(res->start, resource_size(res), IORESOURCE_MEM,
> +			      IORES_DESC_CXL) != REGION_DISJOINT) {
> +		if (!cxl_region_contains_soft_reserve((struct resource *)res))
> +			return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +static void process_defer_work(void *data)
> +{
> +	struct platform_device *pdev = data;
> +	int rc;
> +
> +	/* relies on cxl_acpi and cxl_pci having had a chance to load */
> +	wait_for_device_probe();
> +
> +	rc = walk_hmem_resources(&pdev->dev, soft_reserve_has_cxl_match);
> +
> +	if (!rc) {
> +		dax_cxl_mode = DAX_CXL_MODE_DROP;
> +		dev_dbg(&pdev->dev, "All Soft Reserved ranges claimed by CXL\n");
> +	} else {
> +		dax_cxl_mode = DAX_CXL_MODE_REGISTER;
> +		dev_warn(&pdev->dev,
> +			 "Soft Reserved not fully contained in CXL; using HMEM\n");
> +	}
> +
> +	walk_hmem_resources(&pdev->dev, hmem_register_cxl_device);

I do not think we need to do 2 passes. Just do one
hmem_register_cxl_device() pass that skips a range when
cxl_region_contains_resource() has it covered, otherwise register an
hmem device.

> +}
> +
> +static void kill_defer_work(void *data)
> +{
> +	struct platform_device *pdev = data;
> +
> +	dax_hmem_flush_work();
> +	dax_hmem_unregister_work(process_defer_work, pdev);
> +}
> +
>  static int dax_hmem_platform_probe(struct platform_device *pdev)
>  {
> +	int rc;

This wants a work_pending() return -EBUSY per above.

> +	rc = dax_hmem_register_work(process_defer_work, pdev);
> +	if (rc)
> +		return rc;

The work does not need to be registered every time. Remember this is
only a one-shot problem at first kernel boot, not every time this
platform device is probed. After the workqueue has run at least once it
never needs to be invoked again if dax_hmem is reloaded.

A flag for "dax_hmem flushed initial device probe at least once" needs
to live in drivers/dax/hmem/device.c and be cleared by
process_defer_work().

> +
> +	rc = devm_add_action_or_reset(&pdev->dev, kill_defer_work, pdev);
> +	if (rc)
> +		return rc;
> +
>  	return walk_hmem_resources(&pdev->dev, hmem_register_device);
>  }

The hunk that is missing is that dax_hmem_exit() should flush the work,
and process_defer_work() should give up if the device has been unbound
before it runs. Hopefully that last suggestion does not make lockdep
unhappy about running process_defer_work under the hmem_platform
device_lock(). I *think* it should be ok and solves the TOCTOU race in
hmem_register_device() around whether we are in the pre or post initial
probe world.