linux-kernel - Re: [PATCH 1/11] Add __GFP_MOVABLE flag and update callers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.64.0611242004520.3938@skynet.skynet.ie>
Date:	Fri, 24 Nov 2006 20:13:44 +0000 (GMT)
From:	Mel Gorman <mel@....ul.ie>
To:	Hugh Dickins <hugh@...itas.com>
Cc:	Linus Torvalds <torvalds@...l.org>,
	Christoph Lameter <clameter@....com>,
	Linux Memory Management List <linux-mm@...ck.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/11] Add __GFP_MOVABLE flag and update callers

On Fri, 24 Nov 2006, Hugh Dickins wrote:

> On Fri, 24 Nov 2006, Mel Gorman wrote:
>>
>> This is what the (compile-tested-only on x86) patch looks like for
>> GFP_HIGH_MOVABLE. The remaining in-tree GFP_HIGHUSER users are infiniband,
>> kvm, ncpfs, nfs, pipes (possible the most frequent user), m68knommu, hugepages
>> and kexec.
>>
>> Signed-off-by: Mel Gorman <mel@....ul.ie>
>
> You need to add in something like the patch below (mutatis mutandis
> for whichever approach you end up taking): tmpfs uses highmem pages
> for its swap vector blocks, noting where on swap the data pages are,
> and allocates them with mapping_gfp_mask(inode->i_mapping); but we
> don't have any mechanism in place for reclaiming or migrating those.
>

Good catch. In the page clustering patches I work on, I am doing this;

-       page = alloc_page_vma(gfp | __GFP_ZERO, &pvma, 0);
+       page = alloc_page_vma(
+                       set_migrateflags(gfp | __GFP_ZERO, __GFP_RECLAIMABLE),
+                                                               &pvma, 0);

to get rid of the MOVABLE flag and replace it with __GFP_RECLAIMABLE. This 
clustered the allocations together with allocations like inode cache. In 
retrospect, this was not a good idea because it assumes that tmpfs and 
shmem pages are short-lived. That may not be the case at all.

> (We could add that; but there might be better things to do instead.

Indeed. I believe that making page tables movable would gain more.

> I've often wanted to remove that whole layer from tmpfs, and note
> swap entries in the pagecache's radix-tree slots instead - but that
> does then lock them into low memory.  Hum, haw, never decided.)
>
> You can certainly be forgiven for missing that, and may well wonder
> why it doesn't just use GFP_HIGHUSER explicitly: because the loop
> driver may be on top of that tmpfs file, masking off __GFP_IO and
> __GFP_FS: the swap vector blocks should be allocated with the same
> restrictions as the data pages.
>

Thanks for that clarification. I suspected that something like this was 
the case when I removed the MOVABLE flag and used RECLAIMABLE but I wasn't 
100% certain. In the tests I was running, tmpfs pages weren't a major 
problem so I didn't chase it down.

> Excuse me for moving the __GFP_ZERO too: I think it's tidier to
> do them both within the little helper function.
>

Agreed.

> Hugh
>
> --- 2.6.19-rc5-mm2/mm/shmem.c	2006-11-14 09:58:21.000000000 +0000
> +++ linux/mm/shmem.c	2006-11-24 19:22:30.000000000 +0000
> @@ -94,7 +94,8 @@ static inline struct page *shmem_dir_all
> 	 * BLOCKS_PER_PAGE on indirect pages, assume PAGE_CACHE_SIZE:
> 	 * might be reconsidered if it ever diverges from PAGE_SIZE.
> 	 */
> -	return alloc_pages(gfp_mask, PAGE_CACHE_SHIFT-PAGE_SHIFT);
> +	return alloc_pages((gfp_mask & ~__GFP_MOVABLE) | __GFP_ZERO,
> +				PAGE_CACHE_SHIFT-PAGE_SHIFT);
> }
>
> static inline void shmem_dir_free(struct page *page)
> @@ -372,7 +373,7 @@ static swp_entry_t *shmem_swp_alloc(stru
> 		}
>
> 		spin_unlock(&info->lock);
> -		page = shmem_dir_alloc(mapping_gfp_mask(inode->i_mapping) | __GFP_ZERO);
> +		page = shmem_dir_alloc(mapping_gfp_mask(inode->i_mapping));
> 		if (page)
> 			set_page_private(page, 0);
> 		spin_lock(&info->lock);
>

I'll roll this into the movable patch, run a proper test and post a new 
patch Monday.

Thanks

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/