* [PATCH v2] ndctl: fix libdaxctl memory leak
@ 2018-04-10 17:17 Dave Jiang
2018-04-10 17:38 ` Plewa, Lukasz
0 siblings, 1 reply; 8+ messages in thread
From: Dave Jiang @ 2018-04-10 17:17 UTC (permalink / raw)
To: vishal.l.verma, dan.j.williams; +Cc: linux-nvdimm
When daxctl_unref is releasing the context, we should make sure that the
regions and devices are also being released. free_region() will free
all the devices under the region.
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v2: Use list_for_each_safe() for region removal. (Dan)
daxctl/lib/libdaxctl.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
index 9e503201..22f4210a 100644
--- a/daxctl/lib/libdaxctl.c
+++ b/daxctl/lib/libdaxctl.c
@@ -29,6 +29,8 @@
static const char *attrs = "dax_region";
+static void free_region(struct daxctl_region *region, struct list_head *head);
+
/**
* struct daxctl_ctx - library user context to find "nd" instances
*
@@ -119,11 +121,17 @@ DAXCTL_EXPORT struct daxctl_ctx *daxctl_ref(struct daxctl_ctx *ctx)
*/
DAXCTL_EXPORT void daxctl_unref(struct daxctl_ctx *ctx)
{
+ struct daxctl_region *region, *_r;
+
if (ctx == NULL)
return;
ctx->refcount--;
if (ctx->refcount > 0)
return;
+
+ list_for_each_safe(&ctx->regions, region, _r, list)
+ free_region(region, &ctx->regions);
+
info(ctx, "context %p released\n", ctx);
free(ctx);
}
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply related [flat|nested] 8+ messages in thread
* RE: [PATCH v2] ndctl: fix libdaxctl memory leak
2018-04-10 17:17 [PATCH v2] ndctl: fix libdaxctl memory leak Dave Jiang
@ 2018-04-10 17:38 ` Plewa, Lukasz
2018-04-10 17:43 ` Dave Jiang
0 siblings, 1 reply; 8+ messages in thread
From: Plewa, Lukasz @ 2018-04-10 17:38 UTC (permalink / raw)
To: Jiang, Dave, Verma, Vishal L, Williams, Dan J; +Cc: linux-nvdimm
On Tue, Apr 10, 2018 at 7:17 PM, Dave Jiang <dave.jiang@intel.com> wrote:
> When daxctl_unref is releasing the context, we should make sure that the
> regions and devices are also being released. free_region() will free all the
> devices under the region.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>
> v2: Use list_for_each_safe() for region removal. (Dan)
>
> daxctl/lib/libdaxctl.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c index
> 9e503201..22f4210a 100644
> --- a/daxctl/lib/libdaxctl.c
> +++ b/daxctl/lib/libdaxctl.c
> @@ -29,6 +29,8 @@
>
> static const char *attrs = "dax_region";
>
> +static void free_region(struct daxctl_region *region, struct list_head
> +*head);
> +
> /**
> * struct daxctl_ctx - library user context to find "nd" instances
> *
> @@ -119,11 +121,17 @@ DAXCTL_EXPORT struct daxctl_ctx
> *daxctl_ref(struct daxctl_ctx *ctx)
> */
> DAXCTL_EXPORT void daxctl_unref(struct daxctl_ctx *ctx) {
> + struct daxctl_region *region, *_r;
> +
> if (ctx == NULL)
> return;
> ctx->refcount--;
> if (ctx->refcount > 0)
> return;
> +
> + list_for_each_safe(&ctx->regions, region, _r, list)
> + free_region(region, &ctx->regions);
> +
> info(ctx, "context %p released\n", ctx);
> free(ctx);
> }
As daxctl_region has its own refcount, you need daxctl_ref() in add_dax_region() and daxctl_unref() in free_region().( or daxctl_ref() in daxctl_region_ref() and daxctl_unref() in daxctl_region_unref())
Without it, this code will cause segfault:
daxctl_new(&ctx);
region = daxctl_new_region(...);
daxctl_region_ref(region);
daxctl_unref(ctx);
daxctl_region_unref(region);
Łukasz
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] ndctl: fix libdaxctl memory leak
2018-04-10 17:38 ` Plewa, Lukasz
@ 2018-04-10 17:43 ` Dave Jiang
2018-04-13 2:30 ` Verma, Vishal L
0 siblings, 1 reply; 8+ messages in thread
From: Dave Jiang @ 2018-04-10 17:43 UTC (permalink / raw)
To: Plewa, Lukasz, Verma, Vishal L, Williams, Dan J; +Cc: linux-nvdimm
On 04/10/2018 10:38 AM, Plewa, Lukasz wrote:
> On Tue, Apr 10, 2018 at 7:17 PM, Dave Jiang <dave.jiang@intel.com> wrote:
>> When daxctl_unref is releasing the context, we should make sure that the
>> regions and devices are also being released. free_region() will free all the
>> devices under the region.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>
>> v2: Use list_for_each_safe() for region removal. (Dan)
>>
>> daxctl/lib/libdaxctl.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c index
>> 9e503201..22f4210a 100644
>> --- a/daxctl/lib/libdaxctl.c
>> +++ b/daxctl/lib/libdaxctl.c
>> @@ -29,6 +29,8 @@
>>
>> static const char *attrs = "dax_region";
>>
>> +static void free_region(struct daxctl_region *region, struct list_head
>> +*head);
>> +
>> /**
>> * struct daxctl_ctx - library user context to find "nd" instances
>> *
>> @@ -119,11 +121,17 @@ DAXCTL_EXPORT struct daxctl_ctx
>> *daxctl_ref(struct daxctl_ctx *ctx)
>> */
>> DAXCTL_EXPORT void daxctl_unref(struct daxctl_ctx *ctx) {
>> + struct daxctl_region *region, *_r;
>> +
>> if (ctx == NULL)
>> return;
>> ctx->refcount--;
>> if (ctx->refcount > 0)
>> return;
>> +
>> + list_for_each_safe(&ctx->regions, region, _r, list)
>> + free_region(region, &ctx->regions);
>> +
>> info(ctx, "context %p released\n", ctx);
>> free(ctx);
>> }
>
> As daxctl_region has its own refcount, you need daxctl_ref() in add_dax_region() and daxctl_unref() in free_region().( or daxctl_ref() in daxctl_region_ref() and daxctl_unref() in daxctl_region_unref())
>
> Without it, this code will cause segfault:
>
> daxctl_new(&ctx);
> region = daxctl_new_region(...);
> daxctl_region_ref(region);
> daxctl_unref(ctx);
> daxctl_region_unref(region);
Shouldn't it go in this order:
daxctl_region_unref(region);
daxctl_unref(ctx);
In this case, you won't segfault right? I think ctx should be the very
last thing you free.
>
> Łukasz
>
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] ndctl: fix libdaxctl memory leak
2018-04-10 17:43 ` Dave Jiang
@ 2018-04-13 2:30 ` Verma, Vishal L
2018-04-13 2:39 ` Jiang, Dave
0 siblings, 1 reply; 8+ messages in thread
From: Verma, Vishal L @ 2018-04-13 2:30 UTC (permalink / raw)
To: Williams, Dan J, Jiang, Dave, Plewa, Lukasz; +Cc: linux-nvdimm
On Tue, 2018-04-10 at 10:43 -0700, Dave Jiang wrote:
>
> On 04/10/2018 10:38 AM, Plewa, Lukasz wrote:
> > On Tue, Apr 10, 2018 at 7:17 PM, Dave Jiang <dave.jiang@intel.com>
> > wrote:
> > > When daxctl_unref is releasing the context, we should make sure that
> > > the
> > > regions and devices are also being released. free_region() will free
> > > all the
> > > devices under the region.
> > >
> > > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> > > ---
> > >
> > > v2: Use list_for_each_safe() for region removal. (Dan)
> > >
> > > daxctl/lib/libdaxctl.c | 8 ++++++++
> > > 1 file changed, 8 insertions(+)
> > >
> > > diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c index
> > > 9e503201..22f4210a 100644
> > > --- a/daxctl/lib/libdaxctl.c
> > > +++ b/daxctl/lib/libdaxctl.c
> > > @@ -29,6 +29,8 @@
> > >
> > > static const char *attrs = "dax_region";
> > >
> > > +static void free_region(struct daxctl_region *region, struct
> > > list_head
> > > +*head);
> > > +
> > > /**
> > > * struct daxctl_ctx - library user context to find "nd" instances
> > > *
> > > @@ -119,11 +121,17 @@ DAXCTL_EXPORT struct daxctl_ctx
> > > *daxctl_ref(struct daxctl_ctx *ctx)
> > > */
> > > DAXCTL_EXPORT void daxctl_unref(struct daxctl_ctx *ctx) {
> > > + struct daxctl_region *region, *_r;
> > > +
> > > if (ctx == NULL)
> > > return;
> > > ctx->refcount--;
> > > if (ctx->refcount > 0)
> > > return;
> > > +
> > > + list_for_each_safe(&ctx->regions, region, _r, list)
> > > + free_region(region, &ctx->regions);
> > > +
> > > info(ctx, "context %p released\n", ctx);
> > > free(ctx);
> > > }
> >
> > As daxctl_region has its own refcount, you need daxctl_ref() in
> > add_dax_region() and daxctl_unref() in free_region().( or daxctl_ref()
> > in daxctl_region_ref() and daxctl_unref() in daxctl_region_unref())
> >
> > Without it, this code will cause segfault:
> >
> > daxctl_new(&ctx);
> > region = daxctl_new_region(...);
> > daxctl_region_ref(region);
> > daxctl_unref(ctx);
> > daxctl_region_unref(region);
>
> Shouldn't it go in this order:
>
> daxctl_region_unref(region);
> daxctl_unref(ctx);
>
> In this case, you won't segfault right? I think ctx should be the very
> last thing you free.
Hey Dave, does this need a v3 then?
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] ndctl: fix libdaxctl memory leak
2018-04-13 2:30 ` Verma, Vishal L
@ 2018-04-13 2:39 ` Jiang, Dave
2018-04-13 6:57 ` Plewa, Lukasz
0 siblings, 1 reply; 8+ messages in thread
From: Jiang, Dave @ 2018-04-13 2:39 UTC (permalink / raw)
To: Verma, Vishal L; +Cc: linux-nvdimm
> On Apr 12, 2018, at 7:30 PM, Verma, Vishal L <vishal.l.verma@intel.com> wrote:
>
>> On Tue, 2018-04-10 at 10:43 -0700, Dave Jiang wrote:
>>
>>> On 04/10/2018 10:38 AM, Plewa, Lukasz wrote:
>>> On Tue, Apr 10, 2018 at 7:17 PM, Dave Jiang <dave.jiang@intel.com>
>>> wrote:
>>>> When daxctl_unref is releasing the context, we should make sure that
>>>> the
>>>> regions and devices are also being released. free_region() will free
>>>> all the
>>>> devices under the region.
>>>>
>>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>>> ---
>>>>
>>>> v2: Use list_for_each_safe() for region removal. (Dan)
>>>>
>>>> daxctl/lib/libdaxctl.c | 8 ++++++++
>>>> 1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c index
>>>> 9e503201..22f4210a 100644
>>>> --- a/daxctl/lib/libdaxctl.c
>>>> +++ b/daxctl/lib/libdaxctl.c
>>>> @@ -29,6 +29,8 @@
>>>>
>>>> static const char *attrs = "dax_region";
>>>>
>>>> +static void free_region(struct daxctl_region *region, struct
>>>> list_head
>>>> +*head);
>>>> +
>>>> /**
>>>> * struct daxctl_ctx - library user context to find "nd" instances
>>>> *
>>>> @@ -119,11 +121,17 @@ DAXCTL_EXPORT struct daxctl_ctx
>>>> *daxctl_ref(struct daxctl_ctx *ctx)
>>>> */
>>>> DAXCTL_EXPORT void daxctl_unref(struct daxctl_ctx *ctx) {
>>>> + struct daxctl_region *region, *_r;
>>>> +
>>>> if (ctx == NULL)
>>>> return;
>>>> ctx->refcount--;
>>>> if (ctx->refcount > 0)
>>>> return;
>>>> +
>>>> + list_for_each_safe(&ctx->regions, region, _r, list)
>>>> + free_region(region, &ctx->regions);
>>>> +
>>>> info(ctx, "context %p released\n", ctx);
>>>> free(ctx);
>>>> }
>>>
>>> As daxctl_region has its own refcount, you need daxctl_ref() in
>>> add_dax_region() and daxctl_unref() in free_region().( or daxctl_ref()
>>> in daxctl_region_ref() and daxctl_unref() in daxctl_region_unref())
>>>
>>> Without it, this code will cause segfault:
>>>
>>> daxctl_new(&ctx);
>>> region = daxctl_new_region(...);
>>> daxctl_region_ref(region);
>>> daxctl_unref(ctx);
>>> daxctl_region_unref(region);
>>
>> Shouldn't it go in this order:
>>
>> daxctl_region_unref(region);
>> daxctl_unref(ctx);
>>
>> In this case, you won't segfault right? I think ctx should be the very
>> last thing you free.
>
> Hey Dave, does this need a v3 then?
>
Depends on Lukasz? I don’t think it’s needed if the contexts are freed in the right order.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v2] ndctl: fix libdaxctl memory leak
2018-04-13 2:39 ` Jiang, Dave
@ 2018-04-13 6:57 ` Plewa, Lukasz
2018-04-13 15:09 ` Dave Jiang
0 siblings, 1 reply; 8+ messages in thread
From: Plewa, Lukasz @ 2018-04-13 6:57 UTC (permalink / raw)
To: Jiang, Dave, Verma, Vishal L; +Cc: linux-nvdimm
On Apr 13, 2018, at 4:40AM, Dave Jiang wrote:
> > On Apr 12, 2018, at 7:30 PM, Verma, Vishal L <vishal.l.verma@intel.com>
> wrote:
> >
> >> On Tue, 2018-04-10 at 10:43 -0700, Dave Jiang wrote:
> >>
> >>> On 04/10/2018 10:38 AM, Plewa, Lukasz wrote:
> >>> On Tue, Apr 10, 2018 at 7:17 PM, Dave Jiang <dave.jiang@intel.com>
> >>> wrote:
> >>>> When daxctl_unref is releasing the context, we should make sure
> >>>> that the regions and devices are also being released. free_region()
> >>>> will free all the devices under the region.
> >>>>
> >>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> >>>> ---
> >>>>
> >>>> v2: Use list_for_each_safe() for region removal. (Dan)
> >>>>
> >>>> daxctl/lib/libdaxctl.c | 8 ++++++++
> >>>> 1 file changed, 8 insertions(+)
> >>>>
> >>>> diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c index
> >>>> 9e503201..22f4210a 100644
> >>>> --- a/daxctl/lib/libdaxctl.c
> >>>> +++ b/daxctl/lib/libdaxctl.c
> >>>> @@ -29,6 +29,8 @@
> >>>>
> >>>> static const char *attrs = "dax_region";
> >>>>
> >>>> +static void free_region(struct daxctl_region *region, struct
> >>>> list_head
> >>>> +*head);
> >>>> +
> >>>> /**
> >>>> * struct daxctl_ctx - library user context to find "nd" instances
> >>>> *
> >>>> @@ -119,11 +121,17 @@ DAXCTL_EXPORT struct daxctl_ctx
> >>>> *daxctl_ref(struct daxctl_ctx *ctx) */ DAXCTL_EXPORT void
> >>>> daxctl_unref(struct daxctl_ctx *ctx) {
> >>>> + struct daxctl_region *region, *_r;
> >>>> +
> >>>> if (ctx == NULL)
> >>>> return;
> >>>> ctx->refcount--;
> >>>> if (ctx->refcount > 0)
> >>>> return;
> >>>> +
> >>>> + list_for_each_safe(&ctx->regions, region, _r, list)
> >>>> + free_region(region, &ctx->regions);
> >>>> +
> >>>> info(ctx, "context %p released\n", ctx);
> >>>> free(ctx);
> >>>> }
> >>>
> >>> As daxctl_region has its own refcount, you need daxctl_ref() in
> >>> add_dax_region() and daxctl_unref() in free_region().( or
> >>> daxctl_ref() in daxctl_region_ref() and daxctl_unref() in
> >>> daxctl_region_unref())
> >>>
> >>> Without it, this code will cause segfault:
> >>>
> >>> daxctl_new(&ctx);
> >>> region = daxctl_new_region(...);
> >>> daxctl_region_ref(region);
> >>> daxctl_unref(ctx);
> >>> daxctl_region_unref(region);
> >>
> >> Shouldn't it go in this order:
> >>
> >> daxctl_region_unref(region);
> >> daxctl_unref(ctx);
> >>
> >> In this case, you won't segfault right? I think ctx should be the
> >> very last thing you free.
> >
> > Hey Dave, does this need a v3 then?
> >
>
> Depends on Lukasz? I don't think it's needed if the contexts are freed in the
> right order.
Sorry for late reply, I need only memleak fixed.
Lukasz
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] ndctl: fix libdaxctl memory leak
2018-04-13 6:57 ` Plewa, Lukasz
@ 2018-04-13 15:09 ` Dave Jiang
2018-04-16 7:37 ` Plewa, Lukasz
0 siblings, 1 reply; 8+ messages in thread
From: Dave Jiang @ 2018-04-13 15:09 UTC (permalink / raw)
To: Plewa, Lukasz, Verma, Vishal L; +Cc: linux-nvdimm
On 4/12/2018 11:57 PM, Plewa, Lukasz wrote:
> On Apr 13, 2018, at 4:40AM, Dave Jiang wrote:
>>> On Apr 12, 2018, at 7:30 PM, Verma, Vishal L <vishal.l.verma@intel.com>
>> wrote:
>>>> On Tue, 2018-04-10 at 10:43 -0700, Dave Jiang wrote:
>>>>
>>>>> On 04/10/2018 10:38 AM, Plewa, Lukasz wrote:
>>>>> On Tue, Apr 10, 2018 at 7:17 PM, Dave Jiang <dave.jiang@intel.com>
>>>>> wrote:
>>>>>> When daxctl_unref is releasing the context, we should make sure
>>>>>> that the regions and devices are also being released. free_region()
>>>>>> will free all the devices under the region.
>>>>>>
>>>>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>>>>> ---
>>>>>>
>>>>>> v2: Use list_for_each_safe() for region removal. (Dan)
>>>>>>
>>>>>> daxctl/lib/libdaxctl.c | 8 ++++++++
>>>>>> 1 file changed, 8 insertions(+)
>>>>>>
>>>>>> diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c index
>>>>>> 9e503201..22f4210a 100644
>>>>>> --- a/daxctl/lib/libdaxctl.c
>>>>>> +++ b/daxctl/lib/libdaxctl.c
>>>>>> @@ -29,6 +29,8 @@
>>>>>>
>>>>>> static const char *attrs = "dax_region";
>>>>>>
>>>>>> +static void free_region(struct daxctl_region *region, struct
>>>>>> list_head
>>>>>> +*head);
>>>>>> +
>>>>>> /**
>>>>>> * struct daxctl_ctx - library user context to find "nd" instances
>>>>>> *
>>>>>> @@ -119,11 +121,17 @@ DAXCTL_EXPORT struct daxctl_ctx
>>>>>> *daxctl_ref(struct daxctl_ctx *ctx) */ DAXCTL_EXPORT void
>>>>>> daxctl_unref(struct daxctl_ctx *ctx) {
>>>>>> + struct daxctl_region *region, *_r;
>>>>>> +
>>>>>> if (ctx == NULL)
>>>>>> return;
>>>>>> ctx->refcount--;
>>>>>> if (ctx->refcount > 0)
>>>>>> return;
>>>>>> +
>>>>>> + list_for_each_safe(&ctx->regions, region, _r, list)
>>>>>> + free_region(region, &ctx->regions);
>>>>>> +
>>>>>> info(ctx, "context %p released\n", ctx);
>>>>>> free(ctx);
>>>>>> }
>>>>> As daxctl_region has its own refcount, you need daxctl_ref() in
>>>>> add_dax_region() and daxctl_unref() in free_region().( or
>>>>> daxctl_ref() in daxctl_region_ref() and daxctl_unref() in
>>>>> daxctl_region_unref())
>>>>>
>>>>> Without it, this code will cause segfault:
>>>>>
>>>>> daxctl_new(&ctx);
>>>>> region = daxctl_new_region(...);
>>>>> daxctl_region_ref(region);
>>>>> daxctl_unref(ctx);
>>>>> daxctl_region_unref(region);
>>>> Shouldn't it go in this order:
>>>>
>>>> daxctl_region_unref(region);
>>>> daxctl_unref(ctx);
>>>>
>>>> In this case, you won't segfault right? I think ctx should be the
>>>> very last thing you free.
>>> Hey Dave, does this need a v3 then?
>>>
>> Depends on Lukasz? I don't think it's needed if the contexts are freed in the
>> right order.
> Sorry for late reply, I need only memleak fixed.
Does the patch fix your memory leak if you call the unref in the right
order?
>
> Lukasz
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v2] ndctl: fix libdaxctl memory leak
2018-04-13 15:09 ` Dave Jiang
@ 2018-04-16 7:37 ` Plewa, Lukasz
0 siblings, 0 replies; 8+ messages in thread
From: Plewa, Lukasz @ 2018-04-16 7:37 UTC (permalink / raw)
To: Jiang, Dave, Verma, Vishal L; +Cc: linux-nvdimm
On Apr 13, 2018, at 5:09PM, Dave Jiang wrote:
> On 4/12/2018 11:57 PM, Plewa, Lukasz wrote:
> > On Apr 13, 2018, at 4:40AM, Dave Jiang wrote:
> >>> On Apr 12, 2018, at 7:30 PM, Verma, Vishal L
> >>> <vishal.l.verma@intel.com>
> >> wrote:
> >>>> On Tue, 2018-04-10 at 10:43 -0700, Dave Jiang wrote:
> >>>>
> >>>>> On 04/10/2018 10:38 AM, Plewa, Lukasz wrote:
> >>>>> On Tue, Apr 10, 2018 at 7:17 PM, Dave Jiang <dave.jiang@intel.com>
> >>>>> wrote:
> >>>>>> When daxctl_unref is releasing the context, we should make sure
> >>>>>> that the regions and devices are also being released.
> >>>>>> free_region() will free all the devices under the region.
> >>>>>>
> >>>>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> >>>>>> ---
> >>>>>>
> >>>>>> v2: Use list_for_each_safe() for region removal. (Dan)
> >>>>>>
> >>>>>> daxctl/lib/libdaxctl.c | 8 ++++++++
> >>>>>> 1 file changed, 8 insertions(+)
> >>>>>>
> >>>>>> diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
> >>>>>> index 9e503201..22f4210a 100644
> >>>>>> --- a/daxctl/lib/libdaxctl.c
> >>>>>> +++ b/daxctl/lib/libdaxctl.c
> >>>>>> @@ -29,6 +29,8 @@
> >>>>>>
> >>>>>> static const char *attrs = "dax_region";
> >>>>>>
> >>>>>> +static void free_region(struct daxctl_region *region, struct
> >>>>>> list_head
> >>>>>> +*head);
> >>>>>> +
> >>>>>> /**
> >>>>>> * struct daxctl_ctx - library user context to find "nd" instances
> >>>>>> *
> >>>>>> @@ -119,11 +121,17 @@ DAXCTL_EXPORT struct daxctl_ctx
> >>>>>> *daxctl_ref(struct daxctl_ctx *ctx) */ DAXCTL_EXPORT void
> >>>>>> daxctl_unref(struct daxctl_ctx *ctx) {
> >>>>>> + struct daxctl_region *region, *_r;
> >>>>>> +
> >>>>>> if (ctx == NULL)
> >>>>>> return;
> >>>>>> ctx->refcount--;
> >>>>>> if (ctx->refcount > 0)
> >>>>>> return;
> >>>>>> +
> >>>>>> + list_for_each_safe(&ctx->regions, region, _r, list)
> >>>>>> + free_region(region, &ctx->regions);
> >>>>>> +
> >>>>>> info(ctx, "context %p released\n", ctx);
> >>>>>> free(ctx);
> >>>>>> }
> >>>>> As daxctl_region has its own refcount, you need daxctl_ref() in
> >>>>> add_dax_region() and daxctl_unref() in free_region().( or
> >>>>> daxctl_ref() in daxctl_region_ref() and daxctl_unref() in
> >>>>> daxctl_region_unref())
> >>>>>
> >>>>> Without it, this code will cause segfault:
> >>>>>
> >>>>> daxctl_new(&ctx);
> >>>>> region = daxctl_new_region(...);
> >>>>> daxctl_region_ref(region);
> >>>>> daxctl_unref(ctx);
> >>>>> daxctl_region_unref(region);
> >>>> Shouldn't it go in this order:
> >>>>
> >>>> daxctl_region_unref(region);
> >>>> daxctl_unref(ctx);
> >>>>
> >>>> In this case, you won't segfault right? I think ctx should be the
> >>>> very last thing you free.
> >>> Hey Dave, does this need a v3 then?
> >>>
> >> Depends on Lukasz? I don't think it's needed if the contexts are
> >> freed in the right order.
> > Sorry for late reply, I need only memleak fixed.
>
> Does the patch fix your memory leak if you call the unref in the right order?
Yes it is, thanks.
> >
> > Lukasz
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-04-16 7:37 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-10 17:17 [PATCH v2] ndctl: fix libdaxctl memory leak Dave Jiang
2018-04-10 17:38 ` Plewa, Lukasz
2018-04-10 17:43 ` Dave Jiang
2018-04-13 2:30 ` Verma, Vishal L
2018-04-13 2:39 ` Jiang, Dave
2018-04-13 6:57 ` Plewa, Lukasz
2018-04-13 15:09 ` Dave Jiang
2018-04-16 7:37 ` Plewa, Lukasz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.