From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mikulas Patocka Subject: Re: [PATCH] kvmalloc: always use vmalloc if CONFIG_DEBUG_VM Date: Fri, 20 Apr 2018 08:16:52 -0400 (EDT) Message-ID: References: <3e65977e-53cd-bf09-bc4b-0ce40e9091fe@gmail.com> <20180418.134651.2225112489265654270.davem@davemloft.net> <20180419124751.8884e516e99825d83da3d87a@linux-foundation.org> <20180419162250.00bf82e2c40b4960a7e23cdc@linux-foundation.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20180419162250.00bf82e2c40b4960a7e23cdc@linux-foundation.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org To: Andrew Morton Cc: dm-devel@redhat.com, eric.dumazet@gmail.com, mst@redhat.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, linux-mm@kvack.org, edumazet@google.com, bhutchings@solarflare.com, David Miller , Vlastimil Babka List-Id: virtualization@lists.linuxfoundation.org On Thu, 19 Apr 2018, Andrew Morton wrote: > On Thu, 19 Apr 2018 17:19:20 -0400 (EDT) Mikulas Patocka wrote: > > > > > In order to detect these bugs reliably I submit this patch that changes > > > > kvmalloc to always use vmalloc if CONFIG_DEBUG_VM is turned on. > > > > > > > > ... > > > > > > > > --- linux-2.6.orig/mm/util.c 2018-04-18 15:46:23.000000000 +0200 > > > > +++ linux-2.6/mm/util.c 2018-04-18 16:00:43.000000000 +0200 > > > > @@ -395,6 +395,7 @@ EXPORT_SYMBOL(vm_mmap); > > > > */ > > > > void *kvmalloc_node(size_t size, gfp_t flags, int node) > > > > { > > > > +#ifndef CONFIG_DEBUG_VM > > > > gfp_t kmalloc_flags = flags; > > > > void *ret; > > > > > > > > @@ -426,6 +427,7 @@ void *kvmalloc_node(size_t size, gfp_t f > > > > */ > > > > if (ret || size <= PAGE_SIZE) > > > > return ret; > > > > +#endif > > > > > > > > return __vmalloc_node_flags_caller(size, node, flags, > > > > __builtin_return_address(0)); > > > > > > Well, it doesn't have to be done at compile-time, does it? We could > > > add a knob (in debugfs, presumably) which enables this at runtime. > > > That's far more user-friendly. > > > > But who will turn it on in debugfs? > > But who will turn it on in Kconfig? Just a handful of developers. We So, it won't receive much testing. I've never played with those debugfs files (because I didn't need it), and most users also won't play with it. Having a debugfs option is like having no option at all. > could add SONFIG_DEBUG_SG to the list in > Documentation/process/submit-checklist.rst, but nobody reads that. > > Also, a whole bunch of defconfigs set CONFIG_DEBUG_SG=y and some > googling indicates that they aren't the only ones... > > > It should be default for debugging > > kernels, so that users using them would report the error. > > Well. This isn't the first time we've wanted to enable expensive (or > noisy) debugging things in debug kernels, by any means. > > So how could we define a debug kernel in which it's OK to enable such > things? Debug kernel is what distributions distribute as debug kernel - i.e. RHEL or Fedora have debugging kernels. So it needs to be bound to an option that is turned on in these kernels - so that any user who boots the debugging kernel triggers the bug. > - Could be "it's an -rc kernel". But then we'd be enabling a bunch of > untested code when Linus cuts a release. > > - Could be "it's an -rc kernel with SUBLEVEL <= 5". But then we risk > unexpected things happening when Linux cuts -rc6, which still isn't > good. > > - How about "it's an -rc kernel with odd-numbered SUBLEVEL and > SUBLEVEL <= 5". That way everybody who runs -rc1, -rc3 and -rc5 will > have kvmalloc debugging enabled. That's potentially nasty because > vmalloc is much slower than kmalloc. But kvmalloc() is only used for > large and probably infrequent allocations, so it's probably OK. > > I wonder how we get at SUBLEVEL from within .c. Don't bind it to rc level, bind it to some debugging configuration option. Mikulas