subject:"Re\: \[PATCH v2\] ndctl\: fix libdaxctl memory leak"

RE: [PATCH v2] ndctl: fix libdaxctl memory leak

2018-04-16 Thread Plewa, Lukasz

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
> >>> 
> >> 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 
> > 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 
> >> ---
> >>
> >> 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(>regions, region, _r, list)
> >> +free_region(region, >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();
> > 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

Re: [PATCH v2] ndctl: fix libdaxctl memory leak

2018-04-13 Thread Dave Jiang



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 

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 
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 
---

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(>regions, region, _r, list)
+free_region(region, >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();
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

RE: [PATCH v2] ndctl: fix libdaxctl memory leak

2018-04-13 Thread Plewa, Lukasz

On Apr 13, 2018, at 4:40AM, Dave Jiang wrote:
> > On Apr 12, 2018, at 7:30 PM, Verma, Vishal L 
> 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 
> >>> 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 
>  ---
> 
>  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(>regions, region, _r, list)
>  +free_region(region, >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();
> >>> 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

Re: [PATCH v2] ndctl: fix libdaxctl memory leak

2018-04-12 Thread Jiang, Dave



> On Apr 12, 2018, at 7:30 PM, Verma, Vishal L  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 
>>> 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 
 ---
 
 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(>regions, region, _r, list)
 +free_region(region, >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();
>>> 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

Re: [PATCH v2] ndctl: fix libdaxctl memory leak

2018-04-12 Thread Verma, Vishal L

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 
> > 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 
> > > ---
> > > 
> > > 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(>regions, region, _r, list)
> > > + free_region(region, >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();
> > 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

Re: [PATCH v2] ndctl: fix libdaxctl memory leak

2018-04-10 Thread Dave Jiang



On 04/10/2018 10:38 AM, Plewa, Lukasz wrote:
> On Tue, Apr 10, 2018 at 7:17 PM, Dave Jiang  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 
>> ---
>>
>> 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(>regions, region, _r, list)
>> +free_region(region, >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();
> 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

RE: [PATCH v2] ndctl: fix libdaxctl memory leak

Re: [PATCH v2] ndctl: fix libdaxctl memory leak

RE: [PATCH v2] ndctl: fix libdaxctl memory leak

Re: [PATCH v2] ndctl: fix libdaxctl memory leak

Re: [PATCH v2] ndctl: fix libdaxctl memory leak

Re: [PATCH v2] ndctl: fix libdaxctl memory leak

6 matches

Site Navigation

Mail list logo

Footer information