[PATCH] mm, compaction: Indicate when compaction is manually triggered by sysctl

[PATCH] mm, compaction: Indicate when compaction is manually triggered by sysctl

[Guilherme G. Piccoli]

Currently we have no way to determine if compaction was triggered
by sysctl write, but this is an interesting information to have,
specially in systems with high uptime that presents lots of
fragmented memory. There's no statistic indicating if compaction
was triggered manually or ran by Linux itself, the vmstat numbers
cannot tell the user this information.

This patch adds a very simple message to kernel log when compaction
is requested through a write to sysctl file, and also it accumulates
the number of previously manual compaction executions. It follows
the approach used by drop_caches.

Signed-off-by: Guilherme G. Piccoli <gpiccoli@canonical.com>
---


This patch was based on linux-next/akpm branch, at d0f3f6070c3a.
Thanks in advance for reviews!
Cheers,

Guilherme


 mm/compaction.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index fb43e731ac31..80e748b0bbb6 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -2465,9 +2465,13 @@ int sysctl_compact_memory;
 int sysctl_compaction_handler(struct ctl_table *table, int write,
 			void *buffer, size_t *length, loff_t *ppos)
 {
-	if (write)
+	static unsigned compaction_acct;
+
+	if (write) {
+		pr_info("compact_memory triggered - it already previously ran %u times\n",
+			compaction_acct++);
 		compact_nodes();
-
+	}
 	return 0;
 }
 
-- 
2.25.2

Re: [PATCH] mm, compaction: Indicate when compaction is manually triggered by sysctl

[Andrew Morton]

On Thu,  7 May 2020 18:59:46 -0300 "Guilherme G. Piccoli" <gpiccoli@canonical.com> wrote:

> Currently we have no way to determine if compaction was triggered
> by sysctl write, but this is an interesting information to have,
> specially in systems with high uptime that presents lots of
> fragmented memory. There's no statistic indicating if compaction
> was triggered manually or ran by Linux itself, the vmstat numbers
> cannot tell the user this information.

Could add it to vmstat?

> This patch adds a very simple message to kernel log when compaction
> is requested through a write to sysctl file, and also it accumulates
> the number of previously manual compaction executions. It follows
> the approach used by drop_caches.

Userspace could write to /dev/kmsg when it decides to trigger
compaction?  Although using the kernel log seems a fairly lame way for
userspace to record its own actions...

Re: [PATCH] mm, compaction: Indicate when compaction is manually triggered by sysctl

[Guilherme G. Piccoli]

On Thu, May 7, 2020 at 8:04 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> Could add it to vmstat?

Hi Andrew, thanks for your suggestion! I thought the same, as a second
potential solution for this..was planning to add as a comment below
the "---" but forgot heheh
I agree that would be great in vmstat, do you have a name suggestion
for it? "compaction_triggered" maybe?


> Userspace could write to /dev/kmsg when it decides to trigger
> compaction?  Although using the kernel log seems a fairly lame way for
> userspace to record its own actions...
Well...you can think that the problem we are trying to solve was more
like...admin forgot if they triggered or not the compaction hehe
So, counting on the user to keep track of it is what I'd like to
avoid. And thinking about drop_caches (that have an indication when
triggered) makes me think this indeed does make sense for compaction
too.
Cheers,

Guilherme

Re: [PATCH] mm, compaction: Indicate when compaction is manually triggered by sysctl

[David Rientjes]

On Thu, 7 May 2020, Guilherme G. Piccoli wrote:

> Well...you can think that the problem we are trying to solve was more
> like...admin forgot if they triggered or not the compaction hehe
> So, counting on the user to keep track of it is what I'd like to
> avoid. And thinking about drop_caches (that have an indication when
> triggered) makes me think this indeed does make sense for compaction
> too.
> Cheers,
> 

It doesn't make sense because it's only being done here for the entire 
system, there are also per-node sysfs triggers so you could do something 
like iterate over the nodemask of all nodes with memory and trigger 
compaction manually and then nothing is emitted to the kernel log.

There is new statsfs support that Red Hat is proposing that can be used 
for things like this.  It currently only supports KVM statistics but 
adding MM statistics is something that would be a natural extension and 
avoids polluting both the kernel log and /proc/vmstat.

Re: [PATCH] mm, compaction: Indicate when compaction is manually triggered by sysctl

[Guilherme Piccoli]

On Fri, May 8, 2020 at 3:31 PM David Rientjes <rientjes@google.com> wrote:
> It doesn't make sense because it's only being done here for the entire
> system, there are also per-node sysfs triggers so you could do something
> like iterate over the nodemask of all nodes with memory and trigger
> compaction manually and then nothing is emitted to the kernel log.
>
> There is new statsfs support that Red Hat is proposing that can be used
> for things like this.  It currently only supports KVM statistics but
> adding MM statistics is something that would be a natural extension and
> avoids polluting both the kernel log and /proc/vmstat.

Thanks for the review. Is this what you're talking about [0] ? Very interesting!

Also, I agree about the per-node compaction, it's a good point. But at
the same time, having the information on the number of manual
compaction triggered is interesting, at least for some users. What if
we add that as a per-node stat in zoneinfo?

Cheers,

Guilherme


[0] lore.kernel.org/kvm/20200427141816.16703-1-eesposit@redhat.com

Re: [PATCH] mm, compaction: Indicate when compaction is manually triggered by sysctl

