* Re: [PATCH] cxl/region: Convert cxl_pmem_region_alloc to scope-based resource management
2024-04-30 21:59 [PATCH] cxl/region: Convert cxl_pmem_region_alloc to scope-based resource management Dan Williams
@ 2024-04-30 22:50 ` fan
2024-05-01 12:02 ` Jonathan Cameron
2024-05-09 7:04 ` Zhijian Li (Fujitsu)
2 siblings, 0 replies; 5+ messages in thread
From: fan @ 2024-04-30 22:50 UTC (permalink / raw)
To: Dan Williams; +Cc: dave.jiang, Li Zhijian, Jonathan Cameron, linux-cxl
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] cxl/region: Convert cxl_pmem_region_alloc to scope-based resource management
2024-04-30 21:59 [PATCH] cxl/region: Convert cxl_pmem_region_alloc to scope-based resource management Dan Williams
2024-04-30 22:50 ` fan
@ 2024-05-01 12:02 ` Jonathan Cameron
2024-05-01 23:27 ` Dan Williams
2024-05-09 7:04 ` Zhijian Li (Fujitsu)
2 siblings, 1 reply; 5+ messages in thread
From: Jonathan Cameron @ 2024-05-01 12:02 UTC (permalink / raw)
To: Dan Williams; +Cc: dave.jiang, Li Zhijian, linux-cxl
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;
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] cxl/region: Convert cxl_pmem_region_alloc to scope-based resource management
2024-05-01 12:02 ` Jonathan Cameron
@ 2024-05-01 23:27 ` Dan Williams
0 siblings, 0 replies; 5+ messages in thread
From: Dan Williams @ 2024-05-01 23:27 UTC (permalink / raw)
To: Jonathan Cameron, Dan Williams; +Cc: dave.jiang, Li Zhijian, linux-cxl
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cxl/region: Convert cxl_pmem_region_alloc to scope-based resource management
2024-04-30 21:59 [PATCH] cxl/region: Convert cxl_pmem_region_alloc to scope-based resource management Dan Williams
2024-04-30 22:50 ` fan
2024-05-01 12:02 ` Jonathan Cameron
@ 2024-05-09 7:04 ` Zhijian Li (Fujitsu)
2 siblings, 0 replies; 5+ messages in thread
From: Zhijian Li (Fujitsu) @ 2024-05-09 7:04 UTC (permalink / raw)
To: Dan Williams, dave.jiang@intel.com
Cc: Jonathan Cameron, linux-cxl@vger.kernel.org
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;
>
^ permalink raw reply [flat|nested] 5+ messages in thread