linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Why is the kfree() argument const?
@ 2008-01-16 16:32 Johannes Weiner
  2008-01-16 16:48 ` Christoph Lameter
  2008-01-16 18:39 ` Linus Torvalds
  0 siblings, 2 replies; 51+ messages in thread
From: Johannes Weiner @ 2008-01-16 16:32 UTC (permalink / raw)
  To: Linux Kernel Mailing List; +Cc: clameter, penberg

Hi,

is there any reason why kfree() takes a const pointer just to degrade it
with the call to slab_free()/__cache_free() again?  The promise that the
pointee is not modified is just bogus in this case, anyway, isn't it?

	Hannes

^ permalink raw reply	[flat|nested] 51+ messages in thread
[parent not found: <MDEHLPKNGKAHNMBLJOLKIEGIJJAC.davids@webmaster.com>]
* Re: Why is the kfree() argument const?
@ 2008-01-18 12:45 ecolbus
  2008-01-18 15:20 ` Giacomo A. Catenazzi
  0 siblings, 1 reply; 51+ messages in thread
From: ecolbus @ 2008-01-18 12:45 UTC (permalink / raw)
  To: linux-kernel

Giacomo Catenazzi wrote:

> const No writes through this lvalue. In the absence of this qualifier, writes may occur
> through this lvalue.
> 
> volatile No cacheing through this lvalue: each operation in the abstract semantics must
> be performed (that is, no cacheing assumptions may be made, since the location
> is not guaranteed to contain any previous value). In the absence of this qualifier,
> the contents of the designated location may be assumed to be unchanged except
> for possible aliasing.

Well, I'm still wondering if there is not something dangerous or weird about
declaring the argument of kfree() to be const... 

Since the content of the referenced object is unlikely to be declared volatile, the
compiler should be allowed to assume that its content was not changed, except
for possible aliasing. But what happens if the compiler can also prove there is 
no aliasing? In that case, he should be allowed to assume that the content
pointed to was not modified at all, right?

Consider the following code : 

struct something {
        int i;
};

...

struct something *s1, *s2;
extern int val;

s1 = kmalloc(sizeof(*s1), SOME_FLAGS);
if (s1 == NULL)
        return -ENOMEM;

s1->i = do_something();
do_some_other_thing();

s2 = kmalloc(sizeof(*s2), SOME_FLAGS);
if (s2 == NULL){
        val = s1->i; /* XXX */
        kfree(s1);
        return -ENOMEM;
}

Fortunately, kmalloc is not declared with attribute malloc in the kernel,
so there should be no problem, but if it were (and, actually, I've not found
why it wasn't), the compiler would be able to tell that *s1 *cannot* 
be aliased, and therefore decide to move val = s1->i *after* having
called kfree(). In that case, we would clearly have a bug...

So, although this should currently work, code which breaks if you do
a legitimate modification somewere else looks quite dangerous to me.

Or maybe there is a rationale for never declaring kmalloc to have the
attribute malloc in the first place...

Or is there simply something that I still don't understand, Giacomo?

Cheers,

Emmanuel Colbus



^ permalink raw reply	[flat|nested] 51+ messages in thread
* Re: Why is the kfree() argument const?
@ 2008-01-18 16:45 ecolbus
  2008-01-18 18:20 ` Olivier Galibert
  0 siblings, 1 reply; 51+ messages in thread
From: ecolbus @ 2008-01-18 16:45 UTC (permalink / raw)
  To: Giacomo A. Catenazzi; +Cc: linux-kernel

Giacomo A. Catenazzi wrote : 
> ecolbus@voila.fr wrote:
> > Giacomo Catenazzi wrote:
> > 
> >> const No writes through this lvalue. In the absence of this qualifier, writes may occur
> >> through this lvalue.
> >>
> >> volatile No cacheing through this lvalue: each operation in the abstract semantics must
> >> be performed (that is, no cacheing assumptions may be made, since the location
> >> is not guaranteed to contain any previous value). In the absence of this qualifier,
> >> the contents of the designated location may be assumed to be unchanged except
> >> for possible aliasing.
> > 
> > Well, I'm still wondering if there is not something dangerous or weird about
> > declaring the argument of kfree() to be const... 
> 
> It should be only cosmetic thing (and few warnings in some
> not yet identified cases).
> 
> 
> > Since the content of the referenced object is unlikely to be declared volatile, the
> > compiler should be allowed to assume that its content was not changed, except
> > for possible aliasing. But what happens if the compiler can also prove there is 
> > no aliasing? In that case, he should be allowed to assume that the content
> > pointed to was not modified at all, right?
> 
> I doesn't follow it. Anyway C has the "as-if" rule, which it mean:
> the compiler could optimize as far the result doesn't change
> (time are not issues, and result could change if a valid compiler
> could give the same results). 

