On Apr 13, 2018, at 4:40AM, Dave Jiang wrote: > > On Apr 12, 2018, at 7:30 PM, Verma, Vishal L <[email protected]> > 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 <[email protected]> > >>> 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 <[email protected]> > >>>> --- > >>>> > >>>> 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 [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
