From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934680AbcLBO5K (ORCPT ); Fri, 2 Dec 2016 09:57:10 -0500 Received: from mail-ua0-f196.google.com ([209.85.217.196]:35253 "EHLO mail-ua0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934227AbcLBO5I (ORCPT ); Fri, 2 Dec 2016 09:57:08 -0500 MIME-Version: 1.0 In-Reply-To: <20161202144440.GQ6830@dhcp22.suse.cz> References: <1480540516-6458-1-git-send-email-yuzhao@google.com> <20161202134604.GA6837@dhcp22.suse.cz> <20161202143848.GP6830@dhcp22.suse.cz> <20161202144440.GQ6830@dhcp22.suse.cz> From: Dan Streetman Date: Fri, 2 Dec 2016 09:56:26 -0500 X-Google-Sender-Auth: TBqjfaAafI3WUIAtA6leWWtQr2E Message-ID: Subject: Re: [PATCH v2] zswap: only use CPU notifier when HOTPLUG_CPU=y To: Michal Hocko Cc: Yu Zhao , Seth Jennings , Linux-MM , linux-kernel Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Dec 2, 2016 at 9:44 AM, Michal Hocko wrote: > On Fri 02-12-16 15:38:48, Michal Hocko wrote: >> On Fri 02-12-16 09:24:35, Dan Streetman wrote: >> > On Fri, Dec 2, 2016 at 8:46 AM, Michal Hocko wrote: >> > > On Wed 30-11-16 13:15:16, Yu Zhao wrote: >> > >> __unregister_cpu_notifier() only removes registered notifier from its >> > >> linked list when CPU hotplug is configured. If we free registered CPU >> > >> notifier when HOTPLUG_CPU=n, we corrupt the linked list. >> > >> >> > >> To fix the problem, we can either use a static CPU notifier that walks >> > >> through each pool or just simply disable CPU notifier when CPU hotplug >> > >> is not configured (which is perfectly safe because the code in question >> > >> is called after all possible CPUs are online and will remain online >> > >> until power off). >> > >> >> > >> v2: #ifdef for cpu_notifier_register_done during cleanup. >> > > >> > > this ifedfery is just ugly as hell. I am also wondering whether it is >> > > really needed. __register_cpu_notifier and __unregister_cpu_notifier are >> > > noops for CONFIG_HOTPLUG_CPU=n. So what's exactly that is broken here? >> > >> > hmm, that's interesting, __unregister_cpu_notifier is always a noop if >> > HOTPLUG_CPU=n, but __register_cpu_notifier is only a noop if >> > HOTPLUG_CPU=n *and* MODULE. If !MODULE, __register_cpu_notifier does >> >> OK, I've missed the MODULE part >> >> > actually register! This was added by commit >> > 47e627bc8c9a70392d2049e6af5bd55fae61fe53 ('hotplug: Allow modules to >> > use the cpu hotplug notifiers even if !CONFIG_HOTPLUG_CPU') and looks >> > like it's to allow built-ins to register so they can notice during >> > boot when cpus are initialized. >> >> I cannot say I wound understand the motivation but that is not really >> all that important. >> >> > IMHO, that is the real problem - sure, without HOTPLUG_CPU, nobody >> > should ever get a notification that a cpu is dying, but that doesn't >> > mean builtins that register notifiers will never unregister their >> > notifiers and then free them. >> >> Yes that is true. That suggests that __unregister_cpu_notifier should >> the the symmetric thing to the __register_cpu_notifier for >> CONFIG_MODULE, right? > > I meant the following. Completely untested agreed, but also needs the non-__ version, and kernel/cpu.c needs tweaking to move those functions out of the #ifdef CONFIG_HOTPLUG_CPU section. > --- > diff --git a/include/linux/cpu.h b/include/linux/cpu.h > index 797d9c8e9a1b..8d7b473426af 100644 > --- a/include/linux/cpu.h > +++ b/include/linux/cpu.h > @@ -120,6 +120,7 @@ extern void __unregister_cpu_notifier(struct notifier_block *nb); > #ifndef MODULE > extern int register_cpu_notifier(struct notifier_block *nb); > extern int __register_cpu_notifier(struct notifier_block *nb); > +extern void __unregister_cpu_notifier(struct notifier_block *nb); > #else > static inline int register_cpu_notifier(struct notifier_block *nb) > { > -- > Michal Hocko > SUSE Labs From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-vk0-f72.google.com (mail-vk0-f72.google.com [209.85.213.72]) by kanga.kvack.org (Postfix) with ESMTP id 1BDBC6B025E for ; Fri, 2 Dec 2016 09:57:08 -0500 (EST) Received: by mail-vk0-f72.google.com with SMTP id 192so115922562vkh.5 for ; Fri, 02 Dec 2016 06:57:08 -0800 (PST) Received: from mail-ua0-x244.google.com (mail-ua0-x244.google.com. [2607:f8b0:400c:c08::244]) by mx.google.com with ESMTPS id e10si1328640uaa.209.2016.12.02.06.57.07 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 02 Dec 2016 06:57:07 -0800 (PST) Received: by mail-ua0-x244.google.com with SMTP id 12so24872797uas.3 for ; Fri, 02 Dec 2016 06:57:07 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <20161202144440.GQ6830@dhcp22.suse.cz> References: <1480540516-6458-1-git-send-email-yuzhao@google.com> <20161202134604.GA6837@dhcp22.suse.cz> <20161202143848.GP6830@dhcp22.suse.cz> <20161202144440.GQ6830@dhcp22.suse.cz> From: Dan Streetman Date: Fri, 2 Dec 2016 09:56:26 -0500 Message-ID: Subject: Re: [PATCH v2] zswap: only use CPU notifier when HOTPLUG_CPU=y Content-Type: text/plain; charset=UTF-8 Sender: owner-linux-mm@kvack.org List-ID: To: Michal Hocko Cc: Yu Zhao , Seth Jennings , Linux-MM , linux-kernel On Fri, Dec 2, 2016 at 9:44 AM, Michal Hocko wrote: > On Fri 02-12-16 15:38:48, Michal Hocko wrote: >> On Fri 02-12-16 09:24:35, Dan Streetman wrote: >> > On Fri, Dec 2, 2016 at 8:46 AM, Michal Hocko wrote: >> > > On Wed 30-11-16 13:15:16, Yu Zhao wrote: >> > >> __unregister_cpu_notifier() only removes registered notifier from its >> > >> linked list when CPU hotplug is configured. If we free registered CPU >> > >> notifier when HOTPLUG_CPU=n, we corrupt the linked list. >> > >> >> > >> To fix the problem, we can either use a static CPU notifier that walks >> > >> through each pool or just simply disable CPU notifier when CPU hotplug >> > >> is not configured (which is perfectly safe because the code in question >> > >> is called after all possible CPUs are online and will remain online >> > >> until power off). >> > >> >> > >> v2: #ifdef for cpu_notifier_register_done during cleanup. >> > > >> > > this ifedfery is just ugly as hell. I am also wondering whether it is >> > > really needed. __register_cpu_notifier and __unregister_cpu_notifier are >> > > noops for CONFIG_HOTPLUG_CPU=n. So what's exactly that is broken here? >> > >> > hmm, that's interesting, __unregister_cpu_notifier is always a noop if >> > HOTPLUG_CPU=n, but __register_cpu_notifier is only a noop if >> > HOTPLUG_CPU=n *and* MODULE. If !MODULE, __register_cpu_notifier does >> >> OK, I've missed the MODULE part >> >> > actually register! This was added by commit >> > 47e627bc8c9a70392d2049e6af5bd55fae61fe53 ('hotplug: Allow modules to >> > use the cpu hotplug notifiers even if !CONFIG_HOTPLUG_CPU') and looks >> > like it's to allow built-ins to register so they can notice during >> > boot when cpus are initialized. >> >> I cannot say I wound understand the motivation but that is not really >> all that important. >> >> > IMHO, that is the real problem - sure, without HOTPLUG_CPU, nobody >> > should ever get a notification that a cpu is dying, but that doesn't >> > mean builtins that register notifiers will never unregister their >> > notifiers and then free them. >> >> Yes that is true. That suggests that __unregister_cpu_notifier should >> the the symmetric thing to the __register_cpu_notifier for >> CONFIG_MODULE, right? > > I meant the following. Completely untested agreed, but also needs the non-__ version, and kernel/cpu.c needs tweaking to move those functions out of the #ifdef CONFIG_HOTPLUG_CPU section. > --- > diff --git a/include/linux/cpu.h b/include/linux/cpu.h > index 797d9c8e9a1b..8d7b473426af 100644 > --- a/include/linux/cpu.h > +++ b/include/linux/cpu.h > @@ -120,6 +120,7 @@ extern void __unregister_cpu_notifier(struct notifier_block *nb); > #ifndef MODULE > extern int register_cpu_notifier(struct notifier_block *nb); > extern int __register_cpu_notifier(struct notifier_block *nb); > +extern void __unregister_cpu_notifier(struct notifier_block *nb); > #else > static inline int register_cpu_notifier(struct notifier_block *nb) > { > -- > Michal Hocko > SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org