Re: [PATCH] mm, hotplug: protect nr_zones with pgdat_resize_lock()

From: Michal Hocko <mhocko@suse.com>
To: Wei Yang <richard.weiyang@gmail.com>
Cc: Oscar Salvador <osalvador@suse.de>,
	Andrew Morton <akpm@linux-foundation.org>,
	Linux-MM <linux-mm@kvack.org>
Subject: Re: [PATCH] mm, hotplug: protect nr_zones with pgdat_resize_lock()
Date: Mon, 26 Nov 2018 11:03:30 +0100	[thread overview]
Message-ID: <20181126100330.GF12455@dhcp22.suse.cz> (raw)
In-Reply-To: <20181126090654.hgazohtksychaaf3@master>

On Mon 26-11-18 09:06:54, Wei Yang wrote:
> On Mon, Nov 26, 2018 at 09:16:08AM +0100, Michal Hocko wrote:
> >On Mon 26-11-18 10:28:40, Wei Yang wrote:
> >[...]
> >> But I get some difficulty to understand this TODO. You want to get rid of
> >> these lock? While these locks seem necessary to protect those data of
> >> pgdat/zone. Would you mind sharing more on this statement?
> >
> >Why do we need this lock to be irqsave? Is there any caller that uses
> >the lock from the IRQ context?
> 
> I see you put the comment 'irqsave' in code, I thought this is the
> requirement bringing in by this commit. So this is copyed from somewhere
> else?

No, the irqsave lock has been there for a long time but it was not clear
to me whether it is still required. Maybe it never was. I just didn't
have time to look into that and put a TODO there. The code wouldn't be
less correct if I kept it.

> >From my understanding, we don't access pgdat from interrupt context.
> 
> BTW, one more confirmation. One irqsave lock means we can't do something
> during holding the lock, like sleep. Is my understanding correct?

You cannot sleep in any atomic context. IRQ safe lock only means that
IRQs are disabled along with the lock. The irqsave variant should be
taken when an IRQ context itself can take the lock. There is a lot of
documentation to clarify this e.g. Linux Device Drivers. I would
recommend to read through that.

-- 
Michal Hocko
SUSE Labs