From: Catalin Marinas <catalin.marinas@arm.com>
To: "Bird, Tim" <Tim.Bird@sonymobile.com>
Cc: "Christoph Lameter" <cl@linux.com>,
"Frank Rowand" <frowand.list@gmail.com>,
"Pekka Enberg" <penberg@kernel.org>,
"Matt Mackall" <mpm@selenic.com>,
"linux-mm@kvack.org" <linux-mm@kvack.org>,
"Linux Kernel list" <linux-kernel@vger.kernel.org>,
"Bobniev, Roman" <Roman.Bobniev@sonymobile.com>,
"Andersson, Björn" <Bjorn.Andersson@sonymobile.com>
Subject: Re: [PATCH] slub: Proper kmemleak tracking if CONFIG_SLUB_DEBUG disabled
Date: Wed, 2 Oct 2013 16:54:18 +0100 [thread overview]
Message-ID: <20131002155417.GB29794@arm.com> (raw)
In-Reply-To: <F5184659D418E34EA12B1903EE5EF5FD8538E86615@seldmbx02.corpusers.net>
On Wed, Oct 02, 2013 at 04:33:47PM +0100, Bird, Tim wrote:
> On Wednesday, October 02, 2013 7:41 AM, Christoph Lameter [cl@linux.com] wrote:
> >
> >On Fri, 27 Sep 2013, Frank Rowand wrote:
> >
> >> Move the kmemleak code for small block allocation out from
> >> under CONFIG_SLUB_DEBUG.
> >
> >Well in that case it may be better to move the hooks as a whole out of
> >the CONFIG_SLUB_DEBUG section. Do the #ifdeffering for each call from the
> >hooks instead.
> >
> >The point of the hook functions is to separate the hooks out of the
> >functions so taht they do not accumulate in the main code.
> >
> >The patch moves one hook back into the main code. Please keep the checks
> >in the hooks.
>
> Thanks for the feedback. Roman's first patch, which we discussed internally
> before sending this one, did exactly that. I guess Roman gets to say "I told
> you so." :-) My bad for telling him to change it.
>
> We'll refactor along the lines that you describe, and send another one.
>
> The problem child is actually the unconditional call to kmemleak_alloc()
> in kmalloc_large_node() (in slub.c). The problem comes because that call
> is unconditional on CONFIG_SLUB_DEBUG but the kmemleak
> calls in the hook routines are conditional on CONFIG_SLUB_DEBUG.
> So if you have CONFIG_SLUB_DEBUG=n but CONFIG_DEBUG_KMEMLEAK=y,
> you get the false reports.
>
> Now, there are kmemleak calls in kmalloc_large_node() and kfree() that don't
> follow the "hook" pattern. Should these be moved to 'hook' routines, to keep
> all the checks in the hooks?
>
> Personally, I like the idea of keeping bookeeping/tracing/debug stuff in hook
> routines. I also like de-coupling CONFIG_SLUB_DEBUG and CONFIG_DEBUG_KMEMLEAK,
> but maybe others have a different opinon. Unless someone speaks up, we'll
> move the the currently in-function kmemleak calls into hooks, and all of the
> kmemleak stuff out from under CONFIG_SLUB_DEBUG.
> We'll have to see if the ifdefs get a little messy.
Kmemleak doesn't depend on SLUB_DEBUG (at least it didn't originally ;),
so I don't think we should add an artificial dependency (or select). Can
we have kmemleak_*() calls in both debug and !debug hooks?
--
Catalin
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
WARNING: multiple messages have this Message-ID (diff)
From: Catalin Marinas <catalin.marinas@arm.com>
To: "Bird, Tim" <Tim.Bird@sonymobile.com>
Cc: "Christoph Lameter" <cl@linux.com>,
"Frank Rowand" <frowand.list@gmail.com>,
"Pekka Enberg" <penberg@kernel.org>,
"Matt Mackall" <mpm@selenic.com>,
"linux-mm@kvack.org" <linux-mm@kvack.org>,
"Linux Kernel list" <linux-kernel@vger.kernel.org>,
"Bobniev, Roman" <Roman.Bobniev@sonymobile.com>,
"Andersson, Björn" <Bjorn.Andersson@sonymobile.com>
Subject: Re: [PATCH] slub: Proper kmemleak tracking if CONFIG_SLUB_DEBUG disabled
Date: Wed, 2 Oct 2013 16:54:18 +0100 [thread overview]
Message-ID: <20131002155417.GB29794@arm.com> (raw)
In-Reply-To: <F5184659D418E34EA12B1903EE5EF5FD8538E86615@seldmbx02.corpusers.net>
On Wed, Oct 02, 2013 at 04:33:47PM +0100, Bird, Tim wrote:
> On Wednesday, October 02, 2013 7:41 AM, Christoph Lameter [cl@linux.com] wrote:
> >
> >On Fri, 27 Sep 2013, Frank Rowand wrote:
> >
> >> Move the kmemleak code for small block allocation out from
> >> under CONFIG_SLUB_DEBUG.
> >
> >Well in that case it may be better to move the hooks as a whole out of
> >the CONFIG_SLUB_DEBUG section. Do the #ifdeffering for each call from the
> >hooks instead.
> >
> >The point of the hook functions is to separate the hooks out of the
> >functions so taht they do not accumulate in the main code.
> >
> >The patch moves one hook back into the main code. Please keep the checks
> >in the hooks.
>
> Thanks for the feedback. Roman's first patch, which we discussed internally
> before sending this one, did exactly that. I guess Roman gets to say "I told
> you so." :-) My bad for telling him to change it.
>
> We'll refactor along the lines that you describe, and send another one.
>
> The problem child is actually the unconditional call to kmemleak_alloc()
> in kmalloc_large_node() (in slub.c). The problem comes because that call
> is unconditional on CONFIG_SLUB_DEBUG but the kmemleak
> calls in the hook routines are conditional on CONFIG_SLUB_DEBUG.
> So if you have CONFIG_SLUB_DEBUG=n but CONFIG_DEBUG_KMEMLEAK=y,
> you get the false reports.
>
> Now, there are kmemleak calls in kmalloc_large_node() and kfree() that don't
> follow the "hook" pattern. Should these be moved to 'hook' routines, to keep
> all the checks in the hooks?
>
> Personally, I like the idea of keeping bookeeping/tracing/debug stuff in hook
> routines. I also like de-coupling CONFIG_SLUB_DEBUG and CONFIG_DEBUG_KMEMLEAK,
> but maybe others have a different opinon. Unless someone speaks up, we'll
> move the the currently in-function kmemleak calls into hooks, and all of the
> kmemleak stuff out from under CONFIG_SLUB_DEBUG.
> We'll have to see if the ifdefs get a little messy.
Kmemleak doesn't depend on SLUB_DEBUG (at least it didn't originally ;),
so I don't think we should add an artificial dependency (or select). Can
we have kmemleak_*() calls in both debug and !debug hooks?
--
Catalin
next prev parent reply other threads:[~2013-10-02 15:54 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-09-27 20:38 [PATCH] slub: Proper kmemleak tracking if CONFIG_SLUB_DEBUG disabled Frank Rowand
2013-09-27 20:38 ` Frank Rowand
2013-09-30 9:04 ` Catalin Marinas
2013-09-30 9:04 ` Catalin Marinas
2013-10-02 14:41 ` Christoph Lameter
2013-10-02 14:41 ` Christoph Lameter
2013-10-02 15:33 ` Bird, Tim
2013-10-02 15:33 ` Bird, Tim
2013-10-02 15:54 ` Catalin Marinas [this message]
2013-10-02 15:54 ` Catalin Marinas
2013-10-02 16:24 ` Christoph Lameter
2013-10-02 16:24 ` Christoph Lameter
2013-10-02 15:57 ` Christoph Lameter
2013-10-02 15:57 ` Christoph Lameter
2013-10-02 16:25 ` Catalin Marinas
2013-10-02 16:25 ` Catalin Marinas
-- strict thread matches above, loose matches on Subject: below --
2013-10-08 22:37 [PATCH] slub: proper " Tim Bird
2013-10-08 22:37 ` Tim Bird
2013-10-08 22:58 Tim Bird
2013-10-08 22:58 ` Tim Bird
2013-10-09 13:28 ` Catalin Marinas
2013-10-09 13:28 ` Catalin Marinas
2013-10-09 19:07 ` Christoph Lameter
2013-10-09 19:07 ` Christoph Lameter
2013-10-23 11:52 ` Bobniev, Roman
2013-10-23 11:52 ` Bobniev, Roman
2013-10-24 17:20 ` Pekka Enberg
2013-10-24 17:20 ` Pekka Enberg
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20131002155417.GB29794@arm.com \
--to=catalin.marinas@arm.com \
--cc=Bjorn.Andersson@sonymobile.com \
--cc=Roman.Bobniev@sonymobile.com \
--cc=Tim.Bird@sonymobile.com \
--cc=cl@linux.com \
--cc=frowand.list@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mpm@selenic.com \
--cc=penberg@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.