From mboxrd@z Thu Jan 1 00:00:00 1970 From: Glauber Costa Subject: Re: [PATCH] memcg: Add memory.pressure_level events Date: Wed, 13 Feb 2013 11:55:38 +0400 Message-ID: <511B46FA.1090802@parallels.com> References: <20130211000220.GA28247@lizard.gateway.2wire.net> <5118C522.3070905@parallels.com> <20130213071922.GB20543@lizard.gateway.2wire.net> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20130213071922.GB20543-1CZZkhFgUWNRmOO2HpYVOJIbA5emDH3N@public.gmane.org> Sender: cgroups-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: Content-Type: text/plain; charset="us-ascii" To: Anton Vorontsov Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Tejun Heo , David Rientjes , Pekka Enberg , Mel Gorman , Michal Hocko , "Kirill A. Shutemov" , Kamezawa Hiroyuki , Luiz Capitulino , Andrew Morton , Greg Thelen , Leonid Moiseichuk , KOSAKI Motohiro , Minchan Kim , Bartlomiej Zolnierkiewicz , John Stultz , linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linaro-kernel-cunTk1MwBs8s++Sfvej+rw@public.gmane.org, patches-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, kernel-team-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org >>> +void __init enable_pressure_cgroup(void) >>> +{ >>> + WARN_ON(cgroup_add_cftypes(&mem_cgroup_subsys, >>> + vmpressure_cgroup_files)); >>> +} >> >> There is no functionality discovery going on here, and this is >> conditional on nothing. Isn't it better then to just add the register + >> read functions to memcontrol.c and add the files in the memcontrol cftype ? > > I was trying to make the stuff similar to the existing CONFIG_MEMCG_SWAP > code, which does this kind of adding files to the cgroup. But I can surely > place files into memcontrol cftype as you suggest. > > Thanks a lot for the comments! > Note that swap can be command line disabled, and in that case we won't register the files. Then it makes sense to do it in a separate helper. If I understand your code correctly, once it is compiled in, it will always be enabled. So I personally think it is clearer if you register it together with the rest of the crew. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from psmtp.com (na3sys010amx139.postini.com [74.125.245.139]) by kanga.kvack.org (Postfix) with SMTP id E547B6B0005 for ; Wed, 13 Feb 2013 02:55:31 -0500 (EST) Message-ID: <511B46FA.1090802@parallels.com> Date: Wed, 13 Feb 2013 11:55:38 +0400 From: Glauber Costa MIME-Version: 1.0 Subject: Re: [PATCH] memcg: Add memory.pressure_level events References: <20130211000220.GA28247@lizard.gateway.2wire.net> <5118C522.3070905@parallels.com> <20130213071922.GB20543@lizard.gateway.2wire.net> In-Reply-To: <20130213071922.GB20543@lizard.gateway.2wire.net> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Anton Vorontsov Cc: cgroups@vger.kernel.org, Tejun Heo , David Rientjes , Pekka Enberg , Mel Gorman , Michal Hocko , "Kirill A. Shutemov" , Kamezawa Hiroyuki , Luiz Capitulino , Andrew Morton , Greg Thelen , Leonid Moiseichuk , KOSAKI Motohiro , Minchan Kim , Bartlomiej Zolnierkiewicz , John Stultz , linux-mm@kvack.org, linux-kernel@vger.kernel.org, linaro-kernel@lists.linaro.org, patches@linaro.org, kernel-team@android.com >>> +void __init enable_pressure_cgroup(void) >>> +{ >>> + WARN_ON(cgroup_add_cftypes(&mem_cgroup_subsys, >>> + vmpressure_cgroup_files)); >>> +} >> >> There is no functionality discovery going on here, and this is >> conditional on nothing. Isn't it better then to just add the register + >> read functions to memcontrol.c and add the files in the memcontrol cftype ? > > I was trying to make the stuff similar to the existing CONFIG_MEMCG_SWAP > code, which does this kind of adding files to the cgroup. But I can surely > place files into memcontrol cftype as you suggest. > > Thanks a lot for the comments! > Note that swap can be command line disabled, and in that case we won't register the files. Then it makes sense to do it in a separate helper. If I understand your code correctly, once it is compiled in, it will always be enabled. So I personally think it is clearer if you register it together with the rest of the crew. -- 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758520Ab3BMHze (ORCPT ); Wed, 13 Feb 2013 02:55:34 -0500 Received: from mx0.parallels.com ([199.115.104.20]:48024 "EHLO mx0.parallels.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757959Ab3BMHzc (ORCPT ); Wed, 13 Feb 2013 02:55:32 -0500 Message-ID: <511B46FA.1090802@parallels.com> Date: Wed, 13 Feb 2013 11:55:38 +0400 From: Glauber Costa User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130110 Thunderbird/17.0.2 MIME-Version: 1.0 To: Anton Vorontsov CC: , Tejun Heo , David Rientjes , Pekka Enberg , Mel Gorman , Michal Hocko , "Kirill A. Shutemov" , Kamezawa Hiroyuki , Luiz Capitulino , Andrew Morton , Greg Thelen , Leonid Moiseichuk , KOSAKI Motohiro , Minchan Kim , Bartlomiej Zolnierkiewicz , John Stultz , , , , , Subject: Re: [PATCH] memcg: Add memory.pressure_level events References: <20130211000220.GA28247@lizard.gateway.2wire.net> <5118C522.3070905@parallels.com> <20130213071922.GB20543@lizard.gateway.2wire.net> In-Reply-To: <20130213071922.GB20543@lizard.gateway.2wire.net> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org >>> +void __init enable_pressure_cgroup(void) >>> +{ >>> + WARN_ON(cgroup_add_cftypes(&mem_cgroup_subsys, >>> + vmpressure_cgroup_files)); >>> +} >> >> There is no functionality discovery going on here, and this is >> conditional on nothing. Isn't it better then to just add the register + >> read functions to memcontrol.c and add the files in the memcontrol cftype ? > > I was trying to make the stuff similar to the existing CONFIG_MEMCG_SWAP > code, which does this kind of adding files to the cgroup. But I can surely > place files into memcontrol cftype as you suggest. > > Thanks a lot for the comments! > Note that swap can be command line disabled, and in that case we won't register the files. Then it makes sense to do it in a separate helper. If I understand your code correctly, once it is compiled in, it will always be enabled. So I personally think it is clearer if you register it together with the rest of the crew.