[David Rientjes]

On Fri, 8 May 2020, Guilherme Piccoli wrote:

> On Fri, May 8, 2020 at 3:31 PM David Rientjes <rientjes@google.com> wrote:
> > It doesn't make sense because it's only being done here for the entire
> > system, there are also per-node sysfs triggers so you could do something
> > like iterate over the nodemask of all nodes with memory and trigger
> > compaction manually and then nothing is emitted to the kernel log.
> >
> > There is new statsfs support that Red Hat is proposing that can be used
> > for things like this.  It currently only supports KVM statistics but
> > adding MM statistics is something that would be a natural extension and
> > avoids polluting both the kernel log and /proc/vmstat.
> 
> Thanks for the review. Is this what you're talking about [0] ? Very interesting!
> 

Exactly.

> Also, I agree about the per-node compaction, it's a good point. But at
> the same time, having the information on the number of manual
> compaction triggered is interesting, at least for some users. What if
> we add that as a per-node stat in zoneinfo?
> 

The kernel log is not preferred for this (or drop_caches, really) because 
the amount of info can causing important information to be lost.  We don't 
really gain anything by printing that someone manually triggered 
compaction; they could just write to the kernel log themselves if they 
really wanted to.  The reverse is not true: we can't suppress your kernel 
message with this patch.

Instead, a statsfs-like approach could be used to indicate when this has 
happened and there is no chance of losing events because it got scrolled 
off the kernel log.  It has the added benefit of not requiring the entire 
log to be parsed for such events.

Re: [PATCH] mm, compaction: Indicate when compaction is manually triggered by sysctl

[Guilherme Piccoli]

On Sun, May 10, 2020 at 10:25 PM David Rientjes <rientjes@google.com> wrote:
> [...]
> The kernel log is not preferred for this (or drop_caches, really) because
> the amount of info can causing important information to be lost.  We don't
> really gain anything by printing that someone manually triggered
> compaction; they could just write to the kernel log themselves if they
> really wanted to.  The reverse is not true: we can't suppress your kernel
> message with this patch.
>
> Instead, a statsfs-like approach could be used to indicate when this has
> happened and there is no chance of losing events because it got scrolled
> off the kernel log.  It has the added benefit of not requiring the entire
> log to be parsed for such events.

OK, agreed! Let's forget the kernel log. So, do you think the way to
go is the statsfs, not a zoneinfo stat, a per-node thing? I'm saying
that because kernel mm subsystem statistics seem pretty.."comfortable"
the way they are, in files like vmstat, zoneinfo, etc. Let me know
your thoughts on this, if I could work on that or should wait statsfs.
Thanks,


Guilherme

Re: [PATCH] mm, compaction: Indicate when compaction is manually triggered by sysctl

[peter enderborg]

On 5/11/20 1:26 PM, Guilherme Piccoli wrote:
> On Sun, May 10, 2020 at 10:25 PM David Rientjes <rientjes@google.com> wrote:
>> [...]
>> The kernel log is not preferred for this (or drop_caches, really) because
>> the amount of info can causing important information to be lost.  We don't
>> really gain anything by printing that someone manually triggered
>> compaction; they could just write to the kernel log themselves if they
>> really wanted to.  The reverse is not true: we can't suppress your kernel
>> message with this patch.
>>
>> Instead, a statsfs-like approach could be used to indicate when this has
>> happened and there is no chance of losing events because it got scrolled
>> off the kernel log.  It has the added benefit of not requiring the entire
>> log to be parsed for such events.
> OK, agreed! Let's forget the kernel log. So, do you think the way to
> go is the statsfs, not a zoneinfo stat, a per-node thing? I'm saying
> that because kernel mm subsystem statistics seem pretty.."comfortable"
> the way they are, in files like vmstat, zoneinfo, etc. Let me know
> your thoughts on this, if I could work on that or should wait statsfs.
> Thanks,
>
>
> Guilherme

I think a trace notification in compaction like kcompad_wake would be good.

Re: [PATCH] mm, compaction: Indicate when compaction is manually triggered by sysctl

[Guilherme Piccoli]

Hi Peter, thanks for the feedback. What do you mean by "trace
notification" ? We seem to have a trace event in that function you
mentioned. Also, accounting for that function is enough to
differentiate when the compaction is triggered by the kernel itself or
by the user (which is our use case here) ?

Cheers,


Guilherme

Re: [PATCH] mm, compaction: Indicate when compaction is manually triggered by sysctl

[Enderborg, Peter]

On 5/18/20 2:14 PM, Guilherme Piccoli wrote:
> Hi Peter, thanks for the feedback. What do you mean by "trace
> notification" ? We seem to have a trace event in that function you
> mentioned. Also, accounting for that function is enough to
> differentiate when the compaction is triggered by the kernel itself or
> by the user (which is our use case here) ?

Usually change existing causes confusion. It should not be a problem but it happen.


> Cheers,
>
>
> Guilherme

Re: [PATCH] mm, compaction: Indicate when compaction is manually triggered by sysctl

[Guilherme Piccoli]

On Mon, May 18, 2020 at 9:54 AM Enderborg, Peter
<Peter.Enderborg@sony.com> wrote:
> Usually change existing causes confusion. It should not be a problem but it happen.
>

I am sorry, but I really didn't understand your statement, can you be
more specific?
Thanks again!