linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: Jeremy Fitzhardinge <jeremy@goop.org>
To: Christoph Lameter <clameter@sgi.com>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	akpm@linux-foundation.org
Subject: Re: [RFC 0/4] CONFIG_STABLE to switch off development checks
Date: Fri, 01 Jun 2007 11:58:57 -0700	[thread overview]
Message-ID: <46606C71.9010008@goop.org> (raw)
In-Reply-To: <Pine.LNX.4.64.0706011126030.2284@schroedinger.engr.sgi.com>

Christoph Lameter wrote:
> Hmmm... We got there because SLUB initially return NULL for kmalloc(0). 
> Rationale: The user did not request any memory so we wont give him 
> any.
>
> That (to my surprise) caused some strange behavior of code and so we then 
> decided to keep SLAB behavior and return the smallest available object 
> size and put a warning in there. At some later point we plan to switch
> to returning NULL for kmalloc(0).
>   

Unfortunately, returning NULL is indistinguishable from ENOMEM, so the
caller would have to check to see how much it asked for before deciding
to really fail, which doesn't help things much.

Or does it (should it) return ERRPTR(-ENOMEM)?  Bit of a major API
change if not.

>> Why not just do a 1 byte allocation instead, and be done with it?  Any
>> non-constant-sized allocation will potentially have to deal with this
>> case, so it seems to me we could just put the fix in common code (and
>> use an inline wrapper to avoid it when dealing with constant non-zero
>> sized allocations).
>>     
>
> The smallest allocation that SLUB can do is 8 bytes (SLAB 32). We are
> giving the user the smallest object that we can allocate. We could just 
> remove the warning and would have the behavior that you want.
>   

I think that's reasonable.  0-sized allocations seem to be relatively
rare anyway, so its not like we're going to be filling the heap with
these things.

I'm getting a couple of these warnings out of my code, but I've been
hesitating about fixing them because I can't see it resulting in any
improvement, either locally or globally - it would just be to shut the
warning up.

> But now allocating code gets memory although none was requested. The 
> object can be modified without us being able to check.
>   

Yes, that's a problem, but maybe heap debugging would help (ie, if
enabled make sure the "zero-sized allocation" pattern is undisturbed). 
Or return a pointer to a specific non-NULL unmapped address, though that
would need special handling on the free side.  Also, callers may get
upset if they get non-unique non-NULL addresses back from allocation.

> By returning NULL any use of the returned pointer would BUG.
>   

Well, actually it would stomp usermode memory, but we generally assume
there's nothing mapped at 0.  Or there are no bugs.

> An allocation of zero bytes usually indicates that the code is not dealing 
> with a special case. Later code may operate on the allocated object. I 
> think its clearer and cleaner if code would deal with that special case 
> explicitly. We have seen a series of code pieces that do uncomfortably 
> looking operations on structures with no objects.
>   

I disagree.  There are plenty of boundary conditions where 0 is not
really a special case, and making it a special case just complicates
things.  I think at least some of the patches posted to silence this
warning have been generally negative for code quality.  If we were
seeing lots of zero-sized allocations then that might indicate something
is amiss, but it seems to me that there's just a scattered handful.

I agree that it's always a useful debugging aid to make sure that
allocated regions are not over-run, but 0-sized allocations are not
special in this regard.

    J

--
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>

  reply	other threads:[~2007-06-01 18:58 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-05-31  0:20 clameter
2007-05-31  0:20 ` [RFC 1/4] CONFIG_STABLE: Define it clameter
2007-05-31  0:35   ` young dave
2007-05-31  0:49     ` Christoph Lameter
2007-06-01 18:08       ` Dave Jones
2007-06-01 18:25         ` Christoph Lameter
2007-05-31  8:54   ` Stefan Richter
2007-05-31  9:03     ` David Miller, Stefan Richter
2007-05-31  9:03     ` Stefan Richter
2007-05-31 21:11   ` Andrew Morton
2007-05-31 21:14     ` Christoph Lameter
2007-05-31 21:30     ` Sam Ravnborg
2007-06-01 18:02       ` Dave Jones
2007-06-01 20:22         ` Sam Ravnborg
2007-06-01 20:30           ` Dave Jones
2007-06-01 20:55             ` Sam Ravnborg
2007-06-01 20:25     ` Sam Ravnborg
2007-07-20 10:41   ` Satyam Sharma
2007-07-20 11:09     ` Chris Snook
2007-07-20 11:27       ` Satyam Sharma
2007-07-20 11:34         ` Chris Snook
2007-07-20 11:40           ` Satyam Sharma
2007-07-20 11:50             ` Chris Snook
2007-07-20 16:48               ` Stefan Richter
2007-07-20 16:28     ` Stefan Richter
2007-07-20 16:36       ` Stefan Richter
2007-07-20 19:09         ` Chuck Ebbert
2007-05-31  0:20 ` [RFC 2/4] CONFIG_STABLE: Switch off kmalloc(0) tests in slab allocators clameter
2007-05-31 19:51   ` Zach Brown
2007-05-31 22:37     ` Andi Kleen
2007-05-31  0:20 ` [RFC 3/4] CONFIG_STABLE: Switch off SLUB banner clameter
2007-05-31  0:20 ` [RFC 4/4] CONFIG_STABLE: SLUB: Prefer object corruption over failure clameter
2007-06-01 14:55 ` [RFC 0/4] CONFIG_STABLE to switch off development checks Jeremy Fitzhardinge
2007-06-01 18:38   ` Christoph Lameter
2007-06-01 18:58     ` Jeremy Fitzhardinge [this message]
2007-06-01 20:59       ` Christoph Lameter
2007-06-01 21:24         ` Jeremy Fitzhardinge
2007-06-02 15:23       ` Dave Kleikamp
2007-06-02 16:28         ` Jeremy Fitzhardinge
2007-06-04  1:03           ` Dave Kleikamp
     [not found] <fa.UBCbBXgIW93M6j2F+d+umQ5+v9I@ifi.uio.no>
     [not found] ` <fa.iaekQW/Par/E6eIpnL0NjEdCUxc@ifi.uio.no>
     [not found]   ` <fa.2BlkzuhauAATrsG1MYhPMeWMhPM@ifi.uio.no>
     [not found]     ` <fa.o9WA1K75HxwNnBEQDyoQMfWVpiQ@ifi.uio.no>
     [not found]       ` <fa.wiSgrIhkRNkkC7Wh6Bt3BY4z7BM@ifi.uio.no>
2007-06-05  0:38         ` Robert Hancock

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=46606C71.9010008@goop.org \
    --to=jeremy@goop.org \
    --cc=akpm@linux-foundation.org \
    --cc=clameter@sgi.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox