RE: [PATCH] mm/compaction: remove unused variable sysctl_compact_memory

From: Nitin Gupta <nigupta@nvidia.com>
To: "pintu@codeaurora.org" <pintu@codeaurora.org>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	"iamjoonsoo.kim@lge.com" <iamjoonsoo.kim@lge.com>,
	"sh_def@163.com" <sh_def@163.com>,
	"mateusznosek0@gmail.com" <mateusznosek0@gmail.com>,
	"bhe@redhat.com" <bhe@redhat.com>,
	"vbabka@suse.cz" <vbabka@suse.cz>,
	"yzaikin@google.com" <yzaikin@google.com>,
	"keescook@chromium.org" <keescook@chromium.org>,
	"mcgrof@kernel.org" <mcgrof@kernel.org>,
	"mgorman@techsingularity.net" <mgorman@techsingularity.net>,
	"pintu.ping@gmail.com" <pintu.ping@gmail.com>
Subject: RE: [PATCH] mm/compaction: remove unused variable sysctl_compact_memory
Date: Wed, 3 Mar 2021 18:14:50 +0000	[thread overview]
Message-ID: <BYAPR12MB3416A388F9635A0F8799E49BD8989@BYAPR12MB3416.namprd12.prod.outlook.com> (raw)
In-Reply-To: <d3a2ef132c8deee8ced530367c81479c@codeaurora.org>

> -----Original Message-----
> From: owner-linux-mm@kvack.org <owner-linux-mm@kvack.org> On Behalf
> Of pintu@codeaurora.org
> Sent: Wednesday, March 3, 2021 6:34 AM
> To: Nitin Gupta <nigupta@nvidia.com>
> Cc: linux-kernel@vger.kernel.org; akpm@linux-foundation.org; linux-
> mm@kvack.org; linux-fsdevel@vger.kernel.org; iamjoonsoo.kim@lge.com;
> sh_def@163.com; mateusznosek0@gmail.com; bhe@redhat.com;
> vbabka@suse.cz; yzaikin@google.com; keescook@chromium.org;
> mcgrof@kernel.org; mgorman@techsingularity.net; pintu.ping@gmail.com
> Subject: Re: [PATCH] mm/compaction: remove unused variable
> sysctl_compact_memory
> 
> External email: Use caution opening links or attachments
> 
> 
> On 2021-03-03 01:48, Nitin Gupta wrote:
> >> -----Original Message-----
> >> From: pintu=codeaurora.org@mg.codeaurora.org
> >> <pintu=codeaurora.org@mg.codeaurora.org> On Behalf Of Pintu Kumar
> >> Sent: Tuesday, March 2, 2021 9:56 AM
> >> To: linux-kernel@vger.kernel.org; akpm@linux-foundation.org; linux-
> >> mm@kvack.org; linux-fsdevel@vger.kernel.org; pintu@codeaurora.org;
> >> iamjoonsoo.kim@lge.com; sh_def@163.com;
> mateusznosek0@gmail.com;
> >> bhe@redhat.com; Nitin Gupta <nigupta@nvidia.com>; vbabka@suse.cz;
> >> yzaikin@google.com; keescook@chromium.org; mcgrof@kernel.org;
> >> mgorman@techsingularity.net
> >> Cc: pintu.ping@gmail.com
> >> Subject: [PATCH] mm/compaction: remove unused variable
> >> sysctl_compact_memory
> >>
> >> External email: Use caution opening links or attachments
> >>
> >>
> >> The sysctl_compact_memory is mostly unsed in mm/compaction.c It just
> >> acts as a place holder for sysctl.
> >>
> >> Thus we can remove it from here and move the declaration directly in
> >> kernel/sysctl.c itself.
> >> This will also eliminate the extern declaration from header file.
> >
> >
> > I prefer keeping the existing pattern of listing all compaction
> > related tunables together in compaction.h:
> >
> >       extern int sysctl_compact_memory;
> >       extern unsigned int sysctl_compaction_proactiveness;
> >       extern int sysctl_extfrag_threshold;
> >       extern int sysctl_compact_unevictable_allowed;
> >
> 
> Thanks Nitin for your review.
> You mean, you just wanted to retain this extern declaration ?
> Any real benefit of keeping this declaration if not used elsewhere ?
> 

I see that sysctl_compaction_handler() doesn't use the sysctl value at all.
So, we can get rid of it completely as Vlastimil suggested.

> >
> >> No functionality is broken or changed this way.
> >>
> >> Signed-off-by: Pintu Kumar <pintu@codeaurora.org>
> >> Signed-off-by: Pintu Agarwal <pintu.ping@gmail.com>
> >> ---
> >>  include/linux/compaction.h | 1 -
> >>  kernel/sysctl.c            | 1 +
> >>  mm/compaction.c            | 3 ---
> >>  3 files changed, 1 insertion(+), 4 deletions(-)
> >>
> >> diff --git a/include/linux/compaction.h b/include/linux/compaction.h
> >> index
> >> ed4070e..4221888 100644
> >> --- a/include/linux/compaction.h
> >> +++ b/include/linux/compaction.h
> >> @@ -81,7 +81,6 @@ static inline unsigned long compact_gap(unsigned
> >> int
> >> order)  }
> >>
> >>  #ifdef CONFIG_COMPACTION
> >> -extern int sysctl_compact_memory;
> >>  extern unsigned int sysctl_compaction_proactiveness;  extern int
> >> sysctl_compaction_handler(struct ctl_table *table, int write,
> >>                         void *buffer, size_t *length, loff_t *ppos);
> >> diff --git a/kernel/sysctl.c b/kernel/sysctl.c index c9fbdd8..66aff21
> >> 100644
> >> --- a/kernel/sysctl.c
> >> +++ b/kernel/sysctl.c
> >> @@ -198,6 +198,7 @@ static int max_sched_tunable_scaling =
> >> SCHED_TUNABLESCALING_END-1;  #ifdef CONFIG_COMPACTION  static int
> >> min_extfrag_threshold;  static int max_extfrag_threshold = 1000;
> >> +static int sysctl_compact_memory;
> >>  #endif
> >>
> >>  #endif /* CONFIG_SYSCTL */
> >> diff --git a/mm/compaction.c b/mm/compaction.c index
> 190ccda..ede2886
> >> 100644
> >> --- a/mm/compaction.c
> >> +++ b/mm/compaction.c
> >> @@ -2650,9 +2650,6 @@ static void compact_nodes(void)
> >>                 compact_node(nid);
> >>  }
> >>
> >> -/* The written value is actually unused, all memory is compacted */
> >> -int sysctl_compact_memory;
> >> -
> >
> >
> > Please retain this comment for the tunable.
> 
> Sorry, I could not understand.
> You mean to say just retain this last comment and only remove the
> variable ?
> Again any real benefit you see in retaining this even if its not used?
> 
> 

You are just moving declaration of sysctl_compact_memory from compaction.c
to sysctl.c. So, I wanted the comment "... all memory is compacted" to be retained
with the sysctl variable. Since you are now getting rid of this variable completely,
this comment goes away too.

Thanks,
Nitin