All of lore.kernel.org
 help / color / mirror / Atom feed
From: Will Deacon <will.deacon@arm.com>
To: Robert Richter <rric@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>,
	Robert Richter <robert.richter@cavium.com>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	David Daney <david.daney@cavium.com>,
	Hanjun Guo <hanjun.guo@linaro.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"linux-efi@vger.kernel.org" <linux-efi@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] arm64: mm: Fix memmap to be initialized for the entire section
Date: Thu, 27 Oct 2016 17:01:36 +0100	[thread overview]
Message-ID: <20161027160136.GD24290@arm.com> (raw)
In-Reply-To: <20161017185801.GT25086@rric.localdomain>

Hi Robert,

On Mon, Oct 17, 2016 at 08:58:01PM +0200, Robert Richter wrote:
> Mark, Will, any opinion here?

Having looking at this, I'm inclined to agree with you; pfn_valid() is
all about whether the underlying mem_map (struct page *) entry exists,
not about whether the page is mappable or not.

That said, setting the zone for pages representing NOMAP memory feels
like a slippery slope to losing information about them being NOMAP in
the first place and the whole problem getting out-of-hand. Whilst I'm
happy for pfn_valid() to return true (in the sense that we're within
bounds of mem_map etc), I'm less happy that we're also saying that the
struct page contains useful information, such as the zone and the node
information, which is then subsequently used by the NUMA code.

On top of that, pfn_valid is used in other places as a coarse "is this
memory?" check, and will cause things like ioremap to fail whereas it
wouldn't at the moment. It feels to me like NOMAP memory is a new type
of memory where there *is* a struct page, but it shouldn't be used for
anything. I don't think pfn_valid can describe that, given the way it's
currently used, and flipping the logic is just likely to move the problem
elsewhere.

What options do we have for fixing this in the NUMA code?

Will

WARNING: multiple messages have this Message-ID (diff)
From: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
To: Robert Richter <rric-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>,
	Robert Richter
	<robert.richter-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>,
	Ard Biesheuvel
	<ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>,
	Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org>,
	David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>,
	Hanjun Guo <hanjun.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>,
	"linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org"
	<linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>,
	"linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	"linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Subject: Re: [PATCH] arm64: mm: Fix memmap to be initialized for the entire section
Date: Thu, 27 Oct 2016 17:01:36 +0100	[thread overview]
Message-ID: <20161027160136.GD24290@arm.com> (raw)
In-Reply-To: <20161017185801.GT25086-vWBEXY7mpu582hYKe6nXyg@public.gmane.org>

Hi Robert,

On Mon, Oct 17, 2016 at 08:58:01PM +0200, Robert Richter wrote:
> Mark, Will, any opinion here?

Having looking at this, I'm inclined to agree with you; pfn_valid() is
all about whether the underlying mem_map (struct page *) entry exists,
not about whether the page is mappable or not.

That said, setting the zone for pages representing NOMAP memory feels
like a slippery slope to losing information about them being NOMAP in
the first place and the whole problem getting out-of-hand. Whilst I'm
happy for pfn_valid() to return true (in the sense that we're within
bounds of mem_map etc), I'm less happy that we're also saying that the
struct page contains useful information, such as the zone and the node
information, which is then subsequently used by the NUMA code.

On top of that, pfn_valid is used in other places as a coarse "is this
memory?" check, and will cause things like ioremap to fail whereas it
wouldn't at the moment. It feels to me like NOMAP memory is a new type
of memory where there *is* a struct page, but it shouldn't be used for
anything. I don't think pfn_valid can describe that, given the way it's
currently used, and flipping the logic is just likely to move the problem
elsewhere.

What options do we have for fixing this in the NUMA code?

Will

WARNING: multiple messages have this Message-ID (diff)
From: will.deacon@arm.com (Will Deacon)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] arm64: mm: Fix memmap to be initialized for the entire section
Date: Thu, 27 Oct 2016 17:01:36 +0100	[thread overview]
Message-ID: <20161027160136.GD24290@arm.com> (raw)
In-Reply-To: <20161017185801.GT25086@rric.localdomain>

Hi Robert,

On Mon, Oct 17, 2016 at 08:58:01PM +0200, Robert Richter wrote:
> Mark, Will, any opinion here?

Having looking at this, I'm inclined to agree with you; pfn_valid() is
all about whether the underlying mem_map (struct page *) entry exists,
not about whether the page is mappable or not.

That said, setting the zone for pages representing NOMAP memory feels
like a slippery slope to losing information about them being NOMAP in
the first place and the whole problem getting out-of-hand. Whilst I'm
happy for pfn_valid() to return true (in the sense that we're within
bounds of mem_map etc), I'm less happy that we're also saying that the
struct page contains useful information, such as the zone and the node
information, which is then subsequently used by the NUMA code.

On top of that, pfn_valid is used in other places as a coarse "is this
memory?" check, and will cause things like ioremap to fail whereas it
wouldn't at the moment. It feels to me like NOMAP memory is a new type
of memory where there *is* a struct page, but it shouldn't be used for
anything. I don't think pfn_valid can describe that, given the way it's
currently used, and flipping the logic is just likely to move the problem
elsewhere.

What options do we have for fixing this in the NUMA code?

