Message ID | 171451430965.1147997.15782562063090960666.stgit@dwillia2-xfh.jf.intel.com |
---|---|
State | New |
Headers | show |
Series | cxl/region: Convert cxl_pmem_region_alloc to scope-based resource management | expand |
On Tue, Apr 30, 2024 at 02:59:00PM -0700, Dan Williams wrote: > A recent bugfix to cxl_pmem_region_alloc() to fix an > error-unwind-memleak [1], highlighted a use case for scope-based resource > management. > > Delete the goto for releasing @cxl_region_rwsem, and return error codes > directly from error condition paths. > > The caller, devm_cxl_add_pmem_region(), is no longer given @cxlr_pmem > directly it must retrieve it from @cxlr->cxlr_pmem. This retrieval from > @cxlr was already in place for @cxlr->cxl_nvb, and converting > cxl_pmem_region_alloc() to return an int makes it less awkward to handle > no_free_ptr(). > > Cc: Li Zhijian <lizhijian@fujitsu.com> > Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > Closes: http://lore.kernel.org/r/20240430174540.000039ce@Huawei.com > Link: http://lore.kernel.org/r/20240428030748.318985-1-lizhijian@fujitsu.com > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > Hey Dave, this applies on top of Li's fix. > > drivers/cxl/core/region.c | 43 +++++++++++++++++-------------------------- > 1 file changed, 17 insertions(+), 26 deletions(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 812b2948b6c6..e0577d99c4db 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -2681,26 +2681,21 @@ int cxl_get_poison_by_endpoint(struct cxl_port *port) > > static struct lock_class_key cxl_pmem_region_key; > > -static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr) > +static int cxl_pmem_region_alloc(struct cxl_region *cxlr) > { > struct cxl_region_params *p = &cxlr->params; > struct cxl_nvdimm_bridge *cxl_nvb; > - struct cxl_pmem_region *cxlr_pmem; > struct device *dev; > int i; > > - down_read(&cxl_region_rwsem); > - if (p->state != CXL_CONFIG_COMMIT) { > - cxlr_pmem = ERR_PTR(-ENXIO); > - goto out; > - } > + guard(rwsem_read)(&cxl_region_rwsem); > + if (p->state != CXL_CONFIG_COMMIT) > + return -ENXIO; > > - cxlr_pmem = kzalloc(struct_size(cxlr_pmem, mapping, p->nr_targets), > - GFP_KERNEL); > - if (!cxlr_pmem) { > - cxlr_pmem = ERR_PTR(-ENOMEM); > - goto out; > - } > + struct cxl_pmem_region *cxlr_pmem __free(kfree) = kzalloc( > + struct_size(cxlr_pmem, mapping, p->nr_targets), GFP_KERNEL); > + if (!cxlr_pmem) > + return -ENOMEM; > > cxlr_pmem->hpa_range.start = p->res->start; > cxlr_pmem->hpa_range.end = p->res->end; > @@ -2718,11 +2713,8 @@ static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr) > */ > if (i == 0) { > cxl_nvb = cxl_find_nvdimm_bridge(cxlmd); > - if (!cxl_nvb) { > - kfree(cxlr_pmem); > - cxlr_pmem = ERR_PTR(-ENODEV); > - goto out; > - } > + if (!cxl_nvb) > + return -ENODEV; > cxlr->cxl_nvb = cxl_nvb; > } > m->cxlmd = cxlmd; > @@ -2733,18 +2725,16 @@ static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr) > } > > dev = &cxlr_pmem->dev; > - cxlr_pmem->cxlr = cxlr; > - cxlr->cxlr_pmem = cxlr_pmem; > device_initialize(dev); > lockdep_set_class(&dev->mutex, &cxl_pmem_region_key); > device_set_pm_not_required(dev); > dev->parent = &cxlr->dev; > dev->bus = &cxl_bus_type; > dev->type = &cxl_pmem_region_type; > -out: > - up_read(&cxl_region_rwsem); > + cxlr_pmem->cxlr = cxlr; > + cxlr->cxlr_pmem = no_free_ptr(cxlr_pmem); > > - return cxlr_pmem; > + return 0; > } > > static void cxl_dax_region_release(struct device *dev) > @@ -2861,9 +2851,10 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr) > struct device *dev; > int rc; > > - cxlr_pmem = cxl_pmem_region_alloc(cxlr); > - if (IS_ERR(cxlr_pmem)) > - return PTR_ERR(cxlr_pmem); > + rc = cxl_pmem_region_alloc(cxlr); > + if (rc) > + return rc; > + cxlr_pmem = cxlr->cxlr_pmem; > cxl_nvb = cxlr->cxl_nvb; > > dev = &cxlr_pmem->dev; > LGTM. Fan
On Tue, 30 Apr 2024 14:59:00 -0700 Dan Williams <dan.j.williams@intel.com> wrote: > A recent bugfix to cxl_pmem_region_alloc() to fix an > error-unwind-memleak [1], highlighted a use case for scope-based resource > management. > > Delete the goto for releasing @cxl_region_rwsem, and return error codes > directly from error condition paths. > > The caller, devm_cxl_add_pmem_region(), is no longer given @cxlr_pmem > directly it must retrieve it from @cxlr->cxlr_pmem. This retrieval from > @cxlr was already in place for @cxlr->cxl_nvb, and converting > cxl_pmem_region_alloc() to return an int makes it less awkward to handle > no_free_ptr(). > > Cc: Li Zhijian <lizhijian@fujitsu.com> > Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > Closes: http://lore.kernel.org/r/20240430174540.000039ce@Huawei.com > Link: http://lore.kernel.org/r/20240428030748.318985-1-lizhijian@fujitsu.com > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Trivial comments inline Up to Dave or you on whether you want to act on them. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > Hey Dave, this applies on top of Li's fix. > > drivers/cxl/core/region.c | 43 +++++++++++++++++-------------------------- > 1 file changed, 17 insertions(+), 26 deletions(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 812b2948b6c6..e0577d99c4db 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -2681,26 +2681,21 @@ int cxl_get_poison_by_endpoint(struct cxl_port *port) > > static struct lock_class_key cxl_pmem_region_key; > > -static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr) > +static int cxl_pmem_region_alloc(struct cxl_region *cxlr) > { > struct cxl_region_params *p = &cxlr->params; > struct cxl_nvdimm_bridge *cxl_nvb; > - struct cxl_pmem_region *cxlr_pmem; > struct device *dev; > int i; > > - down_read(&cxl_region_rwsem); > - if (p->state != CXL_CONFIG_COMMIT) { > - cxlr_pmem = ERR_PTR(-ENXIO); > - goto out; > - } > + guard(rwsem_read)(&cxl_region_rwsem); > + if (p->state != CXL_CONFIG_COMMIT) > + return -ENXIO; > > - cxlr_pmem = kzalloc(struct_size(cxlr_pmem, mapping, p->nr_targets), > - GFP_KERNEL); > - if (!cxlr_pmem) { > - cxlr_pmem = ERR_PTR(-ENOMEM); > - goto out; > - } > + struct cxl_pmem_region *cxlr_pmem __free(kfree) = kzalloc( > + struct_size(cxlr_pmem, mapping, p->nr_targets), GFP_KERNEL); That is rather ugly. Maybe bring the kzalloc down onto the second line and wrap the GFP_KERNEL (or just go over 80 chars)? > + if (!cxlr_pmem) > + return -ENOMEM; > > cxlr_pmem->hpa_range.start = p->res->start; > cxlr_pmem->hpa_range.end = p->res->end; > @@ -2718,11 +2713,8 @@ static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr) > */ > if (i == 0) { > cxl_nvb = cxl_find_nvdimm_bridge(cxlmd); > - if (!cxl_nvb) { > - kfree(cxlr_pmem); > - cxlr_pmem = ERR_PTR(-ENODEV); > - goto out; > - } > + if (!cxl_nvb) > + return -ENODEV; > cxlr->cxl_nvb = cxl_nvb; > } > m->cxlmd = cxlmd; > @@ -2733,18 +2725,16 @@ static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr) > } > > dev = &cxlr_pmem->dev; > - cxlr_pmem->cxlr = cxlr; > - cxlr->cxlr_pmem = cxlr_pmem; > device_initialize(dev); > lockdep_set_class(&dev->mutex, &cxl_pmem_region_key); > device_set_pm_not_required(dev); > dev->parent = &cxlr->dev; > dev->bus = &cxl_bus_type; > dev->type = &cxl_pmem_region_type; > -out: > - up_read(&cxl_region_rwsem); > + cxlr_pmem->cxlr = cxlr; > + cxlr->cxlr_pmem = no_free_ptr(cxlr_pmem); > > - return cxlr_pmem; > + return 0; > } > > static void cxl_dax_region_release(struct device *dev) > @@ -2861,9 +2851,10 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr) > struct device *dev; > int rc; > > - cxlr_pmem = cxl_pmem_region_alloc(cxlr); > - if (IS_ERR(cxlr_pmem)) > - return PTR_ERR(cxlr_pmem); > + rc = cxl_pmem_region_alloc(cxlr); > + if (rc) > + return rc; > + cxlr_pmem = cxlr->cxlr_pmem; Is the local variable worth retaining? It's only used twice. The fact it's a clxr->cxlr* also seems rather like it could be named in a more compact fashion inside the cxl_region. Would pmemr work for instance? > cxl_nvb = cxlr->cxl_nvb; > > dev = &cxlr_pmem->dev; > >
Jonathan Cameron wrote: > On Tue, 30 Apr 2024 14:59:00 -0700 > Dan Williams <dan.j.williams@intel.com> wrote: > > > A recent bugfix to cxl_pmem_region_alloc() to fix an > > error-unwind-memleak [1], highlighted a use case for scope-based resource > > management. > > > > Delete the goto for releasing @cxl_region_rwsem, and return error codes > > directly from error condition paths. > > > > The caller, devm_cxl_add_pmem_region(), is no longer given @cxlr_pmem > > directly it must retrieve it from @cxlr->cxlr_pmem. This retrieval from > > @cxlr was already in place for @cxlr->cxl_nvb, and converting > > cxl_pmem_region_alloc() to return an int makes it less awkward to handle > > no_free_ptr(). > > > > Cc: Li Zhijian <lizhijian@fujitsu.com> > > Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > > Closes: http://lore.kernel.org/r/20240430174540.000039ce@Huawei.com > > Link: http://lore.kernel.org/r/20240428030748.318985-1-lizhijian@fujitsu.com > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > Trivial comments inline Up to Dave or you on whether you want to act on them. > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> [..] > > - cxlr_pmem = kzalloc(struct_size(cxlr_pmem, mapping, p->nr_targets), > > - GFP_KERNEL); > > - if (!cxlr_pmem) { > > - cxlr_pmem = ERR_PTR(-ENOMEM); > > - goto out; > > - } > > + struct cxl_pmem_region *cxlr_pmem __free(kfree) = kzalloc( > > + struct_size(cxlr_pmem, mapping, p->nr_targets), GFP_KERNEL); > > That is rather ugly. Maybe bring the kzalloc down onto the second line and > wrap the GFP_KERNEL (or just go over 80 chars)? I have made a conscious decision to stop caring about manually making 80 column collisions look nicer or selectively breaking the 80 column limit rule. For this drivers/cxl/ corner of the kernel, whatever the .clang-format template allows, works for me. So if you can propose a .clang-format change that makes things prettier in this case I would entertain it, but otherwise, life's too short. Along those lines it would be nice if usages of __free caused the automatic line break to happen after the "=" sign rather than later at the arguments to kzalloc() like the above. However, I don't know the template incantations to achieve that. [..] > > @@ -2861,9 +2851,10 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr) > > struct device *dev; > > int rc; > > > > - cxlr_pmem = cxl_pmem_region_alloc(cxlr); > > - if (IS_ERR(cxlr_pmem)) > > - return PTR_ERR(cxlr_pmem); > > + rc = cxl_pmem_region_alloc(cxlr); > > + if (rc) > > + return rc; > > + cxlr_pmem = cxlr->cxlr_pmem; > > Is the local variable worth retaining? It's only used twice. > > The fact it's a clxr->cxlr* also seems rather like it could be named > in a more compact fashion inside the cxl_region. Would pmemr > work for instance? I am ambivalent about a cxlr_pmem rename. It would need to be a separate patch to handle the 46 instances of cxlr_pmem.
On 01/05/2024 05:59, Dan Williams wrote: > A recent bugfix to cxl_pmem_region_alloc() to fix an > error-unwind-memleak [1], highlighted a use case for scope-based resource > management. > > Delete the goto for releasing @cxl_region_rwsem, and return error codes > directly from error condition paths. > > The caller, devm_cxl_add_pmem_region(), is no longer given @cxlr_pmem > directly it must retrieve it from @cxlr->cxlr_pmem. This retrieval from > @cxlr was already in place for @cxlr->cxl_nvb, and converting > cxl_pmem_region_alloc() to return an int makes it less awkward to handle > no_free_ptr(). > > Cc: Li Zhijian <lizhijian@fujitsu.com> > Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > Closes: http://lore.kernel.org/r/20240430174540.000039ce@Huawei.com > Link: http://lore.kernel.org/r/20240428030748.318985-1-lizhijian@fujitsu.com > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Thanks, LGTM Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> and Tested-by: Li Zhijian <lizhijian@fujitsu.com> > --- > Hey Dave, this applies on top of Li's fix. > > drivers/cxl/core/region.c | 43 +++++++++++++++++-------------------------- > 1 file changed, 17 insertions(+), 26 deletions(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 812b2948b6c6..e0577d99c4db 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -2681,26 +2681,21 @@ int cxl_get_poison_by_endpoint(struct cxl_port *port) > > static struct lock_class_key cxl_pmem_region_key; > > -static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr) > +static int cxl_pmem_region_alloc(struct cxl_region *cxlr) > { > struct cxl_region_params *p = &cxlr->params; > struct cxl_nvdimm_bridge *cxl_nvb; > - struct cxl_pmem_region *cxlr_pmem; > struct device *dev; > int i; > > - down_read(&cxl_region_rwsem); > - if (p->state != CXL_CONFIG_COMMIT) { > - cxlr_pmem = ERR_PTR(-ENXIO); > - goto out; > - } > + guard(rwsem_read)(&cxl_region_rwsem); > + if (p->state != CXL_CONFIG_COMMIT) > + return -ENXIO; > > - cxlr_pmem = kzalloc(struct_size(cxlr_pmem, mapping, p->nr_targets), > - GFP_KERNEL); > - if (!cxlr_pmem) { > - cxlr_pmem = ERR_PTR(-ENOMEM); > - goto out; > - } > + struct cxl_pmem_region *cxlr_pmem __free(kfree) = kzalloc( > + struct_size(cxlr_pmem, mapping, p->nr_targets), GFP_KERNEL); > + if (!cxlr_pmem) > + return -ENOMEM; > > cxlr_pmem->hpa_range.start = p->res->start; > cxlr_pmem->hpa_range.end = p->res->end; > @@ -2718,11 +2713,8 @@ static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr) > */ > if (i == 0) { > cxl_nvb = cxl_find_nvdimm_bridge(cxlmd); > - if (!cxl_nvb) { > - kfree(cxlr_pmem); > - cxlr_pmem = ERR_PTR(-ENODEV); > - goto out; > - } > + if (!cxl_nvb) > + return -ENODEV; > cxlr->cxl_nvb = cxl_nvb; > } > m->cxlmd = cxlmd; > @@ -2733,18 +2725,16 @@ static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr) > } > > dev = &cxlr_pmem->dev; > - cxlr_pmem->cxlr = cxlr; > - cxlr->cxlr_pmem = cxlr_pmem; > device_initialize(dev); > lockdep_set_class(&dev->mutex, &cxl_pmem_region_key); > device_set_pm_not_required(dev); > dev->parent = &cxlr->dev; > dev->bus = &cxl_bus_type; > dev->type = &cxl_pmem_region_type; > -out: > - up_read(&cxl_region_rwsem); > + cxlr_pmem->cxlr = cxlr; > + cxlr->cxlr_pmem = no_free_ptr(cxlr_pmem); > > - return cxlr_pmem; > + return 0; > } > > static void cxl_dax_region_release(struct device *dev) > @@ -2861,9 +2851,10 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr) > struct device *dev; > int rc; > > - cxlr_pmem = cxl_pmem_region_alloc(cxlr); > - if (IS_ERR(cxlr_pmem)) > - return PTR_ERR(cxlr_pmem); > + rc = cxl_pmem_region_alloc(cxlr); > + if (rc) > + return rc; > + cxlr_pmem = cxlr->cxlr_pmem; > cxl_nvb = cxlr->cxl_nvb; > > dev = &cxlr_pmem->dev; >
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 812b2948b6c6..e0577d99c4db 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -2681,26 +2681,21 @@ int cxl_get_poison_by_endpoint(struct cxl_port *port) static struct lock_class_key cxl_pmem_region_key; -static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr) +static int cxl_pmem_region_alloc(struct cxl_region *cxlr) { struct cxl_region_params *p = &cxlr->params; struct cxl_nvdimm_bridge *cxl_nvb; - struct cxl_pmem_region *cxlr_pmem; struct device *dev; int i; - down_read(&cxl_region_rwsem); - if (p->state != CXL_CONFIG_COMMIT) { - cxlr_pmem = ERR_PTR(-ENXIO); - goto out; - } + guard(rwsem_read)(&cxl_region_rwsem); + if (p->state != CXL_CONFIG_COMMIT) + return -ENXIO; - cxlr_pmem = kzalloc(struct_size(cxlr_pmem, mapping, p->nr_targets), - GFP_KERNEL); - if (!cxlr_pmem) { - cxlr_pmem = ERR_PTR(-ENOMEM); - goto out; - } + struct cxl_pmem_region *cxlr_pmem __free(kfree) = kzalloc( + struct_size(cxlr_pmem, mapping, p->nr_targets), GFP_KERNEL); + if (!cxlr_pmem) + return -ENOMEM; cxlr_pmem->hpa_range.start = p->res->start; cxlr_pmem->hpa_range.end = p->res->end; @@ -2718,11 +2713,8 @@ static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr) */ if (i == 0) { cxl_nvb = cxl_find_nvdimm_bridge(cxlmd); - if (!cxl_nvb) { - kfree(cxlr_pmem); - cxlr_pmem = ERR_PTR(-ENODEV); - goto out; - } + if (!cxl_nvb) + return -ENODEV; cxlr->cxl_nvb = cxl_nvb; } m->cxlmd = cxlmd; @@ -2733,18 +2725,16 @@ static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr) } dev = &cxlr_pmem->dev; - cxlr_pmem->cxlr = cxlr; - cxlr->cxlr_pmem = cxlr_pmem; device_initialize(dev); lockdep_set_class(&dev->mutex, &cxl_pmem_region_key); device_set_pm_not_required(dev); dev->parent = &cxlr->dev; dev->bus = &cxl_bus_type; dev->type = &cxl_pmem_region_type; -out: - up_read(&cxl_region_rwsem); + cxlr_pmem->cxlr = cxlr; + cxlr->cxlr_pmem = no_free_ptr(cxlr_pmem); - return cxlr_pmem; + return 0; } static void cxl_dax_region_release(struct device *dev) @@ -2861,9 +2851,10 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr) struct device *dev; int rc; - cxlr_pmem = cxl_pmem_region_alloc(cxlr); - if (IS_ERR(cxlr_pmem)) - return PTR_ERR(cxlr_pmem); + rc = cxl_pmem_region_alloc(cxlr); + if (rc) + return rc; + cxlr_pmem = cxlr->cxlr_pmem; cxl_nvb = cxlr->cxl_nvb; dev = &cxlr_pmem->dev;
A recent bugfix to cxl_pmem_region_alloc() to fix an error-unwind-memleak [1], highlighted a use case for scope-based resource management. Delete the goto for releasing @cxl_region_rwsem, and return error codes directly from error condition paths. The caller, devm_cxl_add_pmem_region(), is no longer given @cxlr_pmem directly it must retrieve it from @cxlr->cxlr_pmem. This retrieval from @cxlr was already in place for @cxlr->cxl_nvb, and converting cxl_pmem_region_alloc() to return an int makes it less awkward to handle no_free_ptr(). Cc: Li Zhijian <lizhijian@fujitsu.com> Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com> Closes: http://lore.kernel.org/r/20240430174540.000039ce@Huawei.com Link: http://lore.kernel.org/r/20240428030748.318985-1-lizhijian@fujitsu.com Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- Hey Dave, this applies on top of Li's fix. drivers/cxl/core/region.c | 43 +++++++++++++++++-------------------------- 1 file changed, 17 insertions(+), 26 deletions(-)