| Message ID | 20180521101555.25610-4-bhe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/21/2018 03:15 AM, Baoquan He wrote: > It's used to pass the size of map data unit into alloc_usemap_and_memmap, > and is preparation for next patch. This is the "what", but not the "why". Could you add another sentence or two to explain why we need this?
On 06/07/18 at 03:48pm, Dave Hansen wrote: > On 05/21/2018 03:15 AM, Baoquan He wrote: > > It's used to pass the size of map data unit into alloc_usemap_and_memmap, > > and is preparation for next patch. > > This is the "what", but not the "why". Could you add another sentence > or two to explain why we need this? Thanks for reviewing, Dave. In alloc_usemap_and_memmap(), it will call sparse_early_usemaps_alloc_node() or sparse_early_mem_maps_alloc_node() to allocate usemap and memmap for each node and install them into usemap_map[] and map_map[]. Here we need pass in the number of present sections on this node so that we can move pointer of usemap_map[] and map_map[] to right position. How do think about above words? Thanks Baoquan
On 06/07/2018 11:27 PM, Baoquan He wrote: > In alloc_usemap_and_memmap(), it will call > sparse_early_usemaps_alloc_node() or sparse_early_mem_maps_alloc_node() > to allocate usemap and memmap for each node and install them into > usemap_map[] and map_map[]. Here we need pass in the number of present > sections on this node so that we can move pointer of usemap_map[] and > map_map[] to right position. > > How do think about above words? But you're now passing in the size of the data structure. Why is that needed all of a sudden?
On 06/08/18 at 07:20am, Dave Hansen wrote: > On 06/07/2018 11:27 PM, Baoquan He wrote: > > In alloc_usemap_and_memmap(), it will call > > sparse_early_usemaps_alloc_node() or sparse_early_mem_maps_alloc_node() > > to allocate usemap and memmap for each node and install them into > > usemap_map[] and map_map[]. Here we need pass in the number of present > > sections on this node so that we can move pointer of usemap_map[] and > > map_map[] to right position. > > > > How do think about above words? > > But you're now passing in the size of the data structure. Why is that > needed all of a sudden? Oh, it's the size of the data structure. Because alloc_usemap_and_memmap() is reused for both usemap and memmap allocation, then inside alloc_usemap_and_memmap(), we need move forward the passed in pointer which points at the starting address of usemap_map or memmap_map, to a new position which points at the next starting address on new node. You can see we passed the usemap_map, map_map, and the array element size. void __init sparse_init(void) { ... alloc_usemap_and_memmap(sparse_early_usemaps_alloc_node, (void *)usemap_map, sizeof(usemap_map[0])); ... alloc_usemap_and_memmap(sparse_early_mem_maps_alloc_node, (void *)map_map, sizeof(map_map[0])); ... } Then inside alloc_usemap_and_memmap(), For each node, we get how many present sections on this node, call hook alloc_func(). Then we update the pointer to point at a new position of usemap_map[] or map_map[]. Here usemap_map[] is (unsigned long *), map_map[] is (struct page*). Even though both of them are pointer, I think it might be not good to assume that in alloc_usemap_and_memmap() because alloc_usemap_and_memmap() doesn't know what is stored in its 2nd parameter, namely (void * data). So add this new parameter to tell it. Combine with patch 4/4, it might be easier to understand. static void __init alloc_usemap_and_memmap(void (*alloc_func) .. { ... for_each_present_section_nr(pnum_begin + 1, pnum) { alloc_func(data, pnum_begin, pnum, map_count, nodeid_begin); ... data += map_count * data_unit_size; ... } ... }
On 06/08/2018 08:17 AM, Baoquan He wrote: > > Then inside alloc_usemap_and_memmap(), For each node, we get how many > present sections on this node, call hook alloc_func(). Then we update > the pointer to point at a new position of usemap_map[] or map_map[]. I think this is the key. alloc_usemap_and_memmap() is passed in a "void *" that it needs to update as things get consumed. But, it knows only the quantity of objects consumed and not the type. This effectively tells it enough about the type to let it update the pointer as objects are consumed. Right? Can we get that in the changelog?
On 06/08/18 at 09:13am, Dave Hansen wrote: > On 06/08/2018 08:17 AM, Baoquan He wrote: > > > > Then inside alloc_usemap_and_memmap(), For each node, we get how many > > present sections on this node, call hook alloc_func(). Then we update > > the pointer to point at a new position of usemap_map[] or map_map[]. > > I think this is the key. > > alloc_usemap_and_memmap() is passed in a "void *" that it needs to > update as things get consumed. But, it knows only the quantity of > objects consumed and not the type. This effectively tells it enough > about the type to let it update the pointer as objects are consumed. > > Right? > > Can we get that in the changelog? Hmm, I like above sentences very much, thanks. Do you means putting it in changelog, but not commit log of patch 3/4, right? I can do this when repost. Thanks Baoquan
diff --git a/mm/sparse.c b/mm/sparse.c index 3d697292be08..4a58f8809542 100644 --- a/mm/sparse.c +++ b/mm/sparse.c @@ -491,10 +491,12 @@ void __weak __meminit vmemmap_populate_print_last(void) /** * alloc_usemap_and_memmap - memory alloction for pageblock flags and vmemmap * @map: usemap_map for pageblock flags or mmap_map for vmemmap + * @unit_size: size of map unit */ static void __init alloc_usemap_and_memmap(void (*alloc_func) (void *, unsigned long, unsigned long, - unsigned long, int), void *data) + unsigned long, int), void *data, + int data_unit_size) { unsigned long pnum; unsigned long map_count; @@ -571,7 +573,8 @@ void __init sparse_init(void) if (!usemap_map) panic("can not allocate usemap_map\n"); alloc_usemap_and_memmap(sparse_early_usemaps_alloc_node, - (void *)usemap_map); + (void *)usemap_map, + sizeof(usemap_map[0])); #ifdef CONFIG_SPARSEMEM_ALLOC_MEM_MAP_TOGETHER size2 = sizeof(struct page *) * NR_MEM_SECTIONS; @@ -579,7 +582,8 @@ void __init sparse_init(void) if (!map_map) panic("can not allocate map_map\n"); alloc_usemap_and_memmap(sparse_early_mem_maps_alloc_node, - (void *)map_map); + (void *)map_map, + sizeof(map_map[0])); #endif for_each_present_section_nr(0, pnum) {
It's used to pass the size of map data unit into alloc_usemap_and_memmap, and is preparation for next patch. Signed-off-by: Baoquan He <bhe@redhat.com> --- mm/sparse.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)