On Mon, Nov 26, 2018 at 11:03:30AM +0100, Michal Hocko wrote:
>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.
>
Let me summarize what you expect to do.
Go through all the users of pgdat_resize_lock, if none of them is called
from IRQ context, we could do the following change:
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index ffd9cd10fcf3..45a5affcab8a 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -272,14 +272,14 @@ static inline bool movable_node_is_enabled(void)
* pgdat resizing functions
*/
static inline
-void pgdat_resize_lock(struct pglist_data *pgdat, unsigned long *flags)
+void pgdat_resize_lock(struct pglist_data *pgdat)
{
- spin_lock_irqsave(&pgdat->node_size_lock, *flags);
+ spin_lock(&pgdat->node_size_lock);
}
static inline
-void pgdat_resize_unlock(struct pglist_data *pgdat, unsigned long *flags)
+void pgdat_resize_unlock(struct pglist_data *pgdat)
{
- spin_unlock_irqrestore(&pgdat->node_size_lock, *flags);
+ spin_unlock(&pgdat->node_size_lock);
}
static inline
void pgdat_resize_init(struct pglist_data *pgdat)
>> >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.
>
Thanks.
I took a look at this one which seems to resolve my confusion.
https://www.kernel.org/doc/Documentation/locking/spinlocks.txt
>--
>Michal Hocko
>SUSE Labs
--
Wei Yang
Help you, Help me
