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.ve...@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.ji...@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.ji...@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
