All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mel Gorman <mgorman@suse.de>
To: Anshuman Khandual <khandual@linux.vnet.ibm.com>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	mhocko@suse.com, vbabka@suse.cz, minchan@kernel.org,
	aneesh.kumar@linux.vnet.ibm.com, bsingharora@gmail.com,
	srikar@linux.vnet.ibm.com, haren@linux.vnet.ibm.com,
	jglisse@redhat.com, dave.hansen@intel.com,
	dan.j.williams@intel.com
Subject: Re: [PATCH V3 0/4] Define coherent device memory node
Date: Wed, 15 Feb 2017 18:20:10 +0000	[thread overview]
Message-ID: <20170215182010.reoahjuei5eaxr5s@suse.de> (raw)
In-Reply-To: <20170215120726.9011-1-khandual@linux.vnet.ibm.com>

On Wed, Feb 15, 2017 at 05:37:22PM +0530, Anshuman Khandual wrote:
> 	This four patches define CDM node with HugeTLB & Buddy allocation
> isolation. Please refer to the last RFC posting mentioned here for more

Always include the background with the changelog itself. Do not assume that
people are willing to trawl through a load of past postings to assemble
the picture. I'm only taking a brief look because of the page allocator
impact but it does not appear that previous feedback was addressed.

In itself, the series does very little and as Vlastimil already pointed
out, it's not a good idea to try merge piecemeal when people could not
agree on the big picture (I didn't dig into it).

The only reason I'm commenting at all is to say that I am extremely opposed
to the changes made to the page allocator paths that are specific to
CDM. It's been continual significant effort to keep the cost there down
and this is a mess of special cases for CDM. The changes to hugetlb to
identify "memory that is not really memory" with special casing is also
quite horrible.

It's completely unclear that even if one was to assume that CDM memory
should be expressed as nodes why such systems do not isolate all processes
from CDM nodes by default and then allow access via memory policies or
cpusets instead of special casing the page allocator fast path. It's also
completely unclear what happens if a device should then access the CDM
and how that should be synchronised with the core, if that is even possible.

It's also unclear if this is even usable by an application in userspace
at this point in time. If it is and the special casing is needed then the
regions should be isolated from early mem allocations in the arch layer
that is CDM aware, initialised late, and then setup userspace to isolate
all but privileged applications from the CDM nodes. Do not litter the core
with is_cdm_whatever checks.

At best this is incomplete because it does not look as if it could be used
by anything properly and the fast path alterations are horrible even if
it could be used. As it is, it should not be merged in my opinion.

-- 
Mel Gorman
SUSE Labs

WARNING: multiple messages have this Message-ID (diff)
From: Mel Gorman <mgorman@suse.de>
To: Anshuman Khandual <khandual@linux.vnet.ibm.com>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	mhocko@suse.com, vbabka@suse.cz, minchan@kernel.org,
	aneesh.kumar@linux.vnet.ibm.com, bsingharora@gmail.com,
	srikar@linux.vnet.ibm.com, haren@linux.vnet.ibm.com,
	jglisse@redhat.com, dave.hansen@intel.com,
	dan.j.williams@intel.com
Subject: Re: [PATCH V3 0/4] Define coherent device memory node
Date: Wed, 15 Feb 2017 18:20:10 +0000	[thread overview]
Message-ID: <20170215182010.reoahjuei5eaxr5s@suse.de> (raw)
In-Reply-To: <20170215120726.9011-1-khandual@linux.vnet.ibm.com>

On Wed, Feb 15, 2017 at 05:37:22PM +0530, Anshuman Khandual wrote:
> 	This four patches define CDM node with HugeTLB & Buddy allocation
> isolation. Please refer to the last RFC posting mentioned here for more

Always include the background with the changelog itself. Do not assume that
people are willing to trawl through a load of past postings to assemble
the picture. I'm only taking a brief look because of the page allocator
impact but it does not appear that previous feedback was addressed.

In itself, the series does very little and as Vlastimil already pointed
out, it's not a good idea to try merge piecemeal when people could not
agree on the big picture (I didn't dig into it).

The only reason I'm commenting at all is to say that I am extremely opposed
to the changes made to the page allocator paths that are specific to
CDM. It's been continual significant effort to keep the cost there down
and this is a mess of special cases for CDM. The changes to hugetlb to
identify "memory that is not really memory" with special casing is also
quite horrible.

It's completely unclear that even if one was to assume that CDM memory
should be expressed as nodes why such systems do not isolate all processes
from CDM nodes by default and then allow access via memory policies or
cpusets instead of special casing the page allocator fast path. It's also
completely unclear what happens if a device should then access the CDM
and how that should be synchronised with the core, if that is even possible.

It's also unclear if this is even usable by an application in userspace
at this point in time. If it is and the special casing is needed then the
regions should be isolated from early mem allocations in the arch layer
that is CDM aware, initialised late, and then setup userspace to isolate
all but privileged applications from the CDM nodes. Do not litter the core
with is_cdm_whatever checks.

At best this is incomplete because it does not look as if it could be used
by anything properly and the fast path alterations are horrible even if
it could be used. As it is, it should not be merged in my opinion.

-- 
Mel Gorman
SUSE Labs

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

  parent reply	other threads:[~2017-02-15 18:20 UTC|newest]

Thread overview: 86+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-15 12:07 [PATCH V3 0/4] Define coherent device memory node Anshuman Khandual
2017-02-15 12:07 ` Anshuman Khandual
2017-02-15 12:07 ` [PATCH V3 1/4] mm: Define coherent device memory (CDM) node Anshuman Khandual
2017-02-15 12:07   ` Anshuman Khandual
2017-02-17 14:05   ` Bob Liu
2017-02-17 14:05     ` Bob Liu
2017-02-21 10:20     ` Anshuman Khandual
2017-02-21 10:20       ` Anshuman Khandual
2017-02-15 12:07 ` [PATCH V3 2/4] mm: Enable HugeTLB allocation isolation for CDM nodes Anshuman Khandual
2017-02-15 12:07   ` Anshuman Khandual
2017-02-15 12:07 ` [PATCH V3 3/4] mm: Add new parameter to get_page_from_freelist() function Anshuman Khandual
2017-02-15 12:07   ` Anshuman Khandual
2017-02-15 12:07 ` [PATCH V3 4/4] mm: Enable Buddy allocation isolation for CDM nodes Anshuman Khandual
2017-02-15 12:07   ` Anshuman Khandual
2017-02-15 18:20 ` Mel Gorman [this message]
2017-02-15 18:20   ` [PATCH V3 0/4] Define coherent device memory node Mel Gorman
2017-02-16 22:14   ` Balbir Singh
2017-02-16 22:14     ` Balbir Singh
2017-02-17  9:33     ` Mel Gorman
2017-02-17  9:33       ` Mel Gorman
2017-02-21  2:57       ` Balbir Singh
2017-02-21  2:57         ` Balbir Singh
2017-03-01  2:42         ` Balbir Singh
2017-03-01  2:42           ` Balbir Singh
2017-03-01  9:55           ` Mel Gorman
2017-03-01  9:55             ` Mel Gorman
2017-03-01 10:59             ` Balbir Singh
2017-03-01 10:59               ` Balbir Singh
2017-03-08  9:04               ` Anshuman Khandual
2017-03-08  9:04                 ` Anshuman Khandual
2017-03-08  9:21                 ` [PATCH 1/2] mm: Change generic FALLBACK zonelist creation process Anshuman Khandual
2017-03-08  9:21                   ` Anshuman Khandual
2017-03-08 11:07                   ` John Hubbard
2017-03-08 11:07                     ` John Hubbard
2017-03-14 13:33                     ` Anshuman Khandual
2017-03-14 13:33                       ` Anshuman Khandual
2017-03-15  4:10                       ` John Hubbard
2017-03-15  4:10                         ` John Hubbard
2017-03-08  9:21                 ` [PATCH 2/2] mm: Change mbind(MPOL_BIND) implementation for CDM nodes Anshuman Khandual
2017-03-08  9:21                   ` Anshuman Khandual
2017-02-17 11:41   ` [PATCH V3 0/4] Define coherent device memory node Anshuman Khandual
2017-02-17 11:41     ` Anshuman Khandual
2017-02-17 13:32     ` Mel Gorman
2017-02-17 13:32       ` Mel Gorman
2017-02-21 13:09       ` Anshuman Khandual
2017-02-21 13:09         ` Anshuman Khandual
2017-02-21 20:14         ` Jerome Glisse
2017-02-21 20:14           ` Jerome Glisse
2017-02-23  8:14           ` Anshuman Khandual
2017-02-23  8:14             ` Anshuman Khandual
2017-02-23 15:27             ` Jerome Glisse
2017-02-23 15:27               ` Jerome Glisse
2017-02-22  9:29         ` Michal Hocko
2017-02-22  9:29           ` Michal Hocko
2017-02-22 14:59           ` Jerome Glisse
2017-02-22 14:59             ` Jerome Glisse
2017-02-22 16:54             ` Michal Hocko
2017-02-22 16:54               ` Michal Hocko
2017-03-06  5:48               ` Anshuman Khandual
2017-03-06  5:48                 ` Anshuman Khandual
2017-02-23  8:52           ` Anshuman Khandual
2017-02-23  8:52             ` Anshuman Khandual
2017-02-23 15:57         ` Mel Gorman
2017-02-23 15:57           ` Mel Gorman
2017-03-06  5:12           ` Anshuman Khandual
2017-03-06  5:12             ` Anshuman Khandual
2017-02-21 11:11     ` Michal Hocko
2017-02-21 11:11       ` Michal Hocko
2017-02-21 13:39       ` Anshuman Khandual
2017-02-21 13:39         ` Anshuman Khandual
2017-02-22  9:50         ` Michal Hocko
2017-02-22  9:50           ` Michal Hocko
2017-02-23  6:52           ` Anshuman Khandual
2017-02-23  6:52             ` Anshuman Khandual
2017-03-05 12:39             ` Anshuman Khandual
2017-03-05 12:39               ` Anshuman Khandual
2017-02-24  1:06         ` Bob Liu
2017-02-24  1:06           ` Bob Liu
2017-02-24  4:39           ` John Hubbard
2017-02-24  4:39             ` John Hubbard
2017-02-24  4:53           ` Jerome Glisse
2017-02-24  4:53             ` Jerome Glisse
2017-02-27  1:56             ` Bob Liu
2017-02-27  1:56               ` Bob Liu
2017-02-27  5:41               ` Anshuman Khandual
2017-02-27  5:41                 ` Anshuman Khandual

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=20170215182010.reoahjuei5eaxr5s@suse.de \
    --to=mgorman@suse.de \
    --cc=aneesh.kumar@linux.vnet.ibm.com \
    --cc=bsingharora@gmail.com \
    --cc=dan.j.williams@intel.com \
    --cc=dave.hansen@intel.com \
    --cc=haren@linux.vnet.ibm.com \
    --cc=jglisse@redhat.com \
    --cc=khandual@linux.vnet.ibm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@suse.com \
    --cc=minchan@kernel.org \
    --cc=srikar@linux.vnet.ibm.com \
    --cc=vbabka@suse.cz \
    /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.