All of lore.kernel.org
 help / color / mirror / Atom feed
From: Namhyung Kim <namhyung.kim@lge.com>
To: Christoph Lameter <cl@gentwo.org>
Cc: Namhyung Kim <namhyung@gmail.com>,
	Pekka Enberg <penberg@kernel.org>, Matt Mackall <mpm@selenic.com>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH -next] slub: set PG_slab on all of slab pages
Date: Fri, 02 Mar 2012 16:12:52 +0900	[thread overview]
Message-ID: <4F5072F4.3030505@lge.com> (raw)
In-Reply-To: <alpine.DEB.2.00.1203010901020.5004@router.home>

2012-03-02 12:03 AM, Christoph Lameter wrote:
> On Thu, 1 Mar 2012, Namhyung Kim wrote:
>
>>> You cannot free a tail page of a compound higher order page independently.
>>> You must free the whole compound.
>>>
>>
>> I meant freeing a *slab object* resides in a compound page using buddy
>> system API (e.g. free_pages). I know it's definitely a programming
>> error. However there's no safety net to protect and/or warn such a
>> misbehavior AFAICS - except for head page which has PG_slab set - when
>> it happened by any chance.
>
> ?? One generally passed a struct page pointer to the page allocator. Slab
> allocator takes pointers to object. The calls that take a pointer to an
> object must have a page aligned value.
>

Please see free_pages(). It converts the pointer using virt_to_page().


>> Without it, it might be possible to free part of tail pages silently,
>> and cause unexpected not-so-funny results some time later. It should be
>> hard to find out.
>
> Ok then fix the page allocator to BUG() on tail pages. That is an issue
> with the page allocator not the slab allocator.
>
> Adding PG_tail to the flags checked on free should do the trick (at least
> for 64 bit).
>

Yeah, but doing it requires to change free path of compound pages. It seems 
freeing normal compound pages would not clear PG_head/tail bits before 
free_pages_check() called. I guess moving destroy_compound_page() into 
free_pages_prepare() will solved this issue but I want to make sure it's the 
right approach since I have no idea how it affects huge page behaviors.

Besides, as it has no effect on 32 bit kernels I still want add the PG_slab 
flag to those pages. If you care about the performance of hot path, how about 
adding it under debug configurations at least?


Thanks,
Namhyung Kim

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

WARNING: multiple messages have this Message-ID (diff)
From: Namhyung Kim <namhyung.kim@lge.com>
To: Christoph Lameter <cl@gentwo.org>
Cc: Namhyung Kim <namhyung@gmail.com>,
	Pekka Enberg <penberg@kernel.org>, Matt Mackall <mpm@selenic.com>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH -next] slub: set PG_slab on all of slab pages
Date: Fri, 02 Mar 2012 16:12:52 +0900	[thread overview]
Message-ID: <4F5072F4.3030505@lge.com> (raw)
In-Reply-To: <alpine.DEB.2.00.1203010901020.5004@router.home>

2012-03-02 12:03 AM, Christoph Lameter wrote:
> On Thu, 1 Mar 2012, Namhyung Kim wrote:
>
>>> You cannot free a tail page of a compound higher order page independently.
>>> You must free the whole compound.
>>>
>>
>> I meant freeing a *slab object* resides in a compound page using buddy
>> system API (e.g. free_pages). I know it's definitely a programming
>> error. However there's no safety net to protect and/or warn such a
>> misbehavior AFAICS - except for head page which has PG_slab set - when
>> it happened by any chance.
>
> ?? One generally passed a struct page pointer to the page allocator. Slab
> allocator takes pointers to object. The calls that take a pointer to an
> object must have a page aligned value.
>

Please see free_pages(). It converts the pointer using virt_to_page().


>> Without it, it might be possible to free part of tail pages silently,
>> and cause unexpected not-so-funny results some time later. It should be
>> hard to find out.
>
> Ok then fix the page allocator to BUG() on tail pages. That is an issue
> with the page allocator not the slab allocator.
>
> Adding PG_tail to the flags checked on free should do the trick (at least
> for 64 bit).
>

Yeah, but doing it requires to change free path of compound pages. It seems 
freeing normal compound pages would not clear PG_head/tail bits before 
free_pages_check() called. I guess moving destroy_compound_page() into 
free_pages_prepare() will solved this issue but I want to make sure it's the 
right approach since I have no idea how it affects huge page behaviors.

Besides, as it has no effect on 32 bit kernels I still want add the PG_slab 
flag to those pages. If you care about the performance of hot path, how about 
adding it under debug configurations at least?


Thanks,
Namhyung Kim

  reply	other threads:[~2012-03-02  7:12 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-02-29  8:54 [PATCH -next] slub: set PG_slab on all of slab pages Namhyung Kim
2012-02-29  8:54 ` Namhyung Kim
2012-02-29 15:24 ` Christoph Lameter
2012-02-29 15:24   ` Christoph Lameter
2012-03-01  7:30   ` Namhyung Kim
2012-03-01  7:30     ` Namhyung Kim
2012-03-01 15:03     ` Christoph Lameter
2012-03-01 15:03       ` Christoph Lameter
2012-03-02  7:12       ` Namhyung Kim [this message]
2012-03-02  7:12         ` Namhyung Kim
2012-03-02 16:13         ` Christoph Lameter
2012-03-02 16:13           ` Christoph Lameter
2012-03-04 10:34 ` Minchan Kim
2012-03-04 10:34   ` Minchan Kim
2012-03-05  8:42   ` Namhyung Kim
2012-03-05  8:42     ` Namhyung Kim
2012-03-05 10:59     ` Minchan Kim
2012-03-05 10:59       ` Minchan Kim
2012-03-05 14:48   ` Christoph Lameter
2012-03-05 14:48     ` Christoph Lameter
2012-03-06  1:16     ` Minchan Kim
2012-03-06  1:16       ` Minchan Kim

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=4F5072F4.3030505@lge.com \
    --to=namhyung.kim@lge.com \
    --cc=cl@gentwo.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mpm@selenic.com \
    --cc=namhyung@gmail.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.