[PATCH V2 2/2] mm: compaction: Limit the value of interface compact_memory

[PATCH V2 2/2] mm: compaction: Limit the value of interface compact_memory

[ye.xingchen]

From: Minghao Chi <chi.minghao@zte.com.cn>

Available only when CONFIG_COMPACTION is set. When 1 is written to
the file, all zones are compacted such that free memory is available
in contiguous blocks where possible.
But echo others-parameter > compact_memory, this function will be
triggered by writing parameters to the interface.

Applied this patch,
sh/$ echo 1.1 > /proc/sys/vm/compact_memory
sh/$ sh: write error: Invalid argument
The start and end time of printing triggering compact_memory.

Link: https://lore.kernel.org/all/ZAJwoXJCzfk1WIBx@bombadil.infradead.org/
Signed-off-by: Minghao Chi <chi.minghao@zte.com.cn>
Signed-off-by: Ye Xingchen <ye.xingchen@zte.com.cn>
---
 mm/compaction.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 5a9501e0ae01..2c9ecc4b9d23 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -2763,6 +2763,8 @@ int compaction_proactiveness_sysctl_handler(struct ctl_table *table, int write,
 	return 0;
 }

+/* The written value is actually unused, all memory is compacted */
+int sysctl_compact_memory;
 /*
  * This is the entry point for compacting all nodes via
  * /proc/sys/vm/compact_memory
@@ -2770,8 +2772,16 @@ int compaction_proactiveness_sysctl_handler(struct ctl_table *table, int write,
 int sysctl_compaction_handler(struct ctl_table *table, int write,
 			void *buffer, size_t *length, loff_t *ppos)
 {
-	if (write)
+	int ret;
+
+	ret = proc_dointvec_minmax(table, write, buffer, length, ppos);
+	if (ret)
+		return ret;
+	if (write) {
+		pr_info("compact_nodes start\n");
 		compact_nodes();
+		pr_info("compact_nodes end\n");
+	}

 	return 0;
 }
-- 
2.25.1

Re: [PATCH V2 2/2] mm: compaction: Limit the value of interface compact_memory

[Vlastimil Babka]

On 3/6/23 07:05, ye.xingchen@zte.com.cn wrote:
> From: Minghao Chi <chi.minghao@zte.com.cn>
> 
> Available only when CONFIG_COMPACTION is set. When 1 is written to
> the file, all zones are compacted such that free memory is available
> in contiguous blocks where possible.
> But echo others-parameter > compact_memory, this function will be
> triggered by writing parameters to the interface.
> 
> Applied this patch,
> sh/$ echo 1.1 > /proc/sys/vm/compact_memory
> sh/$ sh: write error: Invalid argument
> The start and end time of printing triggering compact_memory.
> 
> Link: https://lore.kernel.org/all/ZAJwoXJCzfk1WIBx@bombadil.infradead.org/
> Signed-off-by: Minghao Chi <chi.minghao@zte.com.cn>
> Signed-off-by: Ye Xingchen <ye.xingchen@zte.com.cn>
> ---
>  mm/compaction.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 5a9501e0ae01..2c9ecc4b9d23 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -2763,6 +2763,8 @@ int compaction_proactiveness_sysctl_handler(struct ctl_table *table, int write,
>  	return 0;
>  }
> 
> +/* The written value is actually unused, all memory is compacted */
> +int sysctl_compact_memory;
>  /*
>   * This is the entry point for compacting all nodes via
>   * /proc/sys/vm/compact_memory
> @@ -2770,8 +2772,16 @@ int compaction_proactiveness_sysctl_handler(struct ctl_table *table, int write,
>  int sysctl_compaction_handler(struct ctl_table *table, int write,
>  			void *buffer, size_t *length, loff_t *ppos)
>  {
> -	if (write)
> +	int ret;
> +
> +	ret = proc_dointvec_minmax(table, write, buffer, length, ppos);
> +	if (ret)
> +		return ret;
> +	if (write) {
> +		pr_info("compact_nodes start\n");
>  		compact_nodes();
> +		pr_info("compact_nodes end\n");

I'm not sure we want to start spamming the dmesg. This would make sense
if we wanted to deprecate the sysctl and start hunting for remaining
callers to be fixed. Otherwise ftrace can be used to capture e.g. the time.

> +	}
> 
>  	return 0;
>  }

Re: [PATCH V2 2/2] mm: compaction: Limit the value of interface compact_memory

[Luis Chamberlain]

On Wed, Mar 08, 2023 at 11:23:45AM +0100, Vlastimil Babka wrote:
> 
> 
> On 3/6/23 07:05, ye.xingchen@zte.com.cn wrote:
> > From: Minghao Chi <chi.minghao@zte.com.cn>
> > 
> > Available only when CONFIG_COMPACTION is set. When 1 is written to
> > the file, all zones are compacted such that free memory is available
> > in contiguous blocks where possible.
> > But echo others-parameter > compact_memory, this function will be
> > triggered by writing parameters to the interface.
> > 
> > Applied this patch,
> > sh/$ echo 1.1 > /proc/sys/vm/compact_memory
> > sh/$ sh: write error: Invalid argument
> > The start and end time of printing triggering compact_memory.
> > 
> > Link: https://lore.kernel.org/all/ZAJwoXJCzfk1WIBx@bombadil.infradead.org/
> > Signed-off-by: Minghao Chi <chi.minghao@zte.com.cn>
> > Signed-off-by: Ye Xingchen <ye.xingchen@zte.com.cn>
> > ---
> >  mm/compaction.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mm/compaction.c b/mm/compaction.c
> > index 5a9501e0ae01..2c9ecc4b9d23 100644
> > --- a/mm/compaction.c
> > +++ b/mm/compaction.c
> > @@ -2763,6 +2763,8 @@ int compaction_proactiveness_sysctl_handler(struct ctl_table *table, int write,
> >  	return 0;
> >  }
> > 
> > +/* The written value is actually unused, all memory is compacted */
> > +int sysctl_compact_memory;
> >  /*
> >   * This is the entry point for compacting all nodes via
> >   * /proc/sys/vm/compact_memory
> > @@ -2770,8 +2772,16 @@ int compaction_proactiveness_sysctl_handler(struct ctl_table *table, int write,
> >  int sysctl_compaction_handler(struct ctl_table *table, int write,
> >  			void *buffer, size_t *length, loff_t *ppos)
> >  {
> > -	if (write)
> > +	int ret;
> > +
> > +	ret = proc_dointvec_minmax(table, write, buffer, length, ppos);
> > +	if (ret)
> > +		return ret;
> > +	if (write) {
> > +		pr_info("compact_nodes start\n");
> >  		compact_nodes();
> > +		pr_info("compact_nodes end\n");
> 
> I'm not sure we want to start spamming the dmesg. This would make sense
> if we wanted to deprecate the sysctl and start hunting for remaining
> callers to be fixed. Otherwise ftrace can be used to capture e.g. the time.

Without that print, I don't think a custom proc handler is needed too,
right? So what would simplify the code.

  Luis

Re: [PATCH V2 2/2] mm: compaction: Limit the value of interface compact_memory

[Vlastimil Babka]

On 3/8/23 20:57, Luis Chamberlain wrote:
> On Wed, Mar 08, 2023 at 11:23:45AM +0100, Vlastimil Babka wrote:
>> >  {
>> > -	if (write)
>> > +	int ret;
>> > +
>> > +	ret = proc_dointvec_minmax(table, write, buffer, length, ppos);
>> > +	if (ret)
>> > +		return ret;
>> > +	if (write) {
>> > +		pr_info("compact_nodes start\n");
>> >  		compact_nodes();
>> > +		pr_info("compact_nodes end\n");
>> 
>> I'm not sure we want to start spamming the dmesg. This would make sense
>> if we wanted to deprecate the sysctl and start hunting for remaining
>> callers to be fixed. Otherwise ftrace can be used to capture e.g. the time.
> 
> Without that print, I don't think a custom proc handler is needed too,
> right? So what would simplify the code.

But we'd still call compact_nodes(), so that's not possible without a custom
handler, no?

>   Luis

Re: [PATCH V2 2/2] mm: compaction: Limit the value of interface compact_memory

[Luis Chamberlain]

On Wed, Mar 08, 2023 at 10:54:31PM +0100, Vlastimil Babka wrote:
> On 3/8/23 20:57, Luis Chamberlain wrote:
> > On Wed, Mar 08, 2023 at 11:23:45AM +0100, Vlastimil Babka wrote:
> >> >  {
> >> > -	if (write)
> >> > +	int ret;
> >> > +
> >> > +	ret = proc_dointvec_minmax(table, write, buffer, length, ppos);
> >> > +	if (ret)
> >> > +		return ret;
> >> > +	if (write) {
> >> > +		pr_info("compact_nodes start\n");
> >> >  		compact_nodes();
> >> > +		pr_info("compact_nodes end\n");
> >> 
> >> I'm not sure we want to start spamming the dmesg. This would make sense
> >> if we wanted to deprecate the sysctl and start hunting for remaining
> >> callers to be fixed. Otherwise ftrace can be used to capture e.g. the time.
> > 
> > Without that print, I don't think a custom proc handler is needed too,
> > right? So what would simplify the code.
> 
> But we'd still call compact_nodes(), so that's not possible without a custom
> handler, no?

Ah right. It does beg the question if that form is common, so to define one.
But that's just extra work not needed now.

  Luis