Linux-mm Archive on lore.kernel.org
 help / color / Atom feed
From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
To: Minchan Kim <minchan@kernel.org>
Cc: "Shi, Yang" <yang.shi@linaro.org>,
	akpm@linux-foundation.org, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org, linaro-kernel@lists.linaro.org
Subject: Re: [PATCH] mm: check the return value of lookup_page_ext for all call sites
Date: Fri, 27 May 2016 15:08:39 +0900
Message-ID: <20160527060839.GC13661@js1304-P5Q-DELUXE> (raw)
In-Reply-To: <20160527051432.GF2322@bbox>

On Fri, May 27, 2016 at 02:14:32PM +0900, Minchan Kim wrote:
> On Thu, May 26, 2016 at 04:15:28PM -0700, Shi, Yang wrote:
> > On 5/25/2016 5:37 PM, Minchan Kim wrote:
> > >On Tue, May 24, 2016 at 11:58:11AM +0900, Minchan Kim wrote:
> > >>On Mon, May 23, 2016 at 10:16:08AM -0700, Yang Shi wrote:
> > >>>Per the discussion with Joonsoo Kim [1], we need check the return value of
> > >>>lookup_page_ext() for all call sites since it might return NULL in some cases,
> > >>>although it is unlikely, i.e. memory hotplug.
> > >>>
> > >>>Tested with ltp with "page_owner=0".
> > >>>
> > >>>[1] http://lkml.kernel.org/r/20160519002809.GA10245@js1304-P5Q-DELUXE
> > >>>
> > >>>Signed-off-by: Yang Shi <yang.shi@linaro.org>
> > >>
> > >>I didn't read code code in detail to see how page_ext memory space
> > >>allocated in boot code and memory hotplug but to me, it's not good
> > >>to check NULL whenever we calls lookup_page_ext.
> > >>
> > >>More dangerous thing is now page_ext is used by optionable feature(ie, not
> > >>critical for system stability) but if we want to use page_ext as
> > >>another important tool for the system in future,
> > >>it could be a serious problem.

Hello, Minchan.

I wonder how pages that isn't managed by kernel yet will cause serious
problem. Until onlining, these pages are out of our scope. Any
information about them would be useless until it is actually
activated. I guess that returning NULL for those pages will not hurt
any functionality. Do you have any possible scenario that this causes the
serious problem?

And, allocation such memory space doesn't come from free. If someone
just add the memory device and don't online it, these memory will be
wasted. I don't know if there is such a usecase but it's possible
scenario.

> > >>
> > >>Can we put some hooks of page_ext into memory-hotplug so guarantee
> > >>that page_ext memory space is allocated with memmap space at the
> > >>same time? IOW, once every PFN wakers find a page is valid, page_ext
> > >>is valid, too so lookup_page_ext never returns NULL on valid page
> > >>by design.
> > >>
> > >>I hope we consider this direction, too.
> > >
> > >Yang, Could you think about this?
> > 
> > Thanks a lot for the suggestion. Sorry for the late reply, I was
> > busy on preparing patches. I do agree this is a direction we should
> > look into, but I haven't got time to think about it deeper. I hope
> > Joonsoo could chime in too since he is the original author for page
> > extension.
> > 
> > >
> > >Even, your patch was broken, I think.
> > >It doesn't work with !CONFIG_DEBUG_VM && !CONFIG_PAGE_POISONING because
> > >lookup_page_ext doesn't return NULL in that case.
> > 
> > Actually, I think the #ifdef should be removed if lookup_page_ext()
> > is possible to return NULL. It sounds not make sense returning NULL
> > only when DEBUG_VM is enabled. It should return NULL no matter what
> > debug config is selected. If Joonsoo agrees with me I'm going to
> > come up with a patch to fix it.

Agreed but let's wait for Minchan's response.

Thanks.

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

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-05-23 17:16 Yang Shi
2016-05-24  2:58 ` Minchan Kim
2016-05-26  0:37   ` Minchan Kim
2016-05-26 23:15     ` Shi, Yang
2016-05-27  5:14       ` Minchan Kim
2016-05-27  6:08         ` Joonsoo Kim [this message]
2016-05-27  8:11           ` Minchan Kim
2016-05-27 18:16             ` Shi, Yang
2016-05-30  6:11               ` Minchan Kim
2016-06-01 20:40                 ` Shi, Yang
2016-06-02  5:00                   ` Minchan Kim
2016-06-02 23:15                     ` Shi, Yang
2016-05-30  5:39             ` Joonsoo Kim
2016-05-30  6:08               ` Minchan Kim
2016-06-01 20:52                 ` Shi, Yang
2016-05-27 20:02       ` Andrew Morton
2016-05-27 20:17         ` Shi, Yang
2016-05-27 20:30           ` Andrew Morton
2016-05-24 10:08 ` [PATCH] mm: fix build problems from lookup_page_ext Arnd Bergmann
2016-05-24 16:33   ` Shi, Yang
2016-05-25  7:12 ` [PATCH] mm: check the return value of lookup_page_ext for all call sites shakil
2016-05-30  6:17 ` 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=20160527060839.GC13661@js1304-P5Q-DELUXE \
    --to=iamjoonsoo.kim@lge.com \
    --cc=akpm@linux-foundation.org \
    --cc=linaro-kernel@lists.linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=minchan@kernel.org \
    --cc=yang.shi@linaro.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

Linux-mm Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mm/0 linux-mm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mm linux-mm/ https://lore.kernel.org/linux-mm \
		linux-mm@kvack.org
	public-inbox-index linux-mm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kvack.linux-mm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git