> dinghao.liu@ wrote: > > > Dave Jiang wrote: > > [snip] > > > > That said, this patch does not completely fix freelist from leaking in the > > > following error path. > > > > > > discover_arenas() > > > btt_freelist_init() -> ok (memory allocated) > > > btt_rtt_init() -> fail > > > goto out; > > > (leak because arena is not yet on btt->arena_list) > > > OR > > > btt_maplocks_init() -> fail > > > goto out; > > > (leak because arena is not yet on btt->arena_list) > > > > > > > Thanks for pointing out this issue! I rechecked discover_arenas() and found > > that btt_rtt_init() may also trigger a memleak for the same reason as > > btt_freelist_init(). Also, I checked another call trace: > > > > btt_init() -> btt_meta_init() -> btt_maplocks_init() > > > > I think there is a memleak if btt_maplocks_init() succeeds but an error > > happens in btt_init() after btt_meta_init() (e.g., when btt_blk_init() > > returns an error). Therefore, we may need to fix three functions. > > Yea I think we need to trace this code better. This is why devm_ is nice for > memory allocated for the life of the device. > > > > > > This error could be fixed by adding to arena_list earlier but devm_*() > > > also takes care of this without having to worry about that logic. > > > > > > On normal operation all of this memory can be free'ed with the > > > corresponding devm_kfree() and/or devm_add_action_*() calls if arenas come > > > and go. I'm not sure off the top of my head. > > > > > > In addition, looking at this code. discover_arenas() could make use of > > > the scoped based management for struct btt_sb *super! > > > > > > Dinghao would you be willing to submit a series of 2 or 3 patches to fix > > > the above issues? > > > > > > > Sure. Currently I plan to send 2 patches as follows: > > 1. Using devm_kcalloc() to replace kcalloc() in btt_freelist_init(), > > btt_rtt_init(), and btt_maplocks_init(), and removing the corresponding > > kfree in free_arenas(). I checked some uses of devm_kcalloc() and it > > seems that we need not to call devm_kfree(). The memory is automatically > > freed on driver detach, right? > > On device put yes. So if these allocations are scoped to the life of the > device there would be no reason to call devm_kfree() on them at all. I was > not > sure if they got reallocated at some point or not. > > > 2. Using the scoped based management for struct btt_sb *super (not a bug, > > but it could improve the code). > > Good! > > > > > I'm not quite sure whether my understanding or bug fixing plan is correct. > > If there are any issues, please correct me, thanks! > > The above sounds right. > Ira
Thanks for the review! I will send the patches soon. Regards, Dinghao
