Messages in this thread | | | Subject | Re: Query:Regarding percpu_counter debug object destroy | From | "Kohli, Gaurav" <> | Date | Fri, 13 Apr 2018 14:16:48 +0530 |
| |
Hi Nikolay,
Thanks for the comment.
I agree ,like timer , hrtimer we have to mark inactive in destroy function and finally freeing the debug object
after destruction of percpu_counter.
But i am still not sure that this double freeing with same address may create race or not in debug_object list.
Regards
Gaurav
On 4/13/2018 1:12 PM, Nikolay Borisov wrote:
> > On 13.04.2018 10:32, Kohli, Gaurav wrote: >> Hi , >> >> I have checked below code and it seems we are calling debug_object_free >> twice, ideally we should deactivate and later we >> have to destroy. >> >> 1st call -> percpu_counter_destroy->debug_percpu_counter_deactivate -> >> debug_object_free >> 2nd call -> >> debug_object_free >> >> >> >> static bool percpu_counter_fixup_free(void *addr, enum debug_obj_state >> state) >> { >> struct percpu_counter *fbc = addr; >> >> switch (state) { >> case ODEBUG_STATE_ACTIVE: >> percpu_counter_destroy(fbc); -> first call >> debug_object_free(fbc, &percpu_counter_debug_descr); 2nd > > Having looked at the code I'd say this is indeed buggy. I'd say it > stemmed from same cargo culting since timer_fixup_free follows the same > structure of code, except that in del_timer_sync there is no code which > does debug_object_free. The situation is similar in work_fixup_free. > > So at this point I guess the question is whether we want to leave the > debug_object_free call in percpu_counter_fixup_free and just remove > debug_percpu_counter_deactivate and open-code the call to > debug_object_deactivate in percpu_counter_destroy. Or remove the > explicit call in percpu_counter_fixup_free and leave > debug_percpu_counter_deactivate. > > > In the end it's a matter of style, so perhaps Tejun, as the maintainer, > has the final say what style he prefers. Personally, I'd go for the > former solution so that the percpu follows the style of the rest of the > kernel. > >> call >> return true; >> default: >> return false; >> } >> } >> >> >> We are seeing one issue, where one list contain garbage data in >> obj_hash, just before element of that is percpu_counter but >> still not sure as it is very difficult to reproduce. >> >> Can some one please review above code or can we remove one instance of >> debug_object_free from above code. >> >> Regards >> Gaurav >> >> -- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
| |