On Tue 24-11-20 14:49:24, Rik van Riel wrote:
> Matthew Wilcox pointed out that the i915 driver opportunistically
> allocates tmpfs memory, but will happily reclaim some of its
> pool if no memory is available.
>
> Make sure the gfp mask used to opportunistically allocate a THP
> is always at least as restrictive as the original gfp mask.
I have brought this up in the previous version review and I feel my
feedback hasn't been addressed. Please describe the expected behavior by
those shmem users including GFP_KERNEL restriction which would make the
THP flags incompatible. Is this a problem? Is there any actual problem
if the THP uses its own set of flags?
I am also not happy how those two sets of flags are completely detached
and we can only expect surprises there.
> Signed-off-by: Rik van Riel <riel@xxxxxxxxxxx>
> Suggested-by: Matthew Wilcox <willy@xxxxxxxxxxxxx>
> ---
> mm/shmem.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 6c3cb192a88d..ee3cea10c2a4 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1531,6 +1531,26 @@ static struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,
> return page;
> }
>
> +/*
> + * Make sure huge_gfp is always more limited than limit_gfp.
> + * Some of the flags set permissions, while others set limitations.
> + */
> +static gfp_t limit_gfp_mask(gfp_t huge_gfp, gfp_t limit_gfp)
> +{
> + gfp_t allowflags = __GFP_IO | __GFP_FS | __GFP_RECLAIM;
> + gfp_t denyflags = __GFP_NOWARN | __GFP_NORETRY;
> + gfp_t result = huge_gfp & ~allowflags;
> +
> + /*
> + * Minimize the result gfp by taking the union with the deny flags,
> + * and the intersection of the allow flags.
> + */
> + result |= (limit_gfp & denyflags);
> + result |= (huge_gfp & limit_gfp) & allowflags;
> +
> + return result;
> +}
> +
> static struct page *shmem_alloc_hugepage(gfp_t gfp,
> struct shmem_inode_info *info, pgoff_t index)
> {
> @@ -1889,6 +1909,7 @@ static int shmem_getpage_gfp(struct inode *inode, pgoff_t index,
>
> alloc_huge:
> huge_gfp = vma_thp_gfp_mask(vma);
> + huge_gfp = limit_gfp_mask(huge_gfp, gfp);
> page = shmem_alloc_and_acct_page(huge_gfp, inode, index, true);
> if (IS_ERR(page)) {
> alloc_nohuge:
> --
> 2.25.4
>
--
Michal Hocko
SUSE Labs
