2013-07-26 12:45:28

by Michal Hocko

[permalink] [raw]
Subject: [PATCH resend] drop_caches: add some documentation and info message

I would like to resurrect Dave's patch. It was originally posted here
https://lkml.org/lkml/2010/9/16/250 and I have resurrected it here
https://lkml.org/lkml/2012/10/12/175 for the first time. There didn't
seem to be any strong opposition but the patch has been dropped later
from the mm tree.

To summarize concerns:
Kosaki was worried about possible excessive logging when somebody drops
caches too often (but then he claimed he didn't have a strong opinion on
that) and later acked the patch (https://lkml.org/lkml/2012/10/12/350).
I would even dare to say opposite. If somebody drops caches too often
then I would really like to know that from the log when supporting a
system because it almost for sure means that there is something fishy
going on. It is also worth mentioning that only root can write drop
caches so this is not an flooding attack vector.

Andrew was worried (http://lkml.indiana.edu/hypermail/linux/kernel/1210.3/00605.html)
about people hating us because they are using this as a solution to
their issues. I concur that most of those are just hacks that found
their way into scripts looong time agon and stayed there. We should
rather not feed these cargo cults and rather fix the real bugs. History
has been showing us that users are usually getting rid of old hacks when
something starts screeming at them. So let's screem.

Boris then noted (http://lkml.indiana.edu/hypermail/linux/kernel/1210.3/00659.html)
that he is using drop_caches to make s2ram faster but as others noted
this just adds the overhead to the resume path so it might work only for
certain use cases. Having a low priority message under such conditions
shouldn't such a big deal.

I am bringing the patch up again because this has proved being really
helpful when chasing strange performance issues which (surprise
surprise) turn out to be related to artificially dropped caches done
because the admin thinks this would help... So mostly those who support
machines which are not in their hands would benefit from such a change.

I have just refreshed the original patch on top of the current mm tree
and lowered priority to KERN_INFO to make the message less hysterical.

: From: Dave Hansen <[email protected]>
: Date: Fri, 12 Oct 2012 14:30:54 +0200
:
: There is plenty of anecdotal evidence and a load of blog posts
: suggesting that using "drop_caches" periodically keeps your system
: running in "tip top shape". Perhaps adding some kernel
: documentation will increase the amount of accurate data on its use.
:
: If we are not shrinking caches effectively, then we have real bugs.
: Using drop_caches will simply mask the bugs and make them harder
: to find, but certainly does not fix them, nor is it an appropriate
: "workaround" to limit the size of the caches.
:
: It's a great debugging tool, and is really handy for doing things
: like repeatable benchmark runs. So, add a bit more documentation
: about it, and add a little KERN_NOTICE. It should help developers
: who are chasing down reclaim-related bugs.

[[email protected]: refreshed to current -mm tree]
[[email protected]: checkpatch fixes]
Signed-off-by: Dave Hansen <[email protected]>
Signed-off-by: Michal Hocko <[email protected]>
Acked-by: KOSAKI Motohiro <[email protected]>
Acked-by: KAMEZAWA Hiroyuki <[email protected]>
---
Documentation/sysctl/vm.txt | 33 +++++++++++++++++++++++++++------
fs/drop_caches.c | 2 ++
2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/Documentation/sysctl/vm.txt b/Documentation/sysctl/vm.txt
index 36ecc26..15d341a 100644
--- a/Documentation/sysctl/vm.txt
+++ b/Documentation/sysctl/vm.txt
@@ -169,18 +169,39 @@ Setting this to zero disables periodic writeback altogether.

drop_caches

-Writing to this will cause the kernel to drop clean caches, dentries and
-inodes from memory, causing that memory to become free.
+Writing to this will cause the kernel to drop clean caches, as well as
+reclaimable slab objects like dentries and inodes. Once dropped, their
+memory becomes free.

To free pagecache:
echo 1 > /proc/sys/vm/drop_caches
-To free dentries and inodes:
+To free reclaimable slab objects (includes dentries and inodes):
echo 2 > /proc/sys/vm/drop_caches
-To free pagecache, dentries and inodes:
+To free slab objects and pagecache:
echo 3 > /proc/sys/vm/drop_caches

-As this is a non-destructive operation and dirty objects are not freeable, the
-user should run `sync' first.
+This is a non-destructive operation and will not free any dirty objects.
+To increase the number of objects freed by this operation, the user may run
+`sync' prior to writing to /proc/sys/vm/drop_caches. This will minimize the
+number of dirty objects on the system and create more candidates to be
+dropped.
+
+This file is not a means to control the growth of the various kernel caches
+(inodes, dentries, pagecache, etc...) These objects are automatically
+reclaimed by the kernel when memory is needed elsewhere on the system.
+
+Use of this file can cause performance problems. Since it discards cached
+objects, it may cost a significant amount of I/O and CPU to recreate the
+dropped objects, especially if they were under heavy use. Because of this,
+use outside of a testing or debugging environment is not recommended.
+
+You may see informational messages in your kernel log when this file is
+used:
+
+ cat (1234): dropped kernel caches: 3
+
+These are informational only. They do not mean that anything is wrong
+with your system.

==============================================================

diff --git a/fs/drop_caches.c b/fs/drop_caches.c
index 9fd702f..c3f44e7 100644
--- a/fs/drop_caches.c
+++ b/fs/drop_caches.c
@@ -59,6 +59,8 @@ int drop_caches_sysctl_handler(ctl_table *table, int write,
if (ret)
return ret;
if (write) {
+ printk(KERN_INFO "%s (%d): dropped kernel caches: %d\n",
+ current->comm, task_pid_nr(current), sysctl_drop_caches);
if (sysctl_drop_caches & 1)
iterate_supers(drop_pagecache_sb, NULL);
if (sysctl_drop_caches & 2)
--
1.8.3.2


2013-07-26 21:38:02

by Johannes Weiner

[permalink] [raw]
Subject: Re: [PATCH resend] drop_caches: add some documentation and info message

On Fri, Jul 26, 2013 at 02:44:29PM +0200, Michal Hocko wrote:
> I would like to resurrect Dave's patch. It was originally posted here
> https://lkml.org/lkml/2010/9/16/250 and I have resurrected it here
> https://lkml.org/lkml/2012/10/12/175 for the first time. There didn't
> seem to be any strong opposition but the patch has been dropped later
> from the mm tree.
>
> To summarize concerns:
> Kosaki was worried about possible excessive logging when somebody drops
> caches too often (but then he claimed he didn't have a strong opinion on
> that) and later acked the patch (https://lkml.org/lkml/2012/10/12/350).
> I would even dare to say opposite. If somebody drops caches too often
> then I would really like to know that from the log when supporting a
> system because it almost for sure means that there is something fishy
> going on. It is also worth mentioning that only root can write drop
> caches so this is not an flooding attack vector.

Agreed.

> Andrew was worried (http://lkml.indiana.edu/hypermail/linux/kernel/1210.3/00605.html)
> about people hating us because they are using this as a solution to
> their issues. I concur that most of those are just hacks that found
> their way into scripts looong time agon and stayed there. We should
> rather not feed these cargo cults and rather fix the real bugs. History
> has been showing us that users are usually getting rid of old hacks when
> something starts screeming at them. So let's screem.

Agreed. The whole point of this is to be a pain in the ass in order
to establish a feedback loop.

> Boris then noted (http://lkml.indiana.edu/hypermail/linux/kernel/1210.3/00659.html)
> that he is using drop_caches to make s2ram faster but as others noted
> this just adds the overhead to the resume path so it might work only for
> certain use cases. Having a low priority message under such conditions
> shouldn't such a big deal.

A oneliner like this should drown in the overall noise of the
suspend-resume path.

> I am bringing the patch up again because this has proved being really
> helpful when chasing strange performance issues which (surprise
> surprise) turn out to be related to artificially dropped caches done
> because the admin thinks this would help... So mostly those who support
> machines which are not in their hands would benefit from such a change.
>
> I have just refreshed the original patch on top of the current mm tree
> and lowered priority to KERN_INFO to make the message less hysterical.
>
> : From: Dave Hansen <[email protected]>
> : Date: Fri, 12 Oct 2012 14:30:54 +0200
> :
> : There is plenty of anecdotal evidence and a load of blog posts
> : suggesting that using "drop_caches" periodically keeps your system
> : running in "tip top shape". Perhaps adding some kernel
> : documentation will increase the amount of accurate data on its use.
> :
> : If we are not shrinking caches effectively, then we have real bugs.
> : Using drop_caches will simply mask the bugs and make them harder
> : to find, but certainly does not fix them, nor is it an appropriate
> : "workaround" to limit the size of the caches.
> :
> : It's a great debugging tool, and is really handy for doing things
> : like repeatable benchmark runs. So, add a bit more documentation
> : about it, and add a little KERN_NOTICE. It should help developers
> : who are chasing down reclaim-related bugs.
>
> [[email protected]: refreshed to current -mm tree]
> [[email protected]: checkpatch fixes]
> Signed-off-by: Dave Hansen <[email protected]>
> Signed-off-by: Michal Hocko <[email protected]>
> Acked-by: KOSAKI Motohiro <[email protected]>
> Acked-by: KAMEZAWA Hiroyuki <[email protected]>

Acked-by: Johannes Weiner <[email protected]>

2013-07-29 20:57:53

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH resend] drop_caches: add some documentation and info message

On Fri, 26 Jul 2013 14:44:29 +0200 Michal Hocko <[email protected]> wrote:

> ...
>
> : There is plenty of anecdotal evidence and a load of blog posts
> : suggesting that using "drop_caches" periodically keeps your system
> : running in "tip top shape". Perhaps adding some kernel
> : documentation will increase the amount of accurate data on its use.
> :
> : If we are not shrinking caches effectively, then we have real bugs.
> : Using drop_caches will simply mask the bugs and make them harder
> : to find, but certainly does not fix them, nor is it an appropriate
> : "workaround" to limit the size of the caches.
> :
> : It's a great debugging tool, and is really handy for doing things
> : like repeatable benchmark runs. So, add a bit more documentation
> : about it, and add a little KERN_NOTICE. It should help developers
> : who are chasing down reclaim-related bugs.
>
> ...
>
> --- a/fs/drop_caches.c
> +++ b/fs/drop_caches.c
> @@ -59,6 +59,8 @@ int drop_caches_sysctl_handler(ctl_table *table, int write,
> if (ret)
> return ret;
> if (write) {
> + printk(KERN_INFO "%s (%d): dropped kernel caches: %d\n",
> + current->comm, task_pid_nr(current), sysctl_drop_caches);
> if (sysctl_drop_caches & 1)
> iterate_supers(drop_pagecache_sb, NULL);
> if (sysctl_drop_caches & 2)

How about we do

if (!(sysctl_drop_caches & 4))
printk(....)

so people can turn it off if it's causing problems?

2013-07-30 07:45:36

by Michal Hocko

[permalink] [raw]
Subject: Re: [PATCH resend] drop_caches: add some documentation and info message

On Mon 29-07-13 13:57:43, Andrew Morton wrote:
> On Fri, 26 Jul 2013 14:44:29 +0200 Michal Hocko <[email protected]> wrote:
[...]
> > --- a/fs/drop_caches.c
> > +++ b/fs/drop_caches.c
> > @@ -59,6 +59,8 @@ int drop_caches_sysctl_handler(ctl_table *table, int write,
> > if (ret)
> > return ret;
> > if (write) {
> > + printk(KERN_INFO "%s (%d): dropped kernel caches: %d\n",
> > + current->comm, task_pid_nr(current), sysctl_drop_caches);
> > if (sysctl_drop_caches & 1)
> > iterate_supers(drop_pagecache_sb, NULL);
> > if (sysctl_drop_caches & 2)
>
> How about we do
>
> if (!(sysctl_drop_caches & 4))
> printk(....)
>
> so people can turn it off if it's causing problems?

I am OK with that but can we use a top bit instead. Maybe we never have
other entities to drop in the future but it would be better to have a room for them
just in case. So what about using 1<<31 instead?


--
Michal Hocko
SUSE Labs

2013-07-30 08:26:22

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH resend] drop_caches: add some documentation and info message

On Tue, 30 Jul 2013 09:45:31 +0200 Michal Hocko <[email protected]> wrote:

> On Mon 29-07-13 13:57:43, Andrew Morton wrote:
> > On Fri, 26 Jul 2013 14:44:29 +0200 Michal Hocko <[email protected]> wrote:
> [...]
> > > --- a/fs/drop_caches.c
> > > +++ b/fs/drop_caches.c
> > > @@ -59,6 +59,8 @@ int drop_caches_sysctl_handler(ctl_table *table, int write,
> > > if (ret)
> > > return ret;
> > > if (write) {
> > > + printk(KERN_INFO "%s (%d): dropped kernel caches: %d\n",
> > > + current->comm, task_pid_nr(current), sysctl_drop_caches);
> > > if (sysctl_drop_caches & 1)
> > > iterate_supers(drop_pagecache_sb, NULL);
> > > if (sysctl_drop_caches & 2)
> >
> > How about we do
> >
> > if (!(sysctl_drop_caches & 4))
> > printk(....)
> >
> > so people can turn it off if it's causing problems?
>
> I am OK with that but can we use a top bit instead. Maybe we never have
> other entities to drop in the future but it would be better to have a room for them
> just in case.

If we add another flag in the future it can use bit 3?

> So what about using 1<<31 instead?

Could, but negative (or is it positive?) numbers are a bit of a pain.

2013-07-30 12:55:30

by Michal Hocko

[permalink] [raw]
Subject: Re: [PATCH resend] drop_caches: add some documentation and info message

On Tue 30-07-13 01:25:44, Andrew Morton wrote:
> On Tue, 30 Jul 2013 09:45:31 +0200 Michal Hocko <[email protected]> wrote:
>
> > On Mon 29-07-13 13:57:43, Andrew Morton wrote:
> > > On Fri, 26 Jul 2013 14:44:29 +0200 Michal Hocko <[email protected]> wrote:
> > [...]
> > > > --- a/fs/drop_caches.c
> > > > +++ b/fs/drop_caches.c
> > > > @@ -59,6 +59,8 @@ int drop_caches_sysctl_handler(ctl_table *table, int write,
> > > > if (ret)
> > > > return ret;
> > > > if (write) {
> > > > + printk(KERN_INFO "%s (%d): dropped kernel caches: %d\n",
> > > > + current->comm, task_pid_nr(current), sysctl_drop_caches);
> > > > if (sysctl_drop_caches & 1)
> > > > iterate_supers(drop_pagecache_sb, NULL);
> > > > if (sysctl_drop_caches & 2)
> > >
> > > How about we do
> > >
> > > if (!(sysctl_drop_caches & 4))
> > > printk(....)
> > >
> > > so people can turn it off if it's causing problems?
> >
> > I am OK with that but can we use a top bit instead. Maybe we never have
> > other entities to drop in the future but it would be better to have a room for them
> > just in case.
>
> If we add another flag in the future it can use bit 3?

What if we get crazy and need more of them?

> > So what about using 1<<31 instead?
>
> Could, but negative (or is it positive?) numbers are a bit of a pain.

Yes, that was the point ;), I would like to make a new usage a dance on
the meadows.
But I do not care much, let's use 1<<30 if negative sounds too bad but I
would leave some room for further entities.

--
Michal Hocko
SUSE Labs

2013-07-30 14:40:35

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH resend] drop_caches: add some documentation and info message

On Tue, 30 Jul 2013 14:55:25 +0200 Michal Hocko <[email protected]> wrote:

> > > I am OK with that but can we use a top bit instead. Maybe we never have
> > > other entities to drop in the future but it would be better to have a room for them
> > > just in case.
> >
> > If we add another flag in the future it can use bit 3?
>
> What if we get crazy and need more of them?

Then we use bit 4. Then 5. Then 6.

I'm really not understanding your point here ;)

2013-07-30 14:47:18

by Michal Hocko

[permalink] [raw]
Subject: Re: [PATCH resend] drop_caches: add some documentation and info message

On Tue 30-07-13 07:39:56, Andrew Morton wrote:
> On Tue, 30 Jul 2013 14:55:25 +0200 Michal Hocko <[email protected]> wrote:
>
> > > > I am OK with that but can we use a top bit instead. Maybe we never have
> > > > other entities to drop in the future but it would be better to have a room for them
> > > > just in case.
> > >
> > > If we add another flag in the future it can use bit 3?
> >
> > What if we get crazy and need more of them?
>
> Then we use bit 4. Then 5. Then 6.
>
> I'm really not understanding your point here ;)

I meant let's keep entities to drop in the low bits and the mode of
printing in the top so they are separated.

--
Michal Hocko
SUSE Labs

2013-07-30 14:47:30

by Dave Hansen

[permalink] [raw]
Subject: Re: [PATCH resend] drop_caches: add some documentation and info message

On 07/30/2013 05:55 AM, Michal Hocko wrote:
>> > If we add another flag in the future it can use bit 3?
> What if we get crazy and need more of them?

I really hate using bits for these kinds of interfaces. I'm forgetful
and never remember which bit is which, and they're possible to run out of.

I'm not saying do it now, but we can switch over to:

echo 'slab|pagecache' > drop_caches
or
echo 'quiet|slab' > drop_caches

any time we want and still have compatibility with the existing bitwise
interface.

2013-07-30 14:54:11

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH resend] drop_caches: add some documentation and info message

On Tue, Jul 30, 2013 at 07:47:12AM -0700, Dave Hansen wrote:
> On 07/30/2013 05:55 AM, Michal Hocko wrote:
> >> > If we add another flag in the future it can use bit 3?
> > What if we get crazy and need more of them?
>
> I really hate using bits for these kinds of interfaces. I'm forgetful
> and never remember which bit is which, and they're possible to run out of.
>
> I'm not saying do it now, but we can switch over to:
>
> echo 'slab|pagecache' > drop_caches
> or
> echo 'quiet|slab' > drop_caches
>
> any time we want and still have compatibility with the existing bitwise
> interface.

Hohum, definitely a nice idea at a first glance. And when you cat
drop_caches, it could show you what all those commands mean and how you
can issue them, i.e. allowed syntax etc.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

2013-07-30 15:08:50

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH resend] drop_caches: add some documentation and info message

On Tue, 30 Jul 2013 07:47:12 -0700 Dave Hansen <[email protected]> wrote:

> On 07/30/2013 05:55 AM, Michal Hocko wrote:
> >> > If we add another flag in the future it can use bit 3?
> > What if we get crazy and need more of them?
>
> I really hate using bits for these kinds of interfaces. I'm forgetful
> and never remember which bit is which, and they're possible to run out of.
>
> I'm not saying do it now, but we can switch over to:
>
> echo 'slab|pagecache' > drop_caches
> or
> echo 'quiet|slab' > drop_caches
>
> any time we want and still have compatibility with the existing bitwise
> interface.

/bin/drop_caches "slab|pagecache"

That we always insist on doing such things in the kernel instead is
exhibit #1 in Why We Suck.

2013-08-01 03:12:04

by KOSAKI Motohiro

[permalink] [raw]
Subject: Re: [PATCH resend] drop_caches: add some documentation and info message

>> --- a/fs/drop_caches.c
>> +++ b/fs/drop_caches.c
>> @@ -59,6 +59,8 @@ int drop_caches_sysctl_handler(ctl_table *table, int write,
>> if (ret)
>> return ret;
>> if (write) {
>> + printk(KERN_INFO "%s (%d): dropped kernel caches: %d\n",
>> + current->comm, task_pid_nr(current), sysctl_drop_caches);
>> if (sysctl_drop_caches & 1)
>> iterate_supers(drop_pagecache_sb, NULL);
>> if (sysctl_drop_caches & 2)
>
> How about we do
>
> if (!(sysctl_drop_caches & 4))
> printk(....)
>
> so people can turn it off if it's causing problems?

The best interface depends on the purpose. If you want to detect crazy application,
we can't assume an application co-operate us. So, I doubt this works.

2013-08-01 03:17:59

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH resend] drop_caches: add some documentation and info message

On Wed, 31 Jul 2013 23:11:50 -0400 KOSAKI Motohiro <[email protected]> wrote:

> >> --- a/fs/drop_caches.c
> >> +++ b/fs/drop_caches.c
> >> @@ -59,6 +59,8 @@ int drop_caches_sysctl_handler(ctl_table *table, int write,
> >> if (ret)
> >> return ret;
> >> if (write) {
> >> + printk(KERN_INFO "%s (%d): dropped kernel caches: %d\n",
> >> + current->comm, task_pid_nr(current), sysctl_drop_caches);
> >> if (sysctl_drop_caches & 1)
> >> iterate_supers(drop_pagecache_sb, NULL);
> >> if (sysctl_drop_caches & 2)
> >
> > How about we do
> >
> > if (!(sysctl_drop_caches & 4))
> > printk(....)
> >
> > so people can turn it off if it's causing problems?
>
> The best interface depends on the purpose. If you want to detect crazy application,
> we can't assume an application co-operate us. So, I doubt this works.

You missed the "!". I'm proposing that setting the new bit 2 will
permit people to prevent the new printk if it is causing them problems.

2013-08-02 01:40:11

by KOSAKI Motohiro

[permalink] [raw]
Subject: Re: [PATCH resend] drop_caches: add some documentation and info message

On Wed, Jul 31, 2013 at 11:17 PM, Andrew Morton
<[email protected]> wrote:
> On Wed, 31 Jul 2013 23:11:50 -0400 KOSAKI Motohiro <[email protected]> wrote:
>
>> >> --- a/fs/drop_caches.c
>> >> +++ b/fs/drop_caches.c
>> >> @@ -59,6 +59,8 @@ int drop_caches_sysctl_handler(ctl_table *table, int write,
>> >> if (ret)
>> >> return ret;
>> >> if (write) {
>> >> + printk(KERN_INFO "%s (%d): dropped kernel caches: %d\n",
>> >> + current->comm, task_pid_nr(current), sysctl_drop_caches);
>> >> if (sysctl_drop_caches & 1)
>> >> iterate_supers(drop_pagecache_sb, NULL);
>> >> if (sysctl_drop_caches & 2)
>> >
>> > How about we do
>> >
>> > if (!(sysctl_drop_caches & 4))
>> > printk(....)
>> >
>> > so people can turn it off if it's causing problems?
>>
>> The best interface depends on the purpose. If you want to detect crazy application,
>> we can't assume an application co-operate us. So, I doubt this works.
>
> You missed the "!". I'm proposing that setting the new bit 2 will
> permit people to prevent the new printk if it is causing them problems.

No I don't. I'm sure almost all abuse users think our usage is correct. Then,
I can imagine all crazy applications start to use this flag eventually.

2013-08-02 07:33:09

by Michal Hocko

[permalink] [raw]
Subject: Re: [PATCH resend] drop_caches: add some documentation and info message

On Thu 01-08-13 21:39:48, KOSAKI Motohiro wrote:
> On Wed, Jul 31, 2013 at 11:17 PM, Andrew Morton
> <[email protected]> wrote:
> > On Wed, 31 Jul 2013 23:11:50 -0400 KOSAKI Motohiro <[email protected]> wrote:
> >
> >> >> --- a/fs/drop_caches.c
> >> >> +++ b/fs/drop_caches.c
> >> >> @@ -59,6 +59,8 @@ int drop_caches_sysctl_handler(ctl_table *table, int write,
> >> >> if (ret)
> >> >> return ret;
> >> >> if (write) {
> >> >> + printk(KERN_INFO "%s (%d): dropped kernel caches: %d\n",
> >> >> + current->comm, task_pid_nr(current), sysctl_drop_caches);
> >> >> if (sysctl_drop_caches & 1)
> >> >> iterate_supers(drop_pagecache_sb, NULL);
> >> >> if (sysctl_drop_caches & 2)
> >> >
> >> > How about we do
> >> >
> >> > if (!(sysctl_drop_caches & 4))
> >> > printk(....)
> >> >
> >> > so people can turn it off if it's causing problems?
> >>
> >> The best interface depends on the purpose. If you want to detect crazy application,
> >> we can't assume an application co-operate us. So, I doubt this works.
> >
> > You missed the "!". I'm proposing that setting the new bit 2 will
> > permit people to prevent the new printk if it is causing them problems.
>
> No I don't. I'm sure almost all abuse users think our usage is correct. Then,
> I can imagine all crazy applications start to use this flag eventually.

I guess we do not care about those. If somebody wants to shoot his feet
then we cannot do much about it. The primary motivation was to find out
those that think this is right and they are willing to change the setup
once they know this is not the right way to do things.

I think that giving a way to suppress the warning is a good step. Log
level might be to coarse and sysctl would be an overkill.

--
Michal Hocko
SUSE Labs

2013-08-02 16:04:10

by Rob Landley

[permalink] [raw]
Subject: Re: [PATCH resend] drop_caches: add some documentation and info message

On 07/31/2013 10:17:08 PM, Andrew Morton wrote:
> On Wed, 31 Jul 2013 23:11:50 -0400 KOSAKI Motohiro
> <[email protected]> wrote:
>
> > >> --- a/fs/drop_caches.c
> > >> +++ b/fs/drop_caches.c
> > >> @@ -59,6 +59,8 @@ int drop_caches_sysctl_handler(ctl_table
> *table, int write,
> > >> if (ret)
> > >> return ret;
> > >> if (write) {
> > >> + printk(KERN_INFO "%s (%d): dropped kernel
> caches: %d\n",
> > >> + current->comm, task_pid_nr(current),
> sysctl_drop_caches);
> > >> if (sysctl_drop_caches & 1)
> > >> iterate_supers(drop_pagecache_sb, NULL);
> > >> if (sysctl_drop_caches & 2)
> > >
> > > How about we do
> > >
> > > if (!(sysctl_drop_caches & 4))
> > > printk(....)
> > >
> > > so people can turn it off if it's causing problems?
> >
> > The best interface depends on the purpose. If you want to detect
> crazy application,
> > we can't assume an application co-operate us. So, I doubt this
> works.
>
> You missed the "!". I'm proposing that setting the new bit 2 will
> permit people to prevent the new printk if it is causing them
> problems.

Or an alternative for those planning to patch it down to a KERN_DEBUG
locally.

I'd be surprised if anybody who does this sees the printk and thinks
"hey, I'll dig into the VM's balancing logic and come up to speed on
the tradeoffs sufficient to contribute to kernel development" because
of something in dmesg. Anybody actually annoyed by it will chop out the
printk (you barely need to know C to do that), the rest won't notice.

Rob-

2013-08-02 17:10:49

by Dave Hansen

[permalink] [raw]
Subject: Re: [PATCH resend] drop_caches: add some documentation and info message

On 08/02/2013 09:04 AM, Rob Landley wrote:
> I'd be surprised if anybody who does this sees the printk and thinks
> "hey, I'll dig into the VM's balancing logic and come up to speed on the
> tradeoffs sufficient to contribute to kernel development" because of
> something in dmesg. Anybody actually annoyed by it will chop out the
> printk (you barely need to know C to do that), the rest won't notice.

All that I expect is that this will get _some_ of these folks in to a
feedback loop with us. They'll see this in dmesg and either go asking
questions within their respective companies, file bugs with distros, or
post to LKML.

Some of them are going to say things like "My Database Vendor told me
this optimizes my server!", or that the documentation told them to do it
so they don't run out of memory. Some of them might even be running in
to _legitimate_ VM or filesystem bugs that they're working around with this.

2013-08-03 20:17:11

by KOSAKI Motohiro

[permalink] [raw]
Subject: Re: [PATCH resend] drop_caches: add some documentation and info message

>>> You missed the "!". I'm proposing that setting the new bit 2 will
>>> permit people to prevent the new printk if it is causing them problems.
>>
>> No I don't. I'm sure almost all abuse users think our usage is correct. Then,
>> I can imagine all crazy applications start to use this flag eventually.
>
> I guess we do not care about those. If somebody wants to shoot his feet
> then we cannot do much about it. The primary motivation was to find out
> those that think this is right and they are willing to change the setup
> once they know this is not the right way to do things.
>
> I think that giving a way to suppress the warning is a good step. Log
> level might be to coarse and sysctl would be an overkill.

When Dave Hansen reported this issue originally, he explained a lot of userland
developer misuse /proc/drop_caches because they don't understand what
drop_caches do.
So, if they never understand the fact, why can we trust them? I have no
idea.
Or, if you have different motivation w/ Dave, please let me know it.

While the purpose is to shoot misuse, I don't think we can trust userland app.
If "If somebody wants to shoot his feet then we cannot do much about it." is true,
this patch is useless. OK, we still catch the right user. But we never want to know
who is the right users, right?

2013-08-04 08:08:01

by Michal Hocko

[permalink] [raw]
Subject: Re: [PATCH resend] drop_caches: add some documentation and info message

On Sat 03-08-13 16:16:58, KOSAKI Motohiro wrote:
> >>> You missed the "!". I'm proposing that setting the new bit 2 will
> >>> permit people to prevent the new printk if it is causing them problems.
> >>
> >> No I don't. I'm sure almost all abuse users think our usage is correct. Then,
> >> I can imagine all crazy applications start to use this flag eventually.
> >
> > I guess we do not care about those. If somebody wants to shoot his feet
> > then we cannot do much about it. The primary motivation was to find out
> > those that think this is right and they are willing to change the setup
> > once they know this is not the right way to do things.
> >
> > I think that giving a way to suppress the warning is a good step. Log
> > level might be to coarse and sysctl would be an overkill.
>
> When Dave Hansen reported this issue originally, he explained a lot of userland
> developer misuse /proc/drop_caches because they don't understand what
> drop_caches do.
> So, if they never understand the fact, why can we trust them? I have no
> idea.

Well, most of that usage I have come across was legacy scripts which
happened to work at a certain point in time because we sucked.
Thinks have changed but such scripts happen to survive a long time.
We are primarily interested in those.

> Or, if you have different motivation w/ Dave, please let me know it.

We have seen reports where users complained about performance drop down
when in fact the real culprit turned out to be such a clever script
which dropped caches on the background thinking it will help to free
some memory. Such cases are tedious to reveal.

> While the purpose is to shoot misuse, I don't think we can trust
> userland app. If "If somebody wants to shoot his feet then we cannot
> do much about it." is true, this patch is useless. OK, we still catch
> the right user.

I do not think it is useless. It will print a message for all those
users initially. It is a matter of user how to deal with it.

> But we never want to know who is the right users, right?

Well, those that are curious about a new message in the lock and come
back to us asking what is going on are those we are primarily interested
in.
--
Michal Hocko
SUSE Labs

2013-08-05 01:14:07

by KOSAKI Motohiro

[permalink] [raw]
Subject: Re: [PATCH resend] drop_caches: add some documentation and info message

On Sun, Aug 4, 2013 at 4:07 AM, Michal Hocko <[email protected]> wrote:
> On Sat 03-08-13 16:16:58, KOSAKI Motohiro wrote:
>> >>> You missed the "!". I'm proposing that setting the new bit 2 will
>> >>> permit people to prevent the new printk if it is causing them problems.
>> >>
>> >> No I don't. I'm sure almost all abuse users think our usage is correct. Then,
>> >> I can imagine all crazy applications start to use this flag eventually.
>> >
>> > I guess we do not care about those. If somebody wants to shoot his feet
>> > then we cannot do much about it. The primary motivation was to find out
>> > those that think this is right and they are willing to change the setup
>> > once they know this is not the right way to do things.
>> >
>> > I think that giving a way to suppress the warning is a good step. Log
>> > level might be to coarse and sysctl would be an overkill.
>>
>> When Dave Hansen reported this issue originally, he explained a lot of userland
>> developer misuse /proc/drop_caches because they don't understand what
>> drop_caches do.
>> So, if they never understand the fact, why can we trust them? I have no
>> idea.
>
> Well, most of that usage I have come across was legacy scripts which
> happened to work at a certain point in time because we sucked.
> Thinks have changed but such scripts happen to survive a long time.
> We are primarily interested in those.

Well, if the main target is shell script, task_comm and pid don't help us
a lot. I suggest to add ppid too.

>
>> Or, if you have different motivation w/ Dave, please let me know it.
>
> We have seen reports where users complained about performance drop down
> when in fact the real culprit turned out to be such a clever script
> which dropped caches on the background thinking it will help to free
> some memory. Such cases are tedious to reveal.

Imagine such script have bit-2 and no logging output. Because
the script author think "we are doing the right thing".
Why distro guys want such suppress messages?


>> While the purpose is to shoot misuse, I don't think we can trust
>> userland app. If "If somebody wants to shoot his feet then we cannot
>> do much about it." is true, this patch is useless. OK, we still catch
>> the right user.
>
> I do not think it is useless. It will print a message for all those
> users initially. It is a matter of user how to deal with it.

If it is userland matter, we don't need additional logging at all. userland
can write their own log. Again, if a crazy guys write blog "Hey! we should
use echo 7 > /proc/sys/vm/drop_caches" always, we will come back the
original problem. You and Dave wrote we need to care wrong, rumor and
crazy drop_caches usage. And if so, you need to think new additional
crazy rumor.


>> But we never want to know who is the right users, right?
>
> Well, those that are curious about a new message in the lock and come
> back to us asking what is going on are those we are primarily interested
> in.

I didn't say the message is useless. I did say hidden drop-cache user
is useless.

2013-08-05 07:20:19

by Michal Hocko

[permalink] [raw]
Subject: Re: [PATCH resend] drop_caches: add some documentation and info message

On Sun 04-08-13 21:13:44, KOSAKI Motohiro wrote:
> On Sun, Aug 4, 2013 at 4:07 AM, Michal Hocko <[email protected]> wrote:
> > On Sat 03-08-13 16:16:58, KOSAKI Motohiro wrote:
> >> >>> You missed the "!". I'm proposing that setting the new bit 2 will
> >> >>> permit people to prevent the new printk if it is causing them problems.
> >> >>
> >> >> No I don't. I'm sure almost all abuse users think our usage is correct. Then,
> >> >> I can imagine all crazy applications start to use this flag eventually.
> >> >
> >> > I guess we do not care about those. If somebody wants to shoot his feet
> >> > then we cannot do much about it. The primary motivation was to find out
> >> > those that think this is right and they are willing to change the setup
> >> > once they know this is not the right way to do things.
> >> >
> >> > I think that giving a way to suppress the warning is a good step. Log
> >> > level might be to coarse and sysctl would be an overkill.
> >>
> >> When Dave Hansen reported this issue originally, he explained a lot of userland
> >> developer misuse /proc/drop_caches because they don't understand what
> >> drop_caches do.
> >> So, if they never understand the fact, why can we trust them? I have no
> >> idea.
> >
> > Well, most of that usage I have come across was legacy scripts which
> > happened to work at a certain point in time because we sucked.
> > Thinks have changed but such scripts happen to survive a long time.
> > We are primarily interested in those.
>
> Well, if the main target is shell script, task_comm and pid don't help us
> a lot. I suggest to add ppid too.

I do not have any objections to add ppid.

> >> Or, if you have different motivation w/ Dave, please let me know it.
> >
> > We have seen reports where users complained about performance drop down
> > when in fact the real culprit turned out to be such a clever script
> > which dropped caches on the background thinking it will help to free
> > some memory. Such cases are tedious to reveal.
>
> Imagine such script have bit-2 and no logging output. Because
> the script author think "we are doing the right thing".
> Why distro guys want such suppress messages?

I am not really pushing this suppressing functionality. I just
understand that there might be some legitimate use for supressing and if
that is a must for merging the printk, I can live with that.

[...]
--
Michal Hocko
SUSE Labs

2013-09-17 15:14:24

by Johannes Weiner

[permalink] [raw]
Subject: Re: [PATCH resend] drop_caches: add some documentation and info message

On Mon, Aug 05, 2013 at 09:20:13AM +0200, Michal Hocko wrote:
> On Sun 04-08-13 21:13:44, KOSAKI Motohiro wrote:
> > On Sun, Aug 4, 2013 at 4:07 AM, Michal Hocko <[email protected]> wrote:
> > > On Sat 03-08-13 16:16:58, KOSAKI Motohiro wrote:
> > >> >>> You missed the "!". I'm proposing that setting the new bit 2 will
> > >> >>> permit people to prevent the new printk if it is causing them problems.
> > >> >>
> > >> >> No I don't. I'm sure almost all abuse users think our usage is correct. Then,
> > >> >> I can imagine all crazy applications start to use this flag eventually.
> > >> >
> > >> > I guess we do not care about those. If somebody wants to shoot his feet
> > >> > then we cannot do much about it. The primary motivation was to find out
> > >> > those that think this is right and they are willing to change the setup
> > >> > once they know this is not the right way to do things.
> > >> >
> > >> > I think that giving a way to suppress the warning is a good step. Log
> > >> > level might be to coarse and sysctl would be an overkill.
> > >>
> > >> When Dave Hansen reported this issue originally, he explained a lot of userland
> > >> developer misuse /proc/drop_caches because they don't understand what
> > >> drop_caches do.
> > >> So, if they never understand the fact, why can we trust them? I have no
> > >> idea.
> > >
> > > Well, most of that usage I have come across was legacy scripts which
> > > happened to work at a certain point in time because we sucked.
> > > Thinks have changed but such scripts happen to survive a long time.
> > > We are primarily interested in those.
> >
> > Well, if the main target is shell script, task_comm and pid don't help us
> > a lot. I suggest to add ppid too.
>
> I do not have any objections to add ppid.
>
> > >> Or, if you have different motivation w/ Dave, please let me know it.
> > >
> > > We have seen reports where users complained about performance drop down
> > > when in fact the real culprit turned out to be such a clever script
> > > which dropped caches on the background thinking it will help to free
> > > some memory. Such cases are tedious to reveal.
> >
> > Imagine such script have bit-2 and no logging output. Because
> > the script author think "we are doing the right thing".
> > Why distro guys want such suppress messages?
>
> I am not really pushing this suppressing functionality. I just
> understand that there might be some legitimate use for supressing and if
> that is a must for merging the printk, I can live with that.

Is there any conceivable use case that legitimately drops caches at
such a rate that the logging output becomes a problem?

We export an interface that provides something useful in exceptional
cases but has significant impact on the kernel's ongoing operations.
Kernel developers want this information in problem reports. It's the
same thing as forcefully tainting kernels when a proprietary module is
loaded.