Linux Virtualization - Apr 2018 - [PATCH] kvmalloc: always use vmalloc if CONFIG_DEBUG_VM

On Sun, 22 Apr 2018, Michal Hocko wrote:
> On Sat 21-04-18 07:47:57, Matthew Wilcox wrote:
> 
> > > He didn't want to fix vmalloc(GFP_NOIO)
> > 
> > I don't remember that conversation, so I don't know whether I
agree with
> > his reasoning or not.  But we are supposed to be moving away from
GFP_NOIO
> > towards marking regions with memalloc_noio_save() / restore.  If you
do
> > that, you won't need vmalloc(GFP_NOIO).
> 
> It was basically to detect GFP_NOIO context _inside_ vmalloc and use the
> scope API to enforce it there. Does it solve potential problems? Yes it
> does. Does it solve any existing report, no I am not aware of any. Is
> it a good fix longterm? Absolutely no, because the scope API should be
> used _at the place_ where the scope starts rather than a random utility
> function. If we are going the easier way now, we will never teach users
> to use the API properly. And I am willing to risk to keep a broken
> code which we have for years rather than allow a random hack that will
> seemingly fix it.
> 
> Btw. I was pretty much explicit with this reasoning when rejecting the
> patch. Do you still call that evil?
You are making nonsensical excuses.

That patch doesn't prevent you from refactoring the kernel and eliminating 
GFP_NOIO in the long term. You can apply the patch and then continue 
working on GFP_NOIO refactoring as well as before.
> > > he didn't want to fix alloc_pages sleeping when __GFP_NORETRY
is used.
> > 
> > The GFP flags are a mess, still.
> 
> I do not remember that one but __GFP_NORETRY is _allowed_ to sleep. And
> yes I do _agree_ gfp flags are a mess which is really hard to get fixed
> because they are lacking a good design from the very beginning. Fixing
> some of those issues today is a completely PITA.
It may sleep, but if it sleeps regularly, it slows down swapping (because 
the swapping code does mempool_alloc and mempool_alloc does __GFP_NORETRY 
allocation). And there were two INTENTIONAL sleeps with schedule_timeout. 
You removed one and left the other, claiming that __GFP_NORETRY allocation 
should sleep.
> > > I already said that we can change it from CONFIG_DEBUG_VM to 
> > > CONFIG_DEBUG_SG - or to whatever other option you may want, just
to make
> > > sure that it is enabled in distro debug kernels by default.
> > 
> > Yes, and I think that's the right idea.  So send a v2 and ignore
the
> > replies that are clearly relating to an earlier version of the patch.
> > Not everybody reads every mail in the thread before responding to one
they
> > find interesting.  Yes, ideally, one would, but sometimes one
doesn't.
> 
> And look here. This is yet another ad-hoc idea. We have many users of
> kvmalloc which have no relation to SG, yet you are going to control
> their behavior by CONFIG_DEBUG_SG? No way! (yeah evil again)
Why aren't you constructive and pick up pick up the CONFIG flag?
> Really, we have a fault injection framework and this sounds like
> something to hook in there.
The testing people won't set it up. They install the
"kernel-debug"
package and run the tests in it.

If you introduce a hidden option that no one knows about, no one will use 
it.
> -- 
> Michal Hocko
> SUSE Labs
Mikulas

On Mon 23-04-18 10:24:02, Mikulas Patocka wrote:
[...]

I am not going to comment on your continuous accusations. We can discuss
patches but general rants make very limited sense.
 
> > > > I already said that we can change it from CONFIG_DEBUG_VM to
> > > > CONFIG_DEBUG_SG - or to whatever other option you may want,
just to make
> > > > sure that it is enabled in distro debug kernels by default.
> > > 
> > > Yes, and I think that's the right idea.  So send a v2 and
ignore the
> > > replies that are clearly relating to an earlier version of the
patch.
> > > Not everybody reads every mail in the thread before responding to
one they
> > > find interesting.  Yes, ideally, one would, but sometimes one
doesn't.
> > 
> > And look here. This is yet another ad-hoc idea. We have many users of
> > kvmalloc which have no relation to SG, yet you are going to control
> > their behavior by CONFIG_DEBUG_SG? No way! (yeah evil again)
> 
> Why aren't you constructive and pick up pick up the CONFIG flag?
Because config doesn't make that much of a sense. You do not want a
permanent vmalloc fallback unconditionally. Debugging option which
changes the behavior completely is not useful IMNHO. Besides that who is
going to enable it?
> > Really, we have a fault injection framework and this sounds like
> > something to hook in there.
> 
> The testing people won't set it up. They install the
"kernel-debug"
> package and run the tests in it.
> 
> If you introduce a hidden option that no one knows about, no one will use 
> it.
then make sure people know about it. Fuzzers already do test fault
injections.
-- 
Michal Hocko
SUSE Labs

On Mon, 23 Apr 2018, Michal Hocko wrote:
> On Mon 23-04-18 10:24:02, Mikulas Patocka wrote:
> 
> > > Really, we have a fault injection framework and this sounds like
> > > something to hook in there.
> > 
> > The testing people won't set it up. They install the
"kernel-debug"
> > package and run the tests in it.
> > 
> > If you introduce a hidden option that no one knows about, no one will
use
> > it.
> 
> then make sure people know about it. Fuzzers already do test fault
> injections.
I think that in the long term we can introduce a kernel parameter like 
"debug_level=1", "debug_level=2", "debug_level=3"
that will turn on
debugging features across all kernel subsystems and we can teach users to 
use it to diagnose problems.

But it won't work if every subsystem has different debug parameters. There 
are 192 distinct filenames in debugfs, if we add 193rd one, harly anyone 
notices it.

Mikulas