Skip to content

Commit d9a476c

Browse files
weiny2davejiang
authored andcommitted
cxl/region: Remove lock from memory notifier callback
In testing Dynamic Capacity Device (DCD) support, a lockdep splat revealed an ABBA issue between the memory notifiers and the DCD extent processing code.[0] Changing the lock ordering within DCD proved difficult because regions must be stable while searching for the proper region and then the device lock must be held to properly notify the DAX region driver of memory changes. Dan points out in the thread that notifiers should be able to trust that it is safe to access static data. Region data is static once the device is realized and until it's destruction. Thus it is better to manage the notifiers within the region driver. Remove the need for a lock by ensuring the notifiers are active only during the region's lifetime. Furthermore, remove cxl_region_nid() because resource can't be NULL while the region is stable. Link: https://lore.kernel.org/all/[email protected]/ [0] Cc: Ying Huang <[email protected]> Suggested-by: Dan Williams <[email protected]> Reviewed-by: Davidlohr Bueso <[email protected]> Reviewed-by: Jonathan Cameron <[email protected]> Reviewed-by: Ying Huang <[email protected]> Signed-off-by: Ira Weiny <[email protected]> Link: https://patch.msgid.link/[email protected] Signed-off-by: Dave Jiang <[email protected]>
1 parent 3f9e075 commit d9a476c

File tree

1 file changed

+30
-24
lines changed

1 file changed

+30
-24
lines changed

drivers/cxl/core/region.c

Lines changed: 30 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -2313,8 +2313,6 @@ static void unregister_region(void *_cxlr)
23132313
struct cxl_region_params *p = &cxlr->params;
23142314
int i;
23152315

2316-
unregister_memory_notifier(&cxlr->memory_notifier);
2317-
unregister_mt_adistance_algorithm(&cxlr->adist_notifier);
23182316
device_del(&cxlr->dev);
23192317

23202318
/*
@@ -2391,18 +2389,6 @@ static bool cxl_region_update_coordinates(struct cxl_region *cxlr, int nid)
23912389
return true;
23922390
}
23932391

2394-
static int cxl_region_nid(struct cxl_region *cxlr)
2395-
{
2396-
struct cxl_region_params *p = &cxlr->params;
2397-
struct resource *res;
2398-
2399-
guard(rwsem_read)(&cxl_region_rwsem);
2400-
res = p->res;
2401-
if (!res)
2402-
return NUMA_NO_NODE;
2403-
return phys_to_target_node(res->start);
2404-
}
2405-
24062392
static int cxl_region_perf_attrs_callback(struct notifier_block *nb,
24072393
unsigned long action, void *arg)
24082394
{
@@ -2415,7 +2401,11 @@ static int cxl_region_perf_attrs_callback(struct notifier_block *nb,
24152401
if (nid == NUMA_NO_NODE || action != MEM_ONLINE)
24162402
return NOTIFY_DONE;
24172403

2418-
region_nid = cxl_region_nid(cxlr);
2404+
/*
2405+
* No need to hold cxl_region_rwsem; region parameters are stable
2406+
* within the cxl_region driver.
2407+
*/
2408+
region_nid = phys_to_target_node(cxlr->params.res->start);
24192409
if (nid != region_nid)
24202410
return NOTIFY_DONE;
24212411

@@ -2434,7 +2424,11 @@ static int cxl_region_calculate_adistance(struct notifier_block *nb,
24342424
int *adist = data;
24352425
int region_nid;
24362426

2437-
region_nid = cxl_region_nid(cxlr);
2427+
/*
2428+
* No need to hold cxl_region_rwsem; region parameters are stable
2429+
* within the cxl_region driver.
2430+
*/
2431+
region_nid = phys_to_target_node(cxlr->params.res->start);
24382432
if (nid != region_nid)
24392433
return NOTIFY_OK;
24402434

@@ -2484,14 +2478,6 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
24842478
if (rc)
24852479
goto err;
24862480

2487-
cxlr->memory_notifier.notifier_call = cxl_region_perf_attrs_callback;
2488-
cxlr->memory_notifier.priority = CXL_CALLBACK_PRI;
2489-
register_memory_notifier(&cxlr->memory_notifier);
2490-
2491-
cxlr->adist_notifier.notifier_call = cxl_region_calculate_adistance;
2492-
cxlr->adist_notifier.priority = 100;
2493-
register_mt_adistance_algorithm(&cxlr->adist_notifier);
2494-
24952481
rc = devm_add_action_or_reset(port->uport_dev, unregister_region, cxlr);
24962482
if (rc)
24972483
return ERR_PTR(rc);
@@ -3387,6 +3373,14 @@ static int is_system_ram(struct resource *res, void *arg)
33873373
return 1;
33883374
}
33893375

3376+
static void shutdown_notifiers(void *_cxlr)
3377+
{
3378+
struct cxl_region *cxlr = _cxlr;
3379+
3380+
unregister_memory_notifier(&cxlr->memory_notifier);
3381+
unregister_mt_adistance_algorithm(&cxlr->adist_notifier);
3382+
}
3383+
33903384
static int cxl_region_probe(struct device *dev)
33913385
{
33923386
struct cxl_region *cxlr = to_cxl_region(dev);
@@ -3419,6 +3413,18 @@ static int cxl_region_probe(struct device *dev)
34193413
out:
34203414
up_read(&cxl_region_rwsem);
34213415

3416+
if (rc)
3417+
return rc;
3418+
3419+
cxlr->memory_notifier.notifier_call = cxl_region_perf_attrs_callback;
3420+
cxlr->memory_notifier.priority = CXL_CALLBACK_PRI;
3421+
register_memory_notifier(&cxlr->memory_notifier);
3422+
3423+
cxlr->adist_notifier.notifier_call = cxl_region_calculate_adistance;
3424+
cxlr->adist_notifier.priority = 100;
3425+
register_mt_adistance_algorithm(&cxlr->adist_notifier);
3426+
3427+
rc = devm_add_action_or_reset(&cxlr->dev, shutdown_notifiers, cxlr);
34223428
if (rc)
34233429
return rc;
34243430

0 commit comments

Comments
 (0)