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

On Tue 24-04-18 11:50:30, Mikulas Patocka wrote:
> 
> 
> On Tue, 24 Apr 2018, Michal Hocko wrote:
> 
> > On Mon 23-04-18 20:06:16, Mikulas Patocka wrote:
> > [...]
> > > @@ -404,6 +405,12 @@ void *kvmalloc_node(size_t size, gfp_t f
> > >  	 */
> > >  	WARN_ON_ONCE((flags & GFP_KERNEL) != GFP_KERNEL);
> > >  
> > > +#ifdef CONFIG_DEBUG_SG
> > > +	/* Catch bugs when the caller uses DMA API on the result of
kvmalloc. */
> > > +	if (!(prandom_u32_max(2) & 1))
> > > +		goto do_vmalloc;
> > > +#endif
> > 
> > I really do not think there is anything DEBUG_SG specific here. Why
you
> > simply do not follow should_failslab path or even reuse the function?
> 
> CONFIG_DEBUG_SG is enabled by default in RHEL and Fedora debug kernel (if 
> you don't like CONFIG_DEBUG_SG, pick any other option that is enabled 
> there).
Are you telling me that you are shaping a debugging functionality basing
on what RHEL has enabled? And you call me evil. This is just rediculous.
> Fail-injection framework is if off by default and it must be explicitly 
> enabled and configured by the user - and most users won't enable it.
It can be enabled easily. And if you care enough for your debugging
kernel then just make it enabled unconditionally.
-- 
Michal Hocko
SUSE Labs

On Tue, 24 Apr 2018, Michal Hocko wrote:
> On Tue 24-04-18 11:50:30, Mikulas Patocka wrote:
> > 
> > 
> > On Tue, 24 Apr 2018, Michal Hocko wrote:
> > 
> > > On Mon 23-04-18 20:06:16, Mikulas Patocka wrote:
> > > [...]
> > > > @@ -404,6 +405,12 @@ void *kvmalloc_node(size_t size, gfp_t
f
> > > >  	 */
> > > >  	WARN_ON_ONCE((flags & GFP_KERNEL) != GFP_KERNEL);
> > > >  
> > > > +#ifdef CONFIG_DEBUG_SG
> > > > +	/* Catch bugs when the caller uses DMA API on the result
of kvmalloc. */
> > > > +	if (!(prandom_u32_max(2) & 1))
> > > > +		goto do_vmalloc;
> > > > +#endif
> > > 
> > > I really do not think there is anything DEBUG_SG specific here.
Why you
> > > simply do not follow should_failslab path or even reuse the
function?
> > 
> > CONFIG_DEBUG_SG is enabled by default in RHEL and Fedora debug kernel
(if
> > you don't like CONFIG_DEBUG_SG, pick any other option that is
enabled
> > there).
> 
> Are you telling me that you are shaping a debugging functionality basing
> on what RHEL has enabled? And you call me evil. This is just rediculous.
> 
> > Fail-injection framework is if off by default and it must be
explicitly
> > enabled and configured by the user - and most users won't enable
it.
> 
> It can be enabled easily. And if you care enough for your debugging
> kernel then just make it enabled unconditionally.
So, should we add a new option CONFIG_KVMALLOC_FALLBACK_DEFAULT? I'm not 
quite sure if 3 lines of debugging code need an extra option, but if you 
don't want to reuse any existing debug option, it may be possible. Adding 
it to the RHEL debug kernel would be trivial.

Mikulas

On Tue 24-04-18 13:00:11, Mikulas Patocka wrote:
> 
> 
> On Tue, 24 Apr 2018, Michal Hocko wrote:
> 
> > On Tue 24-04-18 11:50:30, Mikulas Patocka wrote:
> > > 
> > > 
> > > On Tue, 24 Apr 2018, Michal Hocko wrote:
> > > 
> > > > On Mon 23-04-18 20:06:16, Mikulas Patocka wrote:
> > > > [...]
> > > > > @@ -404,6 +405,12 @@ void *kvmalloc_node(size_t size,
gfp_t f
> > > > >  	 */
> > > > >  	WARN_ON_ONCE((flags & GFP_KERNEL) != GFP_KERNEL);
> > > > >  
> > > > > +#ifdef CONFIG_DEBUG_SG
> > > > > +	/* Catch bugs when the caller uses DMA API on the
result of kvmalloc. */
> > > > > +	if (!(prandom_u32_max(2) & 1))
> > > > > +		goto do_vmalloc;
> > > > > +#endif
> > > > 
> > > > I really do not think there is anything DEBUG_SG specific
here. Why you
> > > > simply do not follow should_failslab path or even reuse the
function?
> > > 
> > > CONFIG_DEBUG_SG is enabled by default in RHEL and Fedora debug
kernel (if
> > > you don't like CONFIG_DEBUG_SG, pick any other option that is
enabled
> > > there).
> > 
> > Are you telling me that you are shaping a debugging functionality
basing
> > on what RHEL has enabled? And you call me evil. This is just
rediculous.
> > 
> > > Fail-injection framework is if off by default and it must be
explicitly
> > > enabled and configured by the user - and most users won't
enable it.
> > 
> > It can be enabled easily. And if you care enough for your debugging
> > kernel then just make it enabled unconditionally.
> 
> So, should we add a new option CONFIG_KVMALLOC_FALLBACK_DEFAULT? I'm
not
> quite sure if 3 lines of debugging code need an extra option, but if you 
> don't want to reuse any existing debug option, it may be possible.
Adding
> it to the RHEL debug kernel would be trivial.
Wouldn't it be equally trivial to simply enable the fault injection? You
would get additional failure paths testing as a bonus.
-- 
Michal Hocko
SUSE Labs