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

Reply via email to