| Message ID | 20201105191508.1961686-3-riel@surriel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm,thp,shm: limit shmem THP alloc gfp_mask | expand |
On Thu, 5 Nov 2020 14:15:08 -0500 > > 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. > > Signed-off-by: Rik van Riel <riel@surriel.com> > Suggested-by: Matthew Wilcox <willy@infradead.org> > --- > 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); Currently NORETRY is always set regardless of i915 and if it's determined in 1/2 then the i915 thing can be done like return huge_gfp | (limit_gfp & __GFP_RECLAIM); in assumption that 1/2 gives the baseline gfp shmem needs. > + 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
On Fri, 2020-11-06 at 11:05 +0800, Hillf Danton wrote: > On Thu, 5 Nov 2020 14:15:08 -0500 > > 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. > > > > Signed-off-by: Rik van Riel <riel@surriel.com> > > Suggested-by: Matthew Wilcox <willy@infradead.org> > > --- > > 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); > > Currently NORETRY is always set regardless of i915 and if it's > determined in 1/2 then the i915 thing can be done like > > return huge_gfp | (limit_gfp & __GFP_RECLAIM); No, if __GFP_KSWAPD_RECLAIM or __GFP_DIRECT_RECLAIM are not set in either huge_gfp or limit_gfp, we want to ensure the resulting gfp does not have it set, either. Your suggested change would result in __GFP_KSWAPD_RECLAIM or __GFP_DIRECT_RECLAIM getting set if it was set in either of the input gfp variables, which is probably not the desired behavior.
On Fri, 06 Nov 2020 12:53:33 -0500 Rik van Riel wrote: > On Fri, 2020-11-06 at 11:05 +0800, Hillf Danton wrote: > > On Thu, 5 Nov 2020 14:15:08 -0500 > > > 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. > > > > > > Signed-off-by: Rik van Riel <riel@surriel.com> > > > Suggested-by: Matthew Wilcox <willy@infradead.org> > > > --- > > > 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); > > > > Currently NORETRY is always set regardless of i915 and if it's > > determined in 1/2 then the i915 thing can be done like > > > > return huge_gfp | (limit_gfp & __GFP_RECLAIM); > > No, if __GFP_KSWAPD_RECLAIM or __GFP_DIRECT_RECLAIM are > not set in either huge_gfp or limit_gfp, we want to ensure > the resulting gfp does not have it set, either. That means huge_gfp can play game without i915 considered if __GFP_RECLAIM is determined in 1/2 too. Then things become simpler because we have no need to check limit_gfp from the begining. > > Your suggested change > would result in __GFP_KSWAPD_RECLAIM > or __GFP_DIRECT_RECLAIM getting set if it was set in either > of the input gfp variables, which is probably not the desired > behavior. It makes sense on if we could not determine __GFP_RECLAIM without i915 considered. Now it is safe to ignore it.
[Cc Chris for i915 and Andray] On Thu 05-11-20 14:15:08, 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. It would be good to explicitly mention the requested gfp flags for those allocations. i915 uses __GFP_NORETRY | __GFP_NOWARN, or GFP_KERNEL. Is __shmem_rw really meant to not allocate from highmeme/movable zones? Can it be ever backed by THPs? ttm might want __GFP_RETRY_MAYFAIL while shmem_read_mapping_page use the mapping gfp mask which can be NOFS or something else. This is quite messy already and I suspect that they are more targeting regular order-0 requests. E.g. have a look at cb5f1a52caf23. I am worried that this games with gfp flags will lead to unmaintainable code later on. There is a clear disconnect betwen the core THP allocation strategy and what drivers are asking for and those requirements might be really conflicting. Not to mention that flags might be different between regular and THP pages. > Make sure the gfp mask used to opportunistically allocate a THP > is always at least as restrictive as the original gfp mask. > > Signed-off-by: Rik van Riel <riel@surriel.com> > Suggested-by: Matthew Wilcox <willy@infradead.org> > --- > 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
On Thu, 2020-11-12 at 12:22 +0100, Michal Hocko wrote: > [Cc Chris for i915 and Andray] > > On Thu 05-11-20 14:15:08, 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. > > It would be good to explicitly mention the requested gfp flags for > those > allocations. i915 uses __GFP_NORETRY | __GFP_NOWARN, or GFP_KERNEL. > Is > __shmem_rw really meant to not allocate from highmeme/movable zones? > Can > it be ever backed by THPs? You are right, I need to copy the zone flags __GFP_DMA through __GFP_MOVABLE straight from the limiting gfp_mask into the gfp_mask used for THP allocations, and not use the default THP zone flags if the caller specifies something else. I'll send out a new version that fixes that. > ttm might want __GFP_RETRY_MAYFAIL while shmem_read_mapping_page use > the mapping gfp mask which can be NOFS or something else. This is > quite > messy already and I suspect that they are more targeting regular > order-0 > requests. E.g. have a look at cb5f1a52caf23. > > I am worried that this games with gfp flags will lead to > unmaintainable > code later on. There is a clear disconnect betwen the core THP > allocation strategy and what drivers are asking for and those > requirements might be really conflicting. Not to mention that flags > might be different between regular and THP pages. That is exactly why I want to make sure the THP allocations are never more aggressive than the gfp flags the drivers request, and the THP allocations may only ever be less aggressive than the order 0 gfp_mask specified by the drivers.
On Fri 13-11-20 22:40:40, Rik van Riel wrote: > On Thu, 2020-11-12 at 12:22 +0100, Michal Hocko wrote: > > [Cc Chris for i915 and Andray] > > > > On Thu 05-11-20 14:15:08, 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. > > > > It would be good to explicitly mention the requested gfp flags for > > those > > allocations. i915 uses __GFP_NORETRY | __GFP_NOWARN, or GFP_KERNEL. > > Is > > __shmem_rw really meant to not allocate from highmeme/movable zones? > > Can > > it be ever backed by THPs? > > You are right, I need to copy the zone flags __GFP_DMA > through > __GFP_MOVABLE straight from the limiting gfp_mask > into the gfp_mask used for THP allocations, and not use > the default THP zone flags if the caller specifies something > else. > > I'll send out a new version that fixes that. Can we make one step back here and actually check whether all this is actually needed for those shmem users before adding more hacks here and there?
On Thu, 2020-11-19 at 10:38 +0100, Michal Hocko wrote: > On Fri 13-11-20 22:40:40, Rik van Riel wrote: > > On Thu, 2020-11-12 at 12:22 +0100, Michal Hocko wrote: > > > [Cc Chris for i915 and Andray] > > > > > > On Thu 05-11-20 14:15:08, 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. > > > > > > It would be good to explicitly mention the requested gfp flags > > > for > > > those > > > allocations. i915 uses __GFP_NORETRY | __GFP_NOWARN, or > > > GFP_KERNEL. > > > Is > > > __shmem_rw really meant to not allocate from highmeme/movable > > > zones? > > > Can > > > it be ever backed by THPs? > > > > You are right, I need to copy the zone flags __GFP_DMA > > through > > __GFP_MOVABLE straight from the limiting gfp_mask > > into the gfp_mask used for THP allocations, and not use > > the default THP zone flags if the caller specifies something > > else. > > > > I'll send out a new version that fixes that. > > Can we make one step back here and actually check whether all this is > actually needed for those shmem users before adding more hacks here > and > there? It doesn't look like that is needed, after all. The i915 driver seems to support having its buffer in highmem, the shmem_pwrite and shmem_pread functions both do kmap/kunmap.
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:
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. Signed-off-by: Rik van Riel <riel@surriel.com> Suggested-by: Matthew Wilcox <willy@infradead.org> --- mm/shmem.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)