archive mirror
 help / color / mirror / Atom feed
From: "Paul E. McKenney" <>
To: Joonsoo Kim <>
	Christoph Lameter <>,
	Pekka Enberg <>,
	David Rientjes <>,
Subject: Re: [PATCH sl-b 1/6] mm: Add kmem_last_alloc() to return last allocation for memory block
Date: Mon, 7 Dec 2020 09:25:54 -0800	[thread overview]
Message-ID: <20201207172554.GI2657@paulmck-ThinkPad-P72> (raw)
In-Reply-To: <20201207090243.GA20765@js1304-desktop>

On Mon, Dec 07, 2020 at 06:02:53PM +0900, Joonsoo Kim wrote:
> Hello, Paul.
> On Fri, Dec 04, 2020 at 04:40:52PM -0800, wrote:
> > From: "Paul E. McKenney" <>
> > 
> > There are kernel facilities such as per-CPU reference counts that give
> > error messages in generic handlers or callbacks, whose messages are
> > unenlightening.  In the case of per-CPU reference-count underflow, this
> > is not a problem when creating a new use of this facility because in that
> > case the bug is almost certainly in the code implementing that new use.
> > However, trouble arises when deploying across many systems, which might
> > exercise corner cases that were not seen during development and testing.
> > Here, it would be really nice to get some kind of hint as to which of
> > several uses the underflow was caused by.
> > 
> > This commit therefore exposes a new kmem_last_alloc() function that
> > takes a pointer to dynamically allocated memory and returns the return
> > address of the call that allocated it.  This pointer can reference the
> > middle of the block as well as the beginning of the block, as needed
> > by things like RCU callback functions and timer handlers that might not
> > know where the beginning of the memory block is.  These functions and
> > handlers can use the return value from kmem_last_alloc() to give the
> > kernel hacker a better hint as to where the problem might lie.
> I agree with exposing allocation caller information to the other
> subsystem to help the debugging. Some suggestions...

Good to hear!  ;-)

> 1. It's better to separate a slab object check (validity check) and
> retrieving the allocation caller. Someone else would want to check
> only a validity. And, it doesn't depend on the debug configuration so
> it's not good to bind it to the debug function.
> kmem_cache_valid_(obj|ptr)
> kmalloc_valid_(obj|ptr)

Here both functions would say "true" for a pointer from kmalloc()?
Or do I need to add a third function that is happy with a pointer from
either source?

I do understand that people who don't want to distinguish could just do
"kmem_cache_valid_ptr(p) || kmalloc_valid_ptr(p)".  However, the two
use cases in the series have no idea whether the pointer they have came
from kmalloc(), kmem_cache_alloc(), or somewhere else entirely, even an
on-stack variable.

Are you asking me to choose between the _obj() and _ptr() suffixes?
If not, please help me understand the distinction.

Do we want "debug" in these names as well?

> 2. rename kmem_last_alloc to ...
> int kmem_cache_debug_alloc_caller(cache, obj, &ret_addr)
> int kmalloc_debug_alloc_caller(obj, &ret_addr)
> or debug_kmem_cache_alloc_caller()
> I think that function name need to include the keyword 'debug' to show
> itself as a debugging facility (enabled at the debugging). And, return
> errno and get caller address by pointer argument.

I am quite happy to add the "debug", but my use cases have no idea
how the pointer was allocated.  In fact, the next version of the
patch will also handle allocator return addresses from vmalloc().

And for kernels without sufficient debug enabled, I need to provide
the name of the slab cache, and this also is to be in the next version.

> 3. If concrete error message is needed, please introduce more functions.
> void *kmalloc_debug_error(errno)

Agreed, in fact, I was planning to have a function that printed out
a suitable error-message continuation to the console for ease-of-use
reasons.  For example, why is the caller deciding how deep the stack
frame is?  ;-)

So something like this?

	void kmalloc_debug_print_provenance(void *ptr);

With the understanding that it will print something helpful regardless
of where ptr came from, within the constraints of the kernel build and
boot options?

							Thanx, Paul

  reply	other threads:[~2020-12-07 17:25 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20201205004022.GA31166@paulmck-ThinkPad-P72>
2020-12-05  0:40 ` [PATCH sl-b 1/6] mm: Add kmem_last_alloc() to return last allocation for memory block paulmck
2020-12-07  9:02   ` Joonsoo Kim
2020-12-07 17:25     ` Paul E. McKenney [this message]
2020-12-08  8:57       ` Joonsoo Kim
2020-12-08 15:17         ` Paul E. McKenney
2020-12-05  0:40 ` [PATCH sl-b 2/6] mm: Add kmem_last_alloc_errstring() to provide more kmem_last_alloc() info paulmck
2020-12-05  0:40 ` [PATCH sl-b 3/6] rcu: Make call_rcu() print allocation address of double-freed callback paulmck
2020-12-05  0:40 ` [PATCH sl-b 4/6] mm: Create kmem_last_alloc_stack() to provide stack trace in slub paulmck

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:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201207172554.GI2657@paulmck-ThinkPad-P72 \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \

* 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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).