| Message ID | 20181127023630.9066-1-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm, sparse: drop pgdat_resize_lock in sparse_add/remove_one_section() | expand |
[Cc Dave who has added the lock into this path. Maybe he remembers why] On Tue 27-11-18 10:36:30, Wei Yang wrote: > pgdat_resize_lock is used to protect pgdat's memory region information > like: node_start_pfn, node_present_pages, etc. > > In function sparse_add/remove_one_section(), those data is not touched. > This means it is not necessary to acquire pgdat_resize_lock to protect > this area. > > Since the information needed in sparse_add_one_section() is node id to > allocate proper memory. This patch also changes the prototype of > sparse_add_one_section() to pass node id directly. This is intended to > reduce misleading that sparse_add_one_section() would touch pgdat. > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > --- > include/linux/memory_hotplug.h | 2 +- > mm/memory_hotplug.c | 2 +- > mm/sparse.c | 17 +++++------------ > 3 files changed, 7 insertions(+), 14 deletions(-) > > diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h > index 45a5affcab8a..3787d4e913e6 100644 > --- a/include/linux/memory_hotplug.h > +++ b/include/linux/memory_hotplug.h > @@ -333,7 +333,7 @@ extern void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn, > unsigned long nr_pages, struct vmem_altmap *altmap); > extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages); > extern bool is_memblock_offlined(struct memory_block *mem); > -extern int sparse_add_one_section(struct pglist_data *pgdat, > +extern int sparse_add_one_section(int nid, > unsigned long start_pfn, struct vmem_altmap *altmap); > extern void sparse_remove_one_section(struct zone *zone, struct mem_section *ms, > unsigned long map_offset, struct vmem_altmap *altmap); > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > index f626e7e5f57b..5b3a3d7b4466 100644 > --- a/mm/memory_hotplug.c > +++ b/mm/memory_hotplug.c > @@ -253,7 +253,7 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn, > if (pfn_valid(phys_start_pfn)) > return -EEXIST; > > - ret = sparse_add_one_section(NODE_DATA(nid), phys_start_pfn, altmap); > + ret = sparse_add_one_section(nid, phys_start_pfn, altmap); > if (ret < 0) > return ret; > > diff --git a/mm/sparse.c b/mm/sparse.c > index 33307fc05c4d..a4fdbcb21514 100644 > --- a/mm/sparse.c > +++ b/mm/sparse.c > @@ -662,25 +662,24 @@ static void free_map_bootmem(struct page *memmap) > * set. If this is <=0, then that means that the passed-in > * map was not consumed and must be freed. > */ > -int __meminit sparse_add_one_section(struct pglist_data *pgdat, > - unsigned long start_pfn, struct vmem_altmap *altmap) > +int __meminit sparse_add_one_section(int nid, unsigned long start_pfn, > + struct vmem_altmap *altmap) > { > unsigned long section_nr = pfn_to_section_nr(start_pfn); > struct mem_section *ms; > struct page *memmap; > unsigned long *usemap; > - unsigned long flags; > int ret; > > /* > * no locking for this, because it does its own > * plus, it does a kmalloc > */ > - ret = sparse_index_init(section_nr, pgdat->node_id); > + ret = sparse_index_init(section_nr, nid); > if (ret < 0 && ret != -EEXIST) > return ret; > ret = 0; > - memmap = kmalloc_section_memmap(section_nr, pgdat->node_id, altmap); > + memmap = kmalloc_section_memmap(section_nr, nid, altmap); > if (!memmap) > return -ENOMEM; > usemap = __kmalloc_section_usemap(); > @@ -689,8 +688,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat, > return -ENOMEM; > } > > - pgdat_resize_lock(pgdat, &flags); > - > ms = __pfn_to_section(start_pfn); > if (ms->section_mem_map & SECTION_MARKED_PRESENT) { > ret = -EEXIST; > @@ -707,7 +704,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat, > sparse_init_one_section(ms, section_nr, memmap, usemap); > > out: > - pgdat_resize_unlock(pgdat, &flags); > if (ret < 0) { > kfree(usemap); > __kfree_section_memmap(memmap, altmap); > @@ -769,10 +765,8 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms, > unsigned long map_offset, struct vmem_altmap *altmap) > { > struct page *memmap = NULL; > - unsigned long *usemap = NULL, flags; > - struct pglist_data *pgdat = zone->zone_pgdat; > + unsigned long *usemap = NULL; > > - pgdat_resize_lock(pgdat, &flags); > if (ms->section_mem_map) { > usemap = ms->pageblock_flags; > memmap = sparse_decode_mem_map(ms->section_mem_map, > @@ -780,7 +774,6 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms, > ms->section_mem_map = 0; > ms->pageblock_flags = NULL; > } > - pgdat_resize_unlock(pgdat, &flags); > > clear_hwpoisoned_pages(memmap + map_offset, > PAGES_PER_SECTION - map_offset); > -- > 2.15.1 >
On 11/26/18 10:25 PM, Michal Hocko wrote:
> [Cc Dave who has added the lock into this path. Maybe he remembers why]
I don't remember specifically. But, the pattern of:
allocate
lock
set
unlock
is _usually_ so we don't have two "sets" racing with each other. In
this case, that would have been to ensure that two
sparse_init_one_section()'s didn't race and leak one of the two
allocated memmaps or worse.
I think mem_hotplug_lock protects this case these days, though. I don't
think we had it in the early days and were just slumming it with the
pgdat locks.
I really don't like the idea of removing the lock by just saying it
doesn't protect anything without doing some homework first, though. It
would actually be really nice to comment the entire call chain from the
mem_hotplug_lock acquisition to here. There is precious little
commenting in there and it could use some love.
On Mon 26-11-18 23:17:40, Dave Hansen wrote: > On 11/26/18 10:25 PM, Michal Hocko wrote: > > [Cc Dave who has added the lock into this path. Maybe he remembers why] > > I don't remember specifically. But, the pattern of: > > allocate > lock > set > unlock > > is _usually_ so we don't have two "sets" racing with each other. In > this case, that would have been to ensure that two > sparse_init_one_section()'s didn't race and leak one of the two > allocated memmaps or worse. > > I think mem_hotplug_lock protects this case these days, though. I don't > think we had it in the early days and were just slumming it with the > pgdat locks. > > I really don't like the idea of removing the lock by just saying it > doesn't protect anything without doing some homework first, though. It > would actually be really nice to comment the entire call chain from the > mem_hotplug_lock acquisition to here. There is precious little > commenting in there and it could use some love. Agreed. It really seems like the lock is not needed anymore but a more trhougout analysis and explanation is definitely due. Thanks!
> I think mem_hotplug_lock protects this case these days, though. I > don't > think we had it in the early days and were just slumming it with the > pgdat locks. Yes, it does. > > I really don't like the idea of removing the lock by just saying it > doesn't protect anything without doing some homework first, though. It > would actually be really nice to comment the entire call chain from the > mem_hotplug_lock acquisition to here. There is precious little > commenting in there and it could use some love. [hot-add operation] add_memory_resource : acquire mem_hotplug lock arch_add_memory add_pages __add_pages __add_section sparse_add_one_section sparse_init_one_section [hot-remove operation] __remove_memory : acquire mem_hotplug lock arch_remove_memory __remove_pages __remove_section sparse_remove_one_section Both operations are serialized by the mem_hotplug lock, so they cannot step on each other's feet. Now, there seems to be an agreement/thought to remove the global mem_hotplug lock, in favor of a range locking for hot-add/remove and online/offline stage. So, although removing the lock here is pretty straightforward, it does not really get us closer to that goal IMHO, if that is what we want to do in the end.
[I am mostly offline and will be so tomorrow as well] On Tue 27-11-18 08:52:14, osalvador@suse.de wrote: [...] > So, although removing the lock here is pretty straightforward, it does not > really get us closer to that goal IMHO, if that is what we want to do in the > end. But it doesn't get us further either, right? This patch shouldn't make any plan for range locking any worse. Both adding and removing a sparse section is pfn range defined unless I am missing something.
On 2018-11-27 09:00, Michal Hocko wrote: > [I am mostly offline and will be so tomorrow as well] > > On Tue 27-11-18 08:52:14, osalvador@suse.de wrote: > [...] >> So, although removing the lock here is pretty straightforward, it does >> not >> really get us closer to that goal IMHO, if that is what we want to do >> in the >> end. > > But it doesn't get us further either, right? This patch shouldn't make > any plan for range locking any worse. Both adding and removing a sparse > section is pfn range defined unless I am missing something. Yes, you are right, it should not have any impact. It just felt like "we do already have the global lock, so let us stick with that". But the less unneeded locks we have in our way, the better. Sorry for the confusion.
On Tue, Nov 27, 2018 at 08:52:14AM +0100, osalvador@suse.de wrote: >> I think mem_hotplug_lock protects this case these days, though. I don't >> think we had it in the early days and were just slumming it with the >> pgdat locks. > >Yes, it does. > >> >> I really don't like the idea of removing the lock by just saying it >> doesn't protect anything without doing some homework first, though. It >> would actually be really nice to comment the entire call chain from the >> mem_hotplug_lock acquisition to here. There is precious little >> commenting in there and it could use some love. > >[hot-add operation] >add_memory_resource : acquire mem_hotplug lock > arch_add_memory > add_pages > __add_pages > __add_section > sparse_add_one_section > sparse_init_one_section > >[hot-remove operation] >__remove_memory : acquire mem_hotplug lock > arch_remove_memory > __remove_pages > __remove_section > sparse_remove_one_section > Thanks for this detailed analysis. >Both operations are serialized by the mem_hotplug lock, so they cannot step >on each other's feet. > >Now, there seems to be an agreement/thought to remove the global mem_hotplug >lock, in favor of a range locking for hot-add/remove and online/offline >stage. >So, although removing the lock here is pretty straightforward, it does not >really get us closer to that goal IMHO, if that is what we want to do in the >end. > My current idea is : we can try to get rid of global mem_hotplug_lock in logical online/offline phase first, and leave the physical add/remove phase serialized by mem_hotplug_lock for now. There are two phase in memory hotplug: * physical add/remove phase * logical online/offline phase Currently, both of them are protected by the global mem_hotplug_lock. While get rid of the this in logical online/offline phase is a little easier to me, since this procedure is protected by memory_block_dev's lock. This ensures there is no pfn over lap during parallel processing. The physical add/remove phase is a little harder, because it will touch * memblock * kernel page table * node online * sparse mem And I don't see a similar lock as memory_block_dev's lock. Below is the call trace for these two phase and I list some suspicious point which is not safe without mem_hotplug_lock. 1. physical phase __add_memory() register_memory_resource() <- protected by resource_lock add_memory_resource() mem_hotplug_begin() memblock_add_node() <- add to memblock.memory, not safe __try_online_node() <- not safe, related to node_set_online() arch_add_memory() init_memory_mapping() <- not safe add_pages() __add_pages() __add_section() sparse_add_one_section() update_end_of_memory_vars() <- not safe node_set_online(nid) <- need to hold mem_hotplug __register_one_node(nid) link_mem_sections() firmware_map_add_hotplug() mem_hotplug_done() 2. logical phase device_lock(memory_block_dev) online_pages() mem_hotplug_begin() mem = find_memory_block() <- not zone = move_pfn_range() zone_for_pfn_range(); move_pfn_range_to_zone() !populated_zone() setup_zone_pageset(zone) online_pages_range() <- looks safe build_all_zonelists() <- not init_per_zone_wmark_min() <- not kswapd_run() <- may not vm_total_pages = nr_free_pagecache_pages() mem_hotplug_done() device_unlock(memory_block_dev)
On Mon, Nov 26, 2018 at 11:17:40PM -0800, Dave Hansen wrote: >On 11/26/18 10:25 PM, Michal Hocko wrote: >> [Cc Dave who has added the lock into this path. Maybe he remembers why] > >I don't remember specifically. But, the pattern of: > > allocate > lock > set > unlock > >is _usually_ so we don't have two "sets" racing with each other. In >this case, that would have been to ensure that two >sparse_init_one_section()'s didn't race and leak one of the two >allocated memmaps or worse. > >I think mem_hotplug_lock protects this case these days, though. I don't >think we had it in the early days and were just slumming it with the >pgdat locks. > >I really don't like the idea of removing the lock by just saying it >doesn't protect anything without doing some homework first, though. It >would actually be really nice to comment the entire call chain from the >mem_hotplug_lock acquisition to here. There is precious little >commenting in there and it could use some love. Dave, Thanks for your comment :-) I should put more words to the reason for removing the lock. Here is a simplified call trace for sparse_add_one_section() during physical add/remove phase. __add_memory() add_memory_resource() mem_hotplug_begin() arch_add_memory() add_pages() __add_pages() __add_section() sparse_add_one_section(pfn) mem_hotplug_done() When we just look at the sparse section initialization, we can see the contention happens when __add_memory() try to add a same range or range overlapped in SECTIONS_PER_ROOT number of sections. Otherwise, they won't access the same memory. If this happens, we may face two contentions: * reallocation of mem_section[root] * reallocation of memmap and usemap While neither of them could be protected by the pgdat_resize_lock from my understanding. Grab pgdat_resize_lock just slow down the process, while finally they will replace the mem_section[root] and ms->section_mem_map with their own new allocated data. Last bu not the least, to be honest, even the global mem_hotplug_lock doesn't help in this situation. In case __add_memory() try to add the same range twice, the sparse section would be initialized twice. Which means it will be overwritten with the new allocated memmap/usermap. But maybe we have the assumption this reentrance will not happen. This is all what I understand, in case there is some misunderstanding, please let me know.
> My current idea is :
I do not want to hold you back.
I think that if you send a V2 detailing why we should be safe removing
the pgdat lock it is fine (memhotplug lock protects us).
We can later on think about the range locking, but that is another
discussion.
Sorry for having brought in that topic here, out of scope.
On Wed, Nov 28, 2018 at 09:19:27AM +0100, Oscar Salvador wrote: >> My current idea is : > >I do not want to hold you back. >I think that if you send a V2 detailing why we should be safe removing >the pgdat lock it is fine (memhotplug lock protects us). Fine. > >We can later on think about the range locking, but that is another >discussion. >Sorry for having brought in that topic here, out of scope. > >-- >Oscar Salvador >SUSE L3
On Wed, Nov 28, 2018 at 01:01:12AM +0000, Wei Yang wrote: >On Mon, Nov 26, 2018 at 11:17:40PM -0800, Dave Hansen wrote: >>On 11/26/18 10:25 PM, Michal Hocko wrote: >>> [Cc Dave who has added the lock into this path. Maybe he remembers why] >> >>I don't remember specifically. But, the pattern of: >> >> allocate >> lock >> set >> unlock >> >>is _usually_ so we don't have two "sets" racing with each other. In >>this case, that would have been to ensure that two >>sparse_init_one_section()'s didn't race and leak one of the two >>allocated memmaps or worse. >> >>I think mem_hotplug_lock protects this case these days, though. I don't >>think we had it in the early days and were just slumming it with the >>pgdat locks. >> >>I really don't like the idea of removing the lock by just saying it >>doesn't protect anything without doing some homework first, though. It >>would actually be really nice to comment the entire call chain from the >>mem_hotplug_lock acquisition to here. There is precious little >>commenting in there and it could use some love. > >Dave, > >Thanks for your comment :-) > >I should put more words to the reason for removing the lock. > >Here is a simplified call trace for sparse_add_one_section() during >physical add/remove phase. > > __add_memory() > add_memory_resource() > mem_hotplug_begin() > > arch_add_memory() > add_pages() > __add_pages() > __add_section() > sparse_add_one_section(pfn) > > mem_hotplug_done() > >When we just look at the sparse section initialization, we can see the >contention happens when __add_memory() try to add a same range or range >overlapped in SECTIONS_PER_ROOT number of sections. Otherwise, they >won't access the same memory. > >If this happens, we may face two contentions: > > * reallocation of mem_section[root] > * reallocation of memmap and usemap > >While neither of them could be protected by the pgdat_resize_lock from >my understanding. Grab pgdat_resize_lock just slow down the process, >while finally they will replace the mem_section[root] and >ms->section_mem_map with their own new allocated data. > Hmm... sorry, I am not correct here. The pgdat_resize_lock do protect the second case. But not the first one. >Last bu not the least, to be honest, even the global mem_hotplug_lock >doesn't help in this situation. In case __add_memory() try to add the >same range twice, the sparse section would be initialized twice. Which >means it will be overwritten with the new allocated memmap/usermap. > The mem_section[root] still has a chance to face the contention here. >But maybe we have the assumption this reentrance will not happen. > >This is all what I understand, in case there is some misunderstanding, >please let me know. I will rewrite the changelog to emphasize this process is protected by the global mem_hotplug_lock. > >-- >Wei Yang >Help you, Help me
On Wed, Nov 28, 2018 at 08:47:29AM +0000, Wei Yang wrote: >> >>Dave, >> >>Thanks for your comment :-) >> >>I should put more words to the reason for removing the lock. >> >>Here is a simplified call trace for sparse_add_one_section() during >>physical add/remove phase. >> >> __add_memory() >> add_memory_resource() >> mem_hotplug_begin() >> >> arch_add_memory() >> add_pages() >> __add_pages() >> __add_section() >> sparse_add_one_section(pfn) >> >> mem_hotplug_done() >> >>When we just look at the sparse section initialization, we can see the >>contention happens when __add_memory() try to add a same range or range >>overlapped in SECTIONS_PER_ROOT number of sections. Otherwise, they >>won't access the same memory. >> >>If this happens, we may face two contentions: >> >> * reallocation of mem_section[root] >> * reallocation of memmap and usemap >> >>While neither of them could be protected by the pgdat_resize_lock from >>my understanding. Grab pgdat_resize_lock just slow down the process, >>while finally they will replace the mem_section[root] and >>ms->section_mem_map with their own new allocated data. >> > >Hmm... sorry, I am not correct here. > >The pgdat_resize_lock do protect the second case. > >But not the first one. > One more thing, (hope I am not too talkative) Expand the pgdat_resize_lock to include sparse_index_init() may not work. Because SECTIONS_PER_ROOT number of section may span two nodes.
On Wed 28-11-18 08:47:29, Wei Yang wrote:
[...]
> The mem_section[root] still has a chance to face the contention here.
_If_ that is really the case then we need a dedicated lock rather than
rely on pgdat which doesn't even make sense for sparsemem internal.
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h index 45a5affcab8a..3787d4e913e6 100644 --- a/include/linux/memory_hotplug.h +++ b/include/linux/memory_hotplug.h @@ -333,7 +333,7 @@ extern void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn, unsigned long nr_pages, struct vmem_altmap *altmap); extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages); extern bool is_memblock_offlined(struct memory_block *mem); -extern int sparse_add_one_section(struct pglist_data *pgdat, +extern int sparse_add_one_section(int nid, unsigned long start_pfn, struct vmem_altmap *altmap); extern void sparse_remove_one_section(struct zone *zone, struct mem_section *ms, unsigned long map_offset, struct vmem_altmap *altmap); diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index f626e7e5f57b..5b3a3d7b4466 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -253,7 +253,7 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn, if (pfn_valid(phys_start_pfn)) return -EEXIST; - ret = sparse_add_one_section(NODE_DATA(nid), phys_start_pfn, altmap); + ret = sparse_add_one_section(nid, phys_start_pfn, altmap); if (ret < 0) return ret; diff --git a/mm/sparse.c b/mm/sparse.c index 33307fc05c4d..a4fdbcb21514 100644 --- a/mm/sparse.c +++ b/mm/sparse.c @@ -662,25 +662,24 @@ static void free_map_bootmem(struct page *memmap) * set. If this is <=0, then that means that the passed-in * map was not consumed and must be freed. */ -int __meminit sparse_add_one_section(struct pglist_data *pgdat, - unsigned long start_pfn, struct vmem_altmap *altmap) +int __meminit sparse_add_one_section(int nid, unsigned long start_pfn, + struct vmem_altmap *altmap) { unsigned long section_nr = pfn_to_section_nr(start_pfn); struct mem_section *ms; struct page *memmap; unsigned long *usemap; - unsigned long flags; int ret; /* * no locking for this, because it does its own * plus, it does a kmalloc */ - ret = sparse_index_init(section_nr, pgdat->node_id); + ret = sparse_index_init(section_nr, nid); if (ret < 0 && ret != -EEXIST) return ret; ret = 0; - memmap = kmalloc_section_memmap(section_nr, pgdat->node_id, altmap); + memmap = kmalloc_section_memmap(section_nr, nid, altmap); if (!memmap) return -ENOMEM; usemap = __kmalloc_section_usemap(); @@ -689,8 +688,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat, return -ENOMEM; } - pgdat_resize_lock(pgdat, &flags); - ms = __pfn_to_section(start_pfn); if (ms->section_mem_map & SECTION_MARKED_PRESENT) { ret = -EEXIST; @@ -707,7 +704,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat, sparse_init_one_section(ms, section_nr, memmap, usemap); out: - pgdat_resize_unlock(pgdat, &flags); if (ret < 0) { kfree(usemap); __kfree_section_memmap(memmap, altmap); @@ -769,10 +765,8 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms, unsigned long map_offset, struct vmem_altmap *altmap) { struct page *memmap = NULL; - unsigned long *usemap = NULL, flags; - struct pglist_data *pgdat = zone->zone_pgdat; + unsigned long *usemap = NULL; - pgdat_resize_lock(pgdat, &flags); if (ms->section_mem_map) { usemap = ms->pageblock_flags; memmap = sparse_decode_mem_map(ms->section_mem_map, @@ -780,7 +774,6 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms, ms->section_mem_map = 0; ms->pageblock_flags = NULL; } - pgdat_resize_unlock(pgdat, &flags); clear_hwpoisoned_pages(memmap + map_offset, PAGES_PER_SECTION - map_offset);
pgdat_resize_lock is used to protect pgdat's memory region information like: node_start_pfn, node_present_pages, etc. In function sparse_add/remove_one_section(), those data is not touched. This means it is not necessary to acquire pgdat_resize_lock to protect this area. Since the information needed in sparse_add_one_section() is node id to allocate proper memory. This patch also changes the prototype of sparse_add_one_section() to pass node id directly. This is intended to reduce misleading that sparse_add_one_section() would touch pgdat. Signed-off-by: Wei Yang <richard.weiyang@gmail.com> --- include/linux/memory_hotplug.h | 2 +- mm/memory_hotplug.c | 2 +- mm/sparse.c | 17 +++++------------ 3 files changed, 7 insertions(+), 14 deletions(-)