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 <[email protected]>
Signed-off-by: Pintu Agarwal <[email protected]>
---
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;
-
/*
* 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.
> -----Original Message-----
> From: [email protected]
> <[email protected]> On Behalf Of Pintu Kumar
> Sent: Tuesday, March 2, 2021 9:56 AM
> To: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Nitin Gupta <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Cc: [email protected]
> 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;
> No functionality is broken or changed this way.
>
> Signed-off-by: Pintu Kumar <[email protected]>
> Signed-off-by: Pintu Agarwal <[email protected]>
> ---
> 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.
-Nitin
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 <[email protected]>
> Signed-off-by: Pintu Agarwal <[email protected]>
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.
On 2021-03-03 01:48, Nitin Gupta wrote:
>> -----Original Message-----
>> From: [email protected]
>> <[email protected]> On Behalf Of Pintu Kumar
>> Sent: Tuesday, March 2, 2021 9:56 AM
>> To: [email protected]; [email protected]; linux-
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; Nitin Gupta <[email protected]>; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]
>> Cc: [email protected]
>> 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 <[email protected]>
>> Signed-off-by: Pintu Agarwal <[email protected]>
>> ---
>> 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
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 <[email protected]>
> Signed-off-by: Pintu Agarwal <[email protected]>
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
> -----Original Message-----
> From: [email protected] <[email protected]> On Behalf
> Of [email protected]
> Sent: Wednesday, March 3, 2021 6:34 AM
> To: Nitin Gupta <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> 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: [email protected]
> >> <[email protected]> On Behalf Of Pintu Kumar
> >> Sent: Tuesday, March 2, 2021 9:56 AM
> >> To: [email protected]; [email protected]; linux-
> >> [email protected]; [email protected]; [email protected];
> >> [email protected]; [email protected];
> [email protected];
> >> [email protected]; Nitin Gupta <[email protected]>; [email protected];
> >> [email protected]; [email protected]; [email protected];
> >> [email protected]
> >> Cc: [email protected]
> >> 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 <[email protected]>
> >> Signed-off-by: Pintu Agarwal <[email protected]>
> >> ---
> >> 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
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 <[email protected]>
>> Signed-off-by: Pintu Agarwal <[email protected]>
>
> 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