> 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. _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
