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

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

[Chaitanya Kulkarni]

On 3/2/21 22:21, Pintu Kumar wrote:
> 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.
> 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>

You need to specify the first commit which added sysctl_compact_memory
variable and if exists the last commit which removed the last access
of the same variable in the file mm/compaction.c in for completeness
of the commit log.

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

[pintu]

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 ?

> 
>> 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?


Thanks,
Pintu

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

[Nitin Gupta]

> -----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

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

[Vlastimil Babka]

On 3/2/21 6:56 PM, Pintu Kumar wrote:
> 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.
> 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>

You should be able to remove the variable completely and set .data to NULL in
the corresponding entry. The sysctl_compaction_handler doesn't access it at all.

Then you could do the same with drop_caches. Currently
drop_caches_sysctl_handler currently writes to it, but that can be avoided using
a local variable - see how sysrq_sysctl_handler avoids the global variable and
its corresponding .data field is NULL.

Vlastimil

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

[pintu]

On 2021-03-03 22:39, Vlastimil Babka wrote:
> On 3/2/21 6:56 PM, Pintu Kumar wrote:
>> 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.
>> 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>
> 
> You should be able to remove the variable completely and set .data to 
> NULL in
> the corresponding entry. The sysctl_compaction_handler doesn't access 
> it at all.
> 
> Then you could do the same with drop_caches. Currently
> drop_caches_sysctl_handler currently writes to it, but that can be 
> avoided using
> a local variable - see how sysrq_sysctl_handler avoids the global 
> variable and
> its corresponding .data field is NULL.
> 

Oh yes, thank you so much for the reference.
Yes I was looking to do something similar but didn't realize that is 
possible.
I will re-submit the new patch.

And yes, I will try to explore more on drop_caches part as well.

Thanks,
Pintu

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

[Pintu Kumar]

The sysctl_compact_memory is mostly unused in mm/compaction.c
It just acts as a place holder for sysctl to store .data.

But the .data itself is not needed here.
So we can get ride of this variable completely and make .data as NULL.
This will also eliminate the extern declaration from header file.
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>
---
v2: completely get rid of this variable and set .data to NULL
    Suggested-by: Vlastimil Babka <vbabka@suse.cz>

 include/linux/compaction.h | 1 -
 kernel/sysctl.c            | 2 +-
 mm/compaction.c            | 3 ---
 3 files changed, 1 insertion(+), 5 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..07ef240 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2856,7 +2856,7 @@ static struct ctl_table vm_table[] = {
 #ifdef CONFIG_COMPACTION
 	{
 		.procname	= "compact_memory",
-		.data		= &sysctl_compact_memory,
+		.data		= NULL,
 		.maxlen		= sizeof(int),
 		.mode		= 0200,
 		.proc_handler	= sysctl_compaction_handler,
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;
-
 /*
  * Tunable for proactive compaction. It determines how
  * aggressively the kernel should compact memory in the
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v2] mm/compaction: remove unused variable sysctl_compact_memory

[Vlastimil Babka]

On 3/4/21 11:03 AM, Pintu Kumar wrote:
> The sysctl_compact_memory is mostly unused in mm/compaction.c
> It just acts as a place holder for sysctl to store .data.
> 
> But the .data itself is not needed here.
> So we can get ride of this variable completely and make .data as NULL.
> This will also eliminate the extern declaration from header file.
> 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>

Reviewed-by: Vlastimil Babka <vbabka@suse.cz>

> ---
> v2: completely get rid of this variable and set .data to NULL
>     Suggested-by: Vlastimil Babka <vbabka@suse.cz>
> 
>  include/linux/compaction.h | 1 -
>  kernel/sysctl.c            | 2 +-
>  mm/compaction.c            | 3 ---
>  3 files changed, 1 insertion(+), 5 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..07ef240 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -2856,7 +2856,7 @@ static struct ctl_table vm_table[] = {
>  #ifdef CONFIG_COMPACTION
>  	{
>  		.procname	= "compact_memory",
> -		.data		= &sysctl_compact_memory,
> +		.data		= NULL,
>  		.maxlen		= sizeof(int),
>  		.mode		= 0200,
>  		.proc_handler	= sysctl_compaction_handler,
> 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;
> -
>  /*
>   * Tunable for proactive compaction. It determines how
>   * aggressively the kernel should compact memory in the
>