Re: [PATCH v7 3/7] dax/cxl, hmem: Initialize hmem early and defer dax_cxl binding

[Dan Williams]

Koralahalli Channabasappa, Smita wrote:
[..]
> > I agree with Jonathan's comments in Patch 6, using __WORK_INITIALIZER or 
> > initializing work in dax_hmem_init() and gating flush on pdev will fix 
> > the WARN — I will add both for v8. But I think the WARN is likely 
> > indicating an ordering issue here..

Yes, Jonathan is right, static initialization is also my expecation.

> > On initial boot, the Makefile ordering ensures dax_hmem_init() runs
> > before cxl_dax_region_init(), so both work items land on system_long_wq
> > in the right order and dax_hmem's deferred work is queued before 
> > dax_cxl's driver registration work.

There is nothing that guarantees that 2 work items in system_long_wq run
in submission order. Unlikely that matters given the explicit flushing.

> > On module reload which Alison is trying here I dont think, modules are 
> > loaded by Makefile order. I think dax_cxl's workqueue is calling 
> > dax_hmem_flush_work() before dax_hmem probe has had a chance to queue 
> > its work, so flush_work() flushes nothing and dax_cxl registers its 
> > driver without waiting.

Module load order does not matter after initial probe completion.

...and dax_hmem is guaranteed to always load before dax_cxl due to the
symbol dependency of dax_hmem_flush_work().

> > __WORK_INITIALIZER fixes the WARN, but doesn't fix the race I guess if 
> > we are hitting that here..
> > 
> > [   34.673051] initcall dax_hmem_init+0x0/0xff0 [dax_hmem] returned 0 
> > after 2225 usecs
> > [   34.676011] calling  cxl_dax_region_init+0x0/0xff0 [dax_cxl] @ 1059
> > 
> > These two lines indicate cxl_dax started after dax_hmem_init() returns 
> > but I dont think that guarantees dax_hmem_platform_probe() has actually 
> > run..
> > 
> > I dont know if wait_for_device_probe() in cxl_dax_region_driver_register
> > might help..
> > 
> > Thanks
> > Smita
> 
> Actually, thinking about this more..
> 
> dax_hmem_initial_probe lives in device.c (built-in) so it survives 
> module reload. On reload it's still true from the first boot. This means 
> hmem_register_device() skips the deferral path entirely..

Yes, that is the expectation.

> The problem is this bypasses the cxl_region_contains_resource() check 
> that the deferred work normally does. On first boot, 
> process_defer_work() walks each range and decides per-range: if CXL 
> covers it, skip. If not, register with HMEM. On reload, that check never 
> happens — whoever registers first via alloc_dax_region() wins, 
> regardless of whether CXL actually covers the range.

Yes, I think you have hit on a real issue. There is no point in having
dax_hmem auto-attach on driver reload. If userspace unloads the driver
it gets to keep the pieces. So that means something like this:

diff --git a/drivers/dax/hmem/hmem.c b/drivers/dax/hmem/hmem.c
index 15e462589b92..7478bc78a698 100644
--- a/drivers/dax/hmem/hmem.c
+++ b/drivers/dax/hmem/hmem.c
@@ -112,10 +112,12 @@ static int hmem_register_device(struct device *host, int target_nid,
 	    region_intersects(res->start, resource_size(res), IORESOURCE_MEM,
 			      IORES_DESC_CXL) != REGION_DISJOINT) {
 		if (!dax_hmem_initial_probe) {
-			dev_dbg(host, "deferring range to CXL: %pr\n", res);
+			dev_dbg(host, "await CXL initial probe: %pr\n", res);
 			queue_work(system_long_wq, &dax_hmem_work.work);
 			return 0;
 		}
+		dev_dbg(host, "deferring range to CXL: %pr\n", res);
+		return 0;
 	}
 
 	rc = region_intersects_soft_reserve(res->start, resource_size(res));

---

...because if userspace wants to reload the dax_hmem driver, then it
needs to pick what happens with the CXL intersection. Userspace can
always unload cxl_acpi to force everything back to dax_hmem.

Now, you might say, "but this means that if the initial probe results in
a partial result of some regions in dax_hmem and others in dax_cxl, that
state can not be recovered outside of a reboot". I think that is ok.
This mechanism is automatic best-effort workaround for bugs / missing
capabilities in the CXL driver. Module reload fidelity is out of scope.

> So if dax_cxl registers first on reload, it could claim a range that CXL 
> doesn't actually cover, and dax_hmem would lose a range it should own..

With the above change, dax_cxl always wins in the "reload" scenario iff
cxl_acpi is loaded. Otherwise dax_hmem owns all the Soft Reserved.

> I dont know if Im thinking through this right..

You definitely identified the need for that fixup above.