As long as the code is correct, yes. If the code is wrong in the first
place (like adding or removing a required type qualifier), the compiler
optimization can break your code. I'm arguing that having at the same
time kfree() declared as taking a const pointer and kmalloc() with
attribute malloc would be incorrect.

Let me rephrase my argumentation : if the compiler knows a) that a
variable is never modified through a given lvalue (because this pointer
is declared as const), and b) that it is also never modified through any
other lvalues (because it *knows* that no pointer can point to it, and it
can see all other uses), THEN it knows it is not modified at all.

And it can use this knowledge to do all optimizations it wants.

The malloc attribute is exactly about this : giving the compiler the
indication that no other pointer aliases this object, allowing for
better optimizations.

> 
> > Fortunately, kmalloc is not declared with attribute malloc in the kernel,
> > so there should be no problem, but if it were (and, actually, I've not found
> > why it wasn't), the compiler would be able to tell that *s1 *cannot* 
> > be aliased, and therefore decide to move val = s1->i *after* having
> > called kfree(). In that case, we would clearly have a bug...
> 
> IIRC kmalloc(0) return an alias (but not so relevant in this
> discussion).
> 
> Hmm. C is used not to do much optimization. One thing to remember
> is that function could have a lot of side effects, so compiler
> will/should never optimize in your way (but if compiler know exactly
> how kfree work internally).
> 
> C "const" is a lot weaker to C++ "const".
> 
> BTW, I doesn't like const in kfree, but I was talking about
> weak "const" in C.
> 
> 
> > So, although this should currently work, code which breaks if you do
> > a legitimate modification somewere else looks quite dangerous to me.
> 
> for sure!  But C is anal in: "users know better than compiler on what
> they want to do", so compiler cannot do big optimizations C, without
> breaking C rules, and used can do nasty things playing with hidden
> pointers.

Yes. Bad things start to happen when users add wrong indications to
the compiler. By adding the "const" indication to kfree(), the programmer
wrongly tells that it can optimize reading the values pointed to before or
after calling the function (if it is also sure that they cannot be
read/written otherwise). Current gcc implementations seem quite
conservative in this regard, and don't optimize that much, but what about
the future?

Cheers,

Emmanuel Colbus

> 
> PS: use lkml rule: "do CC: to all relevant people!"

Sorry, actually, I'm not subscribed myself, I was just
following this discussion through hypermail.




^ permalink raw reply	[flat|nested] 51+ messages in thread
* Re: Why is the kfree() argument const?
@ 2008-01-18 19:10 ecolbus
  0 siblings, 0 replies; 51+ messages in thread
From: ecolbus @ 2008-01-18 19:10 UTC (permalink / raw)
  To: linux-kernel; +Cc: Giacomo A. Catenazzi


Olivier Galibert wrote:
> On Fri, Jan 18, 2008 at 05:45:49PM +0100, ecolbus@xxxxxxxx wrote:
> > The malloc attribute is exactly about this : giving the compiler the
> > indication that no other pointer aliases this object, allowing for
> > better optimizations.
> 
> If you put a malloc attribute on the allocator and no free attribute
> on the deallocator, you can get bugs indeed. GIGO.
> 
> 
> > Yes. Bad things start to happen when users add wrong indications to
> > the compiler. By adding the "const" indication to kfree(), the programmer
> > wrongly tells that it can optimize reading the values pointed to before or
> > after calling the function (if it is also sure that they cannot be
> > read/written otherwise). Current gcc implementations seem quite
> > conservative in this regard, and don't optimize that much, but what about
> > the future?
> 
> The future should be quite nice because:
> 
> - the compiler can not know that kmalloc does not have an alias to
> the pointer tucked somewhere accessible by other non-inline functions
> (as kfree is), especially since it does have aliases in practice, so
> it cannot prove to "not read/written otherwise" part without the
> malloc attribute
> 
> - if you add the (non-standard C) malloc attribute to kmalloc, then
> you also add the free attribute to kfree which tells the compiler
> that the pointer is invalid after the call, which ensures no
> accesses will be moved after it
> 
> OG.

I completely agree with you here. But, unless this page : 

http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html

is not up-to-date anymore, there is no free attribute. Which
is quite logical, actually, as such an attribute would be
equivalent to simply marking the parameter of the function
non-const.

That's what I was alluding to, when speaking of a legitimate
change (adding attribute malloc to kmalloc()) which would break
another part of the kernel. Of course, we can also assume that
nobody would make the error of letting kfree() taking a 
const void* after that, but experience shows that programmers
are very likely to fall into such traps.

(No flame intended on anybody : in my case, I'm actually able
to fall even when there is no trap at all. That's the reason for
spotting such a weird one makes me anxious. Especially since
having kfree() take a non-const argument comes at no runtime
cost.)

Cheers,

Emmanuel



^ permalink raw reply	[flat|nested] 51+ messages in thread
[parent not found: <fa.cHMztHfqJXv7vw5O0nQ8SdTrma0@ifi.uio.no>]

end of thread, other threads:[~2013-01-14  1:45 UTC | newest]

Thread overview: 51+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-01-16 16:32 Why is the kfree() argument const? Johannes Weiner
2008-01-16 16:48 ` Christoph Lameter
2008-01-16 17:34   ` Bernd Petrovitsch
2008-01-16 17:45   ` Pekka J Enberg
2008-01-16 18:39 ` Linus Torvalds
2008-01-16 22:19   ` Johannes Weiner
2008-01-16 22:20     ` Christoph Lameter
2008-01-16 22:37       ` Johannes Weiner
2008-01-16 23:13       ` Johannes Weiner
2008-01-16 23:18       ` Linus Torvalds
2008-01-16 23:16     ` Linus Torvalds
2008-01-16 22:33   ` Steven Rostedt
     [not found] <MDEHLPKNGKAHNMBLJOLKIEGIJJAC.davids@webmaster.com>
2008-01-17 21:25 ` Linus Torvalds
2008-01-17 22:28   ` David Schwartz
2008-01-17 23:10     ` Linus Torvalds
2008-01-18  0:56       ` David Schwartz
2008-01-18  1:15         ` Linus Torvalds
2008-01-18  5:02           ` David Schwartz
2008-01-18 15:38             ` Chris Friesen
2008-01-18 16:10             ` Linus Torvalds
2008-01-18 20:55               ` David Schwartz
2008-01-18 17:37             ` Olivier Galibert
2008-01-18 18:06             ` DM
2008-01-18  7:51           ` Giacomo Catenazzi
2008-01-18  8:20             ` Giacomo Catenazzi
2008-01-18 13:53               ` Andy Lutomirski
2008-01-18 17:24                 ` Olivier Galibert
2008-01-18 22:29                   ` J.A. Magallón
2008-01-18 23:44                     ` Krzysztof Halasa
2008-01-18 13:54               ` Andy Lutomirski
2008-01-18 19:14                 ` Vadim Lobanov
2008-01-18 19:31                   ` Zan Lynx
2008-01-18 19:55                     ` Vadim Lobanov
2008-01-18  8:30             ` Vadim Lobanov
2008-01-18  9:48   ` Jakob Oestergaard
2008-01-18 11:47     ` Giacomo A. Catenazzi
2008-01-18 14:39       ` Jakob Oestergaard
2008-01-18 19:06       ` Vadim Lobanov
2008-01-18 13:31     ` Björn Steinbrink
2008-01-18 14:53       ` Jakob Oestergaard
  -- strict thread matches above, loose matches on Subject: below --
2008-01-18 12:45 ecolbus
2008-01-18 15:20 ` Giacomo A. Catenazzi
2008-01-18 16:45 ecolbus
2008-01-18 18:20 ` Olivier Galibert
2008-01-18 19:10 ecolbus
     [not found] <fa.cHMztHfqJXv7vw5O0nQ8SdTrma0@ifi.uio.no>
     [not found] ` <fa.V9M+5l8C/um5KEiBtZOjbJDQmu4@ifi.uio.no>
2013-01-12 19:18   ` antoine.trux
2013-01-13  8:10     ` Chen Gang F T
2013-01-13 17:41       ` Guenter Roeck
2013-01-14  1:45         ` Chen Gang F T
2013-01-13 20:54       ` Cong Ding
2013-01-14  1:18         ` Chen Gang F T

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