Re: [PATCH v3 1/2] mm/sparse: add sparse_init_nid()

From: Baoquan He
Date: Sun Jul 01 2018 - 22:11:31 EST


Hi Pavel,

Thanks for your quick fix. You might have missed another comment to v2
patch 1/2 which is at the bottom.

On 07/01/18 at 10:04pm, Pavel Tatashin wrote:
> +/*
> + * Initialize sparse on a specific node. The node spans [pnum_begin, pnum_end)
> + * And number of present sections in this node is map_count.
> + */
> +void __init sparse_init_nid(int nid, unsigned long pnum_begin,
> + unsigned long pnum_end,
> + unsigned long map_count)
> +{
> + unsigned long pnum, usemap_longs, *usemap, map_index;
> + struct page *map, *map_base;
> +
> + usemap_longs = BITS_TO_LONGS(SECTION_BLOCKFLAGS_BITS);
> + usemap = sparse_early_usemaps_alloc_pgdat_section(NODE_DATA(nid),
> + usemap_size() *
> + map_count);
> + if (!usemap) {
> + pr_err("%s: usemap allocation failed", __func__);
> + goto failed;
> + }
> + map_base = sparse_populate_node(pnum_begin, pnum_end,
> + map_count, nid);
> + map_index = 0;
> + for_each_present_section_nr(pnum_begin, pnum) {
> + if (pnum >= pnum_end)
> + break;
> +
> + BUG_ON(map_index == map_count);
> + map = sparse_populate_node_section(map_base, map_index,
> + pnum, nid);
> + if (!map) {

Here, I think it might be not right to jump to 'failed' directly if one
section of the node failed to populate memmap. I think the original code
is only skipping the section which memmap failed to populate by marking
it as not present with "ms->section_mem_map = 0".

> + pr_err("%s: memory map backing failed. Some memory will not be available.",
> + __func__);
> + pnum_begin = pnum;
> + goto failed;
> + }
> + check_usemap_section_nr(nid, usemap);
> + sparse_init_one_section(__nr_to_section(pnum), pnum, map,
> + usemap);
> + map_index++;
> + usemap += usemap_longs;
> + }
> + return;
> +failed:
> + /* We failed to allocate, mark all the following pnums as not present */
> + for_each_present_section_nr(pnum_begin, pnum) {
> + struct mem_section *ms;
> +
> + if (pnum >= pnum_end)
> + break;
> + ms = __nr_to_section(pnum);
> + ms->section_mem_map = 0;
> + }
> +}
> +
> /*
> * Allocate the accumulated non-linear sections, allocate a mem_map
> * for each and record the physical to section mapping.
> --
> 2.18.0
>