Re: lockdep warning: console vs. mem hotplug

[Petr Mladek]

On Wed 2017-03-29 09:31:47, Michal Hocko wrote:
> On Tue 28-03-17 18:00:16, Petr Mladek wrote:
> > On Tue 2017-03-28 16:22:27, Michal Hocko wrote:
> > > On Sat 25-03-17 09:04:42, Sergey Senozhatsky wrote:
> > > > On (03/21/17 13:44), Sergey Senozhatsky wrote:
> > > > [..]
> > > > > so we probably can
> > > > > 
> > > > > 
> > > > > 1) move pr_info() out of zone->lock in __offline_isolated_pages().
> > > > >    meh...
> > > > > 
> > > > > 
> > > > > 2) switch to printk_deferred() in __offline_isolated_pages().
> > > > >    meh.. there might a bunch of other printks done from under zone->lock.
> > > > > 
> > > > > 
> > > > > 3) move add_timer() out of sclp_con_lock console in sclp_console_write().
> > > > >    well, there can be other consoles that do something similar.
> > > > > 
> > > > > 
> > > > > 4) ... something smart.
> > > > 
> > > > 
> > Regarding the timer code. The problem seems to be with static
> > timers. They call debug_object_init() when the timer is used
> > for the first time. See the special handling of not-found
> > objects in debug_object_activate().
> 
> Thanks for the clarification!
> 
> > Now, __debug_object_init() calls fill_pool() that allocates
> > the memory. A solution would be to either use static
> > struct kmem_cache
> 
> I am not sure what do you mean by that.

The problem is when the timer is defined using
DEFINE_TIMER() and initialized using __TIMER_INITIALIZER().

I had an unclear idea about extending the macro to define
also the needed structure that might later be used by
debug_object_init() instead of taking it from the pool.

I am sorry for the confusion with struct kmem_cache.
It is the type of the pointer that is passed when
allocating the memory in fill_pool(). It seems
we actually need struct debug_obj. But it might
be even more complicated.


> > for statically defined timers and avoid the allocation. Or we should
> > call fill_pool() asynchronously from a safe context.
> 
> I think we should avoid the allocation completely. It is GFP_ATOMIC and
> so likely to fail under heavy memory pressure. Async fill will make it
> slightly more complicated but still unreliable.
>  
> > What do you think?
> 
> Why cannot we simply embed this debugging data into the timer itself?
> It will make the structure larger (I didn't check how much) but this is
> an opt in feature so it should be acceptable.

This sounds reasonable to me.

> A subtle dependecny on the allocator is really bad and
> we should get rid of it.

Yup.

Best Regards,
Petr