Re: Query:Regarding percpu_counter debug object destroy

[Nikolay Borisov]

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
>