Will

  reply	other threads:[~2016-10-27 16:01 UTC|newest]

Thread overview: 86+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-10-06  9:52 [PATCH] arm64: mm: Fix memmap to be initialized for the entire section Robert Richter
2016-10-06  9:52 ` Robert Richter
2016-10-06  9:52 ` Robert Richter
2016-10-06 10:00 ` Ard Biesheuvel
2016-10-06 10:00   ` Ard Biesheuvel
2016-10-06 10:00   ` Ard Biesheuvel
2016-10-06 16:11   ` Robert Richter
2016-10-06 16:11     ` Robert Richter
2016-10-06 16:11     ` Robert Richter
2016-10-17 18:58     ` Robert Richter
2016-10-17 18:58       ` Robert Richter
2016-10-17 18:58       ` Robert Richter
2016-10-27 16:01       ` Will Deacon [this message]
2016-10-27 16:01         ` Will Deacon
2016-10-27 16:01         ` Will Deacon
2016-10-28  9:19         ` Robert Richter
2016-10-28  9:19           ` Robert Richter
2016-10-28  9:19           ` Robert Richter
2016-11-07 21:05           ` Will Deacon
2016-11-07 21:05             ` Will Deacon
2016-11-07 21:05             ` Will Deacon
2016-11-09 19:51             ` Robert Richter
2016-11-09 19:51               ` Robert Richter
2016-11-09 19:51               ` Robert Richter
2016-11-17 14:25               ` Will Deacon
2016-11-17 14:25                 ` Will Deacon
2016-11-17 14:25                 ` Will Deacon
2016-11-17 15:18                 ` Robert Richter
2016-11-17 15:18                   ` Robert Richter
2016-11-17 15:18                   ` Robert Richter
2016-11-20 17:07                   ` Ard Biesheuvel
2016-11-20 17:07                     ` Ard Biesheuvel
2016-11-20 17:07                     ` Ard Biesheuvel
2016-11-23 21:15                     ` Robert Richter
2016-11-23 21:15                       ` Robert Richter
2016-11-23 21:15                       ` Robert Richter
2016-11-23 21:25                       ` Ard Biesheuvel
2016-11-23 21:25                         ` Ard Biesheuvel
2016-11-23 21:25                         ` Ard Biesheuvel
2016-11-24 13:42                         ` Robert Richter
2016-11-24 13:42                           ` Robert Richter
2016-11-24 13:42                           ` Robert Richter
2016-11-24 13:44                           ` Ard Biesheuvel
2016-11-24 13:44                             ` Ard Biesheuvel
2016-11-24 13:44                             ` Ard Biesheuvel
2016-11-24 13:51                             ` Robert Richter
2016-11-24 13:51                               ` Robert Richter
2016-11-24 13:51                               ` Robert Richter
2016-11-24 13:58                               ` Ard Biesheuvel
2016-11-24 13:58                                 ` Ard Biesheuvel
2016-11-24 13:58                                 ` Ard Biesheuvel
2016-11-24 14:11                                 ` Robert Richter
2016-11-24 14:11                                   ` Robert Richter
2016-11-24 14:11                                   ` Robert Richter
2016-11-24 14:23                                   ` Ard Biesheuvel
2016-11-24 14:23                                     ` Ard Biesheuvel
2016-11-24 14:23                                     ` Ard Biesheuvel
2016-11-24 15:09                                     ` Robert Richter
2016-11-24 15:09                                       ` Robert Richter
2016-11-24 15:09                                       ` Robert Richter
2016-11-24 19:26                                       ` Robert Richter
2016-11-24 19:26                                         ` Robert Richter
2016-11-24 19:26                                         ` Robert Richter
2016-11-24 19:42                                         ` Ard Biesheuvel
2016-11-24 19:42                                           ` Ard Biesheuvel
2016-11-24 19:42                                           ` Ard Biesheuvel
2016-11-25 11:29                                           ` Robert Richter
2016-11-25 11:29                                             ` Robert Richter
2016-11-25 11:29                                             ` Robert Richter
2016-11-25 12:28                                             ` Ard Biesheuvel
2016-11-25 12:28                                               ` Ard Biesheuvel
2016-11-25 12:28                                               ` Ard Biesheuvel
2016-11-25 17:01                                               ` Ard Biesheuvel
2016-11-25 17:01                                                 ` Ard Biesheuvel
2016-11-25 17:01                                                 ` Ard Biesheuvel
2016-10-18 10:18     ` Mark Rutland
2016-10-18 10:18       ` Mark Rutland
2016-10-18 10:18       ` Mark Rutland
2016-10-18 15:02       ` Robert Richter
2016-10-18 15:02         ` Robert Richter
2016-10-18 15:02         ` Robert Richter
2016-10-10 15:33 ` David Daney
2016-10-10 15:33   ` David Daney
2016-11-01 16:55 ` Robert Richter
2016-11-01 16:55   ` Robert Richter
2016-11-01 16:55   ` Robert Richter

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=20161027160136.GD24290@arm.com \
    --to=will.deacon@arm.com \
    --cc=ard.biesheuvel@linaro.org \
    --cc=catalin.marinas@arm.com \
    --cc=david.daney@cavium.com \
    --cc=hanjun.guo@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-efi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=robert.richter@cavium.com \
    --cc=rric@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.