This series add support for high order folios in shmem write
path.
This is a continuation of the shmem work from Luis here [1]
following Matthew Wilcox's suggestion [2] regarding the path to take
for the folio allocation order calculation.
[1] RFC v2 add support for blocksize > PAGE_SIZE
https://lore.kernel.org/all/[email protected]/T/#md3e93ab46ce2ad9254e1eb54ffe71211988b5632
[2] https://lore.kernel.org/all/[email protected]/
Patches have been tested and sent from next-230911. They do apply
cleanly to the latest next-230914.
fsx and fstests has been performed on tmpfs with noswap with the
following results:
- fsx: 2d test, 21,5B
- fstests: Same result as baseline for next-230911 [3][4][5]
[3] Baseline next-230911 failures are: generic/080 generic/126
generic/193 generic/633 generic/689
[4] fstests logs baseline: https://gitlab.com/-/snippets/3598621
[5] fstests logs patches: https://gitlab.com/-/snippets/3598628
There are at least 2 cases/topics to handle that I'd appreciate
feedback.
1. With the new strategy, you might end up with a folio order matching
HPAGE_PMD_ORDER. However, we won't respect the 'huge' flag anymore if
THP is enabled.
2. When the above (1.) occurs, the code skips the huge path, so
xa_find with hindex is skipped.
Daniel
Daniel Gomez (5):
filemap: make the folio order calculation shareable
shmem: drop BLOCKS_PER_PAGE macro
shmem: add order parameter support to shmem_alloc_folio
shmem: add file length in shmem_get_folio path
shmem: add large folios support to the write path
Luis Chamberlain (1):
shmem: account for large order folios
fs/iomap/buffered-io.c | 6 ++-
include/linux/pagemap.h | 42 ++++++++++++++++---
include/linux/shmem_fs.h | 2 +-
mm/filemap.c | 8 ----
mm/khugepaged.c | 2 +-
mm/shmem.c | 91 +++++++++++++++++++++++++---------------
6 files changed, 100 insertions(+), 51 deletions(-)
--
2.39.2
On 15.09.23 17:34, Matthew Wilcox wrote:
> On Fri, Sep 15, 2023 at 05:29:51PM +0200, David Hildenbrand wrote:
>> On 15.09.23 11:51, Daniel Gomez wrote:
>>> This series add support for high order folios in shmem write
>>> path.
>>> There are at least 2 cases/topics to handle that I'd appreciate
>>> feedback.
>>> 1. With the new strategy, you might end up with a folio order matching
>>> HPAGE_PMD_ORDER. However, we won't respect the 'huge' flag anymore if
>>> THP is enabled.
>>> 2. When the above (1.) occurs, the code skips the huge path, so
>>> xa_find with hindex is skipped.
>>
>> Similar to large anon folios (but different to large non-shmem folios in the
>> pagecache), this can result in memory waste.
>
> No, it can't. This patchset triggers only on write, not on read or page
> fault, and it's conservative, so it will only allocate folios which are
> entirely covered by the write. IOW this is memory we must allocate in
> order to satisfy the write; we're just allocating it in larger chunks
> when we can.
Oh, good! I was assuming you would eventually over-allocate on the write
path.
--
Cheers,
David / dhildenb
The commit [1] replaced all BLOCKS_PER_PAGE in favor of the
generic PAGE_SECTORS but definition was not removed. Drop it
as unused macro.
[1] e09764cff44b5 ("shmem: quota support").
Signed-off-by: Daniel Gomez <[email protected]>
Reviewed-by: Luis Chamberlain <[email protected]>
---
mm/shmem.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/mm/shmem.c b/mm/shmem.c
index 13c27c343820..8b3823e4d344 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -84,7 +84,6 @@ static struct vfsmount *shm_mnt;
#include "internal.h"
-#define BLOCKS_PER_PAGE (PAGE_SIZE/512)
#define VM_ACCT(size) (PAGE_ALIGN(size) >> PAGE_SHIFT)
/* Pretend that each entry is of this size in directory's i_size */
--
2.39.2
On Fri, Sep 15, 2023 at 05:29:51PM +0200, David Hildenbrand wrote:
> On 15.09.23 11:51, Daniel Gomez wrote:
> > This series add support for high order folios in shmem write
> > path.
> > There are at least 2 cases/topics to handle that I'd appreciate
> > feedback.
> > 1. With the new strategy, you might end up with a folio order matching
> > HPAGE_PMD_ORDER. However, we won't respect the 'huge' flag anymore if
> > THP is enabled.
> > 2. When the above (1.) occurs, the code skips the huge path, so
> > xa_find with hindex is skipped.
>
> Similar to large anon folios (but different to large non-shmem folios in the
> pagecache), this can result in memory waste.
No, it can't. This patchset triggers only on write, not on read or page
fault, and it's conservative, so it will only allocate folios which are
entirely covered by the write. IOW this is memory we must allocate in
order to satisfy the write; we're just allocating it in larger chunks
when we can.
On 15.09.23 17:40, Matthew Wilcox wrote:
> On Fri, Sep 15, 2023 at 05:36:27PM +0200, David Hildenbrand wrote:
>> On 15.09.23 17:34, Matthew Wilcox wrote:
>>> No, it can't. This patchset triggers only on write, not on read or page
>>> fault, and it's conservative, so it will only allocate folios which are
>>> entirely covered by the write. IOW this is memory we must allocate in
>>> order to satisfy the write; we're just allocating it in larger chunks
>>> when we can.
>>
>> Oh, good! I was assuming you would eventually over-allocate on the write
>> path.
>
> We might! But that would be a different patchset, and it would be
> subject to its own discussion.
>
> Something else I've been wondering about is possibly reallocating the
> pages on a write. This would apply to both normal files and shmem.
> If you read in a file one byte at a time, then overwrite a big chunk of
> it with a large single write, that seems like a good signal that maybe
> we should manage that part of the file as a single large chunk instead
> of individual pages. Maybe.
>
> Lots of things for people who are obsessed with performance to play
> with ;-)
:) Absolutely. ... because if nobody will be consuming that written
memory any time soon, it might also be the wrong place for a large/huge
folio.
--
Cheers,
David / dhildenb
On 15.09.23 11:51, Daniel Gomez wrote:
> This series add support for high order folios in shmem write
> path.
>
> This is a continuation of the shmem work from Luis here [1]
> following Matthew Wilcox's suggestion [2] regarding the path to take
> for the folio allocation order calculation.
>
> [1] RFC v2 add support for blocksize > PAGE_SIZE
> https://lore.kernel.org/all/[email protected]/T/#md3e93ab46ce2ad9254e1eb54ffe71211988b5632
> [2] https://lore.kernel.org/all/[email protected]/
>
> Patches have been tested and sent from next-230911. They do apply
> cleanly to the latest next-230914.
>
> fsx and fstests has been performed on tmpfs with noswap with the
> following results:
> - fsx: 2d test, 21,5B
> - fstests: Same result as baseline for next-230911 [3][4][5]
>
> [3] Baseline next-230911 failures are: generic/080 generic/126
> generic/193 generic/633 generic/689
> [4] fstests logs baseline: https://gitlab.com/-/snippets/3598621
> [5] fstests logs patches: https://gitlab.com/-/snippets/3598628
>
> There are at least 2 cases/topics to handle that I'd appreciate
> feedback.
> 1. With the new strategy, you might end up with a folio order matching
> HPAGE_PMD_ORDER. However, we won't respect the 'huge' flag anymore if
> THP is enabled.
> 2. When the above (1.) occurs, the code skips the huge path, so
> xa_find with hindex is skipped.
Similar to large anon folios (but different to large non-shmem folios in
the pagecache), this can result in memory waste.
We discussed that topic in the last bi-weekly mm meeting, and also how
to eventually configure that for shmem.
Refer to of a summary. [1]
[1] https://lkml.kernel.org/r/[email protected]
--
Cheers,
David / dhildenb
Add large folio support for shmem write path matching the same high
order preference mechanism used for iomap buffered IO path as used in
__filemap_get_folio().
Use the __folio_get_max_order to get a hint for the order of the folio
based on file size which takes care of the mapping requirements.
Swap does not support high order folios for now, so make it order 0 in
case swap is enabled.
Signed-off-by: Daniel Gomez <[email protected]>
---
mm/shmem.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/mm/shmem.c b/mm/shmem.c
index adff74751065..26ca555b1669 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1683,13 +1683,19 @@ static struct folio *shmem_alloc_folio(gfp_t gfp,
}
static struct folio *shmem_alloc_and_acct_folio(gfp_t gfp, struct inode *inode,
- pgoff_t index, bool huge, unsigned int *order)
+ pgoff_t index, bool huge, unsigned int *order,
+ struct shmem_sb_info *sbinfo)
{
struct shmem_inode_info *info = SHMEM_I(inode);
struct folio *folio;
int nr;
int err;
+ if (!sbinfo->noswap)
+ *order = 0;
+ else
+ *order = (*order == 1) ? 0 : *order;
+
if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE))
huge = false;
nr = huge ? HPAGE_PMD_NR : 1U << *order;
@@ -2032,6 +2038,8 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
return 0;
}
+ order = mapping_size_order(inode->i_mapping, index, len);
+
if (!shmem_is_huge(inode, index, false,
vma ? vma->vm_mm : NULL, vma ? vma->vm_flags : 0))
goto alloc_nohuge;
@@ -2039,11 +2047,11 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
huge_gfp = vma_thp_gfp_mask(vma);
huge_gfp = limit_gfp_mask(huge_gfp, gfp);
folio = shmem_alloc_and_acct_folio(huge_gfp, inode, index, true,
- &order);
+ &order, sbinfo);
if (IS_ERR(folio)) {
alloc_nohuge:
folio = shmem_alloc_and_acct_folio(gfp, inode, index, false,
- &order);
+ &order, sbinfo);
}
if (IS_ERR(folio)) {
int retry = 5;
@@ -2147,6 +2155,8 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
if (folio_test_large(folio)) {
folio_unlock(folio);
folio_put(folio);
+ if (order > 0)
+ order--;
goto alloc_nohuge;
}
unlock:
--
2.39.2
On Fri, Sep 15, 2023 at 05:36:27PM +0200, David Hildenbrand wrote:
> On 15.09.23 17:34, Matthew Wilcox wrote:
> > No, it can't. This patchset triggers only on write, not on read or page
> > fault, and it's conservative, so it will only allocate folios which are
> > entirely covered by the write. IOW this is memory we must allocate in
> > order to satisfy the write; we're just allocating it in larger chunks
> > when we can.
>
> Oh, good! I was assuming you would eventually over-allocate on the write
> path.
We might! But that would be a different patchset, and it would be
subject to its own discussion.
Something else I've been wondering about is possibly reallocating the
pages on a write. This would apply to both normal files and shmem.
If you read in a file one byte at a time, then overwrite a big chunk of
it with a large single write, that seems like a good signal that maybe
we should manage that part of the file as a single large chunk instead
of individual pages. Maybe.
Lots of things for people who are obsessed with performance to play
with ;-)
On Fri, Sep 15, 2023 at 2:51 AM Daniel Gomez <[email protected]> wrote:
>
> Add large folio support for shmem write path matching the same high
> order preference mechanism used for iomap buffered IO path as used in
> __filemap_get_folio().
>
> Use the __folio_get_max_order to get a hint for the order of the folio
> based on file size which takes care of the mapping requirements.
>
> Swap does not support high order folios for now, so make it order 0 in
> case swap is enabled.
I didn't take a close look at the series, but I am not sure I
understand the rationale here. Reclaim will split high order shmem
folios anyway, right?
It seems like we only enable high order folios if the "noswap" mount
option is used, which is fairly recent. I doubt it is widely used.
>
> Signed-off-by: Daniel Gomez <[email protected]>
> ---
> mm/shmem.c | 16 +++++++++++++---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index adff74751065..26ca555b1669 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1683,13 +1683,19 @@ static struct folio *shmem_alloc_folio(gfp_t gfp,
> }
>
> static struct folio *shmem_alloc_and_acct_folio(gfp_t gfp, struct inode *inode,
> - pgoff_t index, bool huge, unsigned int *order)
> + pgoff_t index, bool huge, unsigned int *order,
> + struct shmem_sb_info *sbinfo)
> {
> struct shmem_inode_info *info = SHMEM_I(inode);
> struct folio *folio;
> int nr;
> int err;
>
> + if (!sbinfo->noswap)
> + *order = 0;
> + else
> + *order = (*order == 1) ? 0 : *order;
> +
> if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE))
> huge = false;
> nr = huge ? HPAGE_PMD_NR : 1U << *order;
> @@ -2032,6 +2038,8 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
> return 0;
> }
>
> + order = mapping_size_order(inode->i_mapping, index, len);
> +
> if (!shmem_is_huge(inode, index, false,
> vma ? vma->vm_mm : NULL, vma ? vma->vm_flags : 0))
> goto alloc_nohuge;
> @@ -2039,11 +2047,11 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
> huge_gfp = vma_thp_gfp_mask(vma);
> huge_gfp = limit_gfp_mask(huge_gfp, gfp);
> folio = shmem_alloc_and_acct_folio(huge_gfp, inode, index, true,
> - &order);
> + &order, sbinfo);
> if (IS_ERR(folio)) {
> alloc_nohuge:
> folio = shmem_alloc_and_acct_folio(gfp, inode, index, false,
> - &order);
> + &order, sbinfo);
> }
> if (IS_ERR(folio)) {
> int retry = 5;
> @@ -2147,6 +2155,8 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
> if (folio_test_large(folio)) {
> folio_unlock(folio);
> folio_put(folio);
> + if (order > 0)
> + order--;
> goto alloc_nohuge;
> }
> unlock:
> --
> 2.39.2
>
On Fri, Sep 15, 2023 at 05:29:51PM +0200, David Hildenbrand wrote:
> On 15.09.23 11:51, Daniel Gomez wrote:
> > This series add support for high order folios in shmem write
> > path.
> >
> > This is a continuation of the shmem work from Luis here [1]
> > following Matthew Wilcox's suggestion [2] regarding the path to take
> > for the folio allocation order calculation.
> >
> > [1] RFC v2 add support for blocksize > PAGE_SIZE
> > https://lore.kernel.org/all/[email protected]/T/#md3e93ab46ce2ad9254e1eb54ffe71211988b5632
> > [2] https://lore.kernel.org/all/[email protected]/
> >
> > Patches have been tested and sent from next-230911. They do apply
> > cleanly to the latest next-230914.
> >
> > fsx and fstests has been performed on tmpfs with noswap with the
> > following results:
> > - fsx: 2d test, 21,5B
> > - fstests: Same result as baseline for next-230911 [3][4][5]
> >
> > [3] Baseline next-230911 failures are: generic/080 generic/126
> > generic/193 generic/633 generic/689
> > [4] fstests logs baseline: https://gitlab.com/-/snippets/3598621
> > [5] fstests logs patches: https://gitlab.com/-/snippets/3598628
> >
> > There are at least 2 cases/topics to handle that I'd appreciate
> > feedback.
> > 1. With the new strategy, you might end up with a folio order matching
> > HPAGE_PMD_ORDER. However, we won't respect the 'huge' flag anymore if
> > THP is enabled.
> > 2. When the above (1.) occurs, the code skips the huge path, so
> > xa_find with hindex is skipped.
>
> Similar to large anon folios (but different to large non-shmem folios in the
> pagecache), this can result in memory waste.
>
> We discussed that topic in the last bi-weekly mm meeting, and also how to
> eventually configure that for shmem.
>
> Refer to of a summary. [1]
>
> [1] https://lkml.kernel.org/r/[email protected]
Thanks for the summary David (I was missing linux-MM from kvack in lei).
I think the PMD_ORDER-1 as max would suffice here to honor/respect the
huge flag. Although, we would end up having a different max value
than pagecache/readahead.
>
> --
> Cheers,
>
> David / dhildenb
>
On Fri, Sep 15, 2023 at 11:26:37AM -0700, Yosry Ahmed wrote:
> On Fri, Sep 15, 2023 at 2:51 AM Daniel Gomez <[email protected]> wrote:
> >
> > Add large folio support for shmem write path matching the same high
> > order preference mechanism used for iomap buffered IO path as used in
> > __filemap_get_folio().
> >
> > Use the __folio_get_max_order to get a hint for the order of the folio
> > based on file size which takes care of the mapping requirements.
> >
> > Swap does not support high order folios for now, so make it order 0 in
> > case swap is enabled.
>
> I didn't take a close look at the series, but I am not sure I
> understand the rationale here. Reclaim will split high order shmem
> folios anyway, right?
For context, this is part of the enablement of large block sizes (LBS)
effort [1][2][3], so the assumption here is that the kernel will
reclaim memory with the same (large) block sizes that were written to
the device.
I'll add more context in the V2.
[1] https://kernelnewbies.org/KernelProjects/large-block-size
[2] https://docs.google.com/spreadsheets/d/e/2PACX-1vS7sQfw90S00l2rfOKm83Jlg0px8KxMQE4HHp_DKRGbAGcAV-xu6LITHBEc4xzVh9wLH6WM2lR0cZS8/pubhtml#
[3] https://lore.kernel.org/all/[email protected]/
>
> It seems like we only enable high order folios if the "noswap" mount
> option is used, which is fairly recent. I doubt it is widely used.
For now, I skipped the swap path as it currently lacks support for
high order folios. But I'm currently looking into it as part of the LBS
effort (please check spreadsheet at [2] for that).
>
> >
> > Signed-off-by: Daniel Gomez <[email protected]>
> > ---
> > mm/shmem.c | 16 +++++++++++++---
> > 1 file changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index adff74751065..26ca555b1669 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -1683,13 +1683,19 @@ static struct folio *shmem_alloc_folio(gfp_t gfp,
> > }
> >
> > static struct folio *shmem_alloc_and_acct_folio(gfp_t gfp, struct inode *inode,
> > - pgoff_t index, bool huge, unsigned int *order)
> > + pgoff_t index, bool huge, unsigned int *order,
> > + struct shmem_sb_info *sbinfo)
> > {
> > struct shmem_inode_info *info = SHMEM_I(inode);
> > struct folio *folio;
> > int nr;
> > int err;
> >
> > + if (!sbinfo->noswap)
> > + *order = 0;
> > + else
> > + *order = (*order == 1) ? 0 : *order;
> > +
> > if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE))
> > huge = false;
> > nr = huge ? HPAGE_PMD_NR : 1U << *order;
> > @@ -2032,6 +2038,8 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
> > return 0;
> > }
> >
> > + order = mapping_size_order(inode->i_mapping, index, len);
> > +
> > if (!shmem_is_huge(inode, index, false,
> > vma ? vma->vm_mm : NULL, vma ? vma->vm_flags : 0))
> > goto alloc_nohuge;
> > @@ -2039,11 +2047,11 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
> > huge_gfp = vma_thp_gfp_mask(vma);
> > huge_gfp = limit_gfp_mask(huge_gfp, gfp);
> > folio = shmem_alloc_and_acct_folio(huge_gfp, inode, index, true,
> > - &order);
> > + &order, sbinfo);
> > if (IS_ERR(folio)) {
> > alloc_nohuge:
> > folio = shmem_alloc_and_acct_folio(gfp, inode, index, false,
> > - &order);
> > + &order, sbinfo);
> > }
> > if (IS_ERR(folio)) {
> > int retry = 5;
> > @@ -2147,6 +2155,8 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
> > if (folio_test_large(folio)) {
> > folio_unlock(folio);
> > folio_put(folio);
> > + if (order > 0)
> > + order--;
> > goto alloc_nohuge;
> > }
> > unlock:
> > --
> > 2.39.2
> >
On Mon, Sep 18, 2023 at 1:00 AM Daniel Gomez <[email protected]> wrote:
>
> On Fri, Sep 15, 2023 at 11:26:37AM -0700, Yosry Ahmed wrote:
> > On Fri, Sep 15, 2023 at 2:51 AM Daniel Gomez <[email protected]> wrote:
> > >
> > > Add large folio support for shmem write path matching the same high
> > > order preference mechanism used for iomap buffered IO path as used in
> > > __filemap_get_folio().
> > >
> > > Use the __folio_get_max_order to get a hint for the order of the folio
> > > based on file size which takes care of the mapping requirements.
> > >
> > > Swap does not support high order folios for now, so make it order 0 in
> > > case swap is enabled.
> >
> > I didn't take a close look at the series, but I am not sure I
> > understand the rationale here. Reclaim will split high order shmem
> > folios anyway, right?
>
> For context, this is part of the enablement of large block sizes (LBS)
> effort [1][2][3], so the assumption here is that the kernel will
> reclaim memory with the same (large) block sizes that were written to
> the device.
>
> I'll add more context in the V2.
>
> [1] https://kernelnewbies.org/KernelProjects/large-block-size
> [2] https://docs.google.com/spreadsheets/d/e/2PACX-1vS7sQfw90S00l2rfOKm83Jlg0px8KxMQE4HHp_DKRGbAGcAV-xu6LITHBEc4xzVh9wLH6WM2lR0cZS8/pubhtml#
> [3] https://lore.kernel.org/all/[email protected]/
> >
> > It seems like we only enable high order folios if the "noswap" mount
> > option is used, which is fairly recent. I doubt it is widely used.
>
> For now, I skipped the swap path as it currently lacks support for
> high order folios. But I'm currently looking into it as part of the LBS
> effort (please check spreadsheet at [2] for that).
Thanks for the context, but I am not sure I understand.
IIUC we are skipping allocating large folios in shmem if swap is
enabled in this patch. Swap does not support swapping out large folios
as a whole (except THPs), but page reclaim will split those large
folios and swap them out as order-0 pages anyway. So I am not sure I
understand why we need to skip allocating large folios if swap is
enabled.
> >
> > >
> > > Signed-off-by: Daniel Gomez <[email protected]>
> > > ---
> > > mm/shmem.c | 16 +++++++++++++---
> > > 1 file changed, 13 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/mm/shmem.c b/mm/shmem.c
> > > index adff74751065..26ca555b1669 100644
> > > --- a/mm/shmem.c
> > > +++ b/mm/shmem.c
> > > @@ -1683,13 +1683,19 @@ static struct folio *shmem_alloc_folio(gfp_t gfp,
> > > }
> > >
> > > static struct folio *shmem_alloc_and_acct_folio(gfp_t gfp, struct inode *inode,
> > > - pgoff_t index, bool huge, unsigned int *order)
> > > + pgoff_t index, bool huge, unsigned int *order,
> > > + struct shmem_sb_info *sbinfo)
> > > {
> > > struct shmem_inode_info *info = SHMEM_I(inode);
> > > struct folio *folio;
> > > int nr;
> > > int err;
> > >
> > > + if (!sbinfo->noswap)
> > > + *order = 0;
> > > + else
> > > + *order = (*order == 1) ? 0 : *order;
> > > +
> > > if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE))
> > > huge = false;
> > > nr = huge ? HPAGE_PMD_NR : 1U << *order;
> > > @@ -2032,6 +2038,8 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
> > > return 0;
> > > }
> > >
> > > + order = mapping_size_order(inode->i_mapping, index, len);
> > > +
> > > if (!shmem_is_huge(inode, index, false,
> > > vma ? vma->vm_mm : NULL, vma ? vma->vm_flags : 0))
> > > goto alloc_nohuge;
> > > @@ -2039,11 +2047,11 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
> > > huge_gfp = vma_thp_gfp_mask(vma);
> > > huge_gfp = limit_gfp_mask(huge_gfp, gfp);
> > > folio = shmem_alloc_and_acct_folio(huge_gfp, inode, index, true,
> > > - &order);
> > > + &order, sbinfo);
> > > if (IS_ERR(folio)) {
> > > alloc_nohuge:
> > > folio = shmem_alloc_and_acct_folio(gfp, inode, index, false,
> > > - &order);
> > > + &order, sbinfo);
> > > }
> > > if (IS_ERR(folio)) {
> > > int retry = 5;
> > > @@ -2147,6 +2155,8 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
> > > if (folio_test_large(folio)) {
> > > folio_unlock(folio);
> > > folio_put(folio);
> > > + if (order > 0)
> > > + order--;
> > > goto alloc_nohuge;
> > > }
> > > unlock:
> > > --
> > > 2.39.2
> > >
On Mon, Sep 18, 2023 at 11:55:34AM -0700, Yosry Ahmed wrote:
> On Mon, Sep 18, 2023 at 1:00 AM Daniel Gomez <[email protected]> wrote:
> >
> > On Fri, Sep 15, 2023 at 11:26:37AM -0700, Yosry Ahmed wrote:
> > > On Fri, Sep 15, 2023 at 2:51 AM Daniel Gomez <[email protected]> wrote:
> > > >
> > > > Add large folio support for shmem write path matching the same high
> > > > order preference mechanism used for iomap buffered IO path as used in
> > > > __filemap_get_folio().
> > > >
> > > > Use the __folio_get_max_order to get a hint for the order of the folio
> > > > based on file size which takes care of the mapping requirements.
> > > >
> > > > Swap does not support high order folios for now, so make it order 0 in
> > > > case swap is enabled.
> > >
> > > I didn't take a close look at the series, but I am not sure I
> > > understand the rationale here. Reclaim will split high order shmem
> > > folios anyway, right?
> >
> > For context, this is part of the enablement of large block sizes (LBS)
> > effort [1][2][3], so the assumption here is that the kernel will
> > reclaim memory with the same (large) block sizes that were written to
> > the device.
> >
> > I'll add more context in the V2.
> >
> > [1] https://protect2.fireeye.com/v1/url?k=a80aab33-c981be05-a80b207c-000babff9b5d-b656d8860b04562f&q=1&e=46666acf-d70d-4e8d-8d00-b027808ae400&u=https%3A%2F%2Fkernelnewbies.org%2FKernelProjects%2Flarge-block-size
> > [2] https://protect2.fireeye.com/v1/url?k=3f753ca2-5efe2994-3f74b7ed-000babff9b5d-e678f885471555e3&q=1&e=46666acf-d70d-4e8d-8d00-b027808ae400&u=https%3A%2F%2Fdocs.google.com%2Fspreadsheets%2Fd%2Fe%2F2PACX-1vS7sQfw90S00l2rfOKm83Jlg0px8KxMQE4HHp_DKRGbAGcAV-xu6LITHBEc4xzVh9wLH6WM2lR0cZS8%2Fpubhtml%23
> > [3] https://lore.kernel.org/all/[email protected]/
> > >
> > > It seems like we only enable high order folios if the "noswap" mount
> > > option is used, which is fairly recent. I doubt it is widely used.
> >
> > For now, I skipped the swap path as it currently lacks support for
> > high order folios. But I'm currently looking into it as part of the LBS
> > effort (please check spreadsheet at [2] for that).
>
> Thanks for the context, but I am not sure I understand.
>
> IIUC we are skipping allocating large folios in shmem if swap is
> enabled in this patch. Swap does not support swapping out large folios
> as a whole (except THPs), but page reclaim will split those large
> folios and swap them out as order-0 pages anyway. So I am not sure I
> understand why we need to skip allocating large folios if swap is
> enabled.
I lifted noswap condition and retested it again on top of 230918 and
there is some regression. So, based on the results I guess the initial
requirement may be the way to go. But what do you think?
Here the logs:
* shmem-large-folios-swap: https://gitlab.com/-/snippets/3600360
* shmem-baseline-swap : https://gitlab.com/-/snippets/3600362
-Failures: generic/080 generic/126 generic/193 generic/633 generic/689
-Failed 5 of 730 tests
\ No newline at end of file
+Failures: generic/080 generic/103 generic/126 generic/193 generic/285 generic/436 generic/619 generic/633 generic/689
+Failed 9 of 730 tests
\ No newline at end of file
>
>
> > >
> > > >
> > > > Signed-off-by: Daniel Gomez <[email protected]>
> > > > ---
> > > > mm/shmem.c | 16 +++++++++++++---
> > > > 1 file changed, 13 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/mm/shmem.c b/mm/shmem.c
> > > > index adff74751065..26ca555b1669 100644
> > > > --- a/mm/shmem.c
> > > > +++ b/mm/shmem.c
> > > > @@ -1683,13 +1683,19 @@ static struct folio *shmem_alloc_folio(gfp_t gfp,
> > > > }
> > > >
> > > > static struct folio *shmem_alloc_and_acct_folio(gfp_t gfp, struct inode *inode,
> > > > - pgoff_t index, bool huge, unsigned int *order)
> > > > + pgoff_t index, bool huge, unsigned int *order,
> > > > + struct shmem_sb_info *sbinfo)
> > > > {
> > > > struct shmem_inode_info *info = SHMEM_I(inode);
> > > > struct folio *folio;
> > > > int nr;
> > > > int err;
> > > >
> > > > + if (!sbinfo->noswap)
> > > > + *order = 0;
> > > > + else
> > > > + *order = (*order == 1) ? 0 : *order;
> > > > +
> > > > if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE))
> > > > huge = false;
> > > > nr = huge ? HPAGE_PMD_NR : 1U << *order;
> > > > @@ -2032,6 +2038,8 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
> > > > return 0;
> > > > }
> > > >
> > > > + order = mapping_size_order(inode->i_mapping, index, len);
> > > > +
> > > > if (!shmem_is_huge(inode, index, false,
> > > > vma ? vma->vm_mm : NULL, vma ? vma->vm_flags : 0))
> > > > goto alloc_nohuge;
> > > > @@ -2039,11 +2047,11 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
> > > > huge_gfp = vma_thp_gfp_mask(vma);
> > > > huge_gfp = limit_gfp_mask(huge_gfp, gfp);
> > > > folio = shmem_alloc_and_acct_folio(huge_gfp, inode, index, true,
> > > > - &order);
> > > > + &order, sbinfo);
> > > > if (IS_ERR(folio)) {
> > > > alloc_nohuge:
> > > > folio = shmem_alloc_and_acct_folio(gfp, inode, index, false,
> > > > - &order);
> > > > + &order, sbinfo);
> > > > }
> > > > if (IS_ERR(folio)) {
> > > > int retry = 5;
> > > > @@ -2147,6 +2155,8 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
> > > > if (folio_test_large(folio)) {
> > > > folio_unlock(folio);
> > > > folio_put(folio);
> > > > + if (order > 0)
> > > > + order--;
> > > > goto alloc_nohuge;
> > > > }
> > > > unlock:
> > > > --
> > > > 2.39.2
> > > >
On Tue, Sep 19, 2023 at 6:27 AM Daniel Gomez <[email protected]> wrote:
>
> On Mon, Sep 18, 2023 at 11:55:34AM -0700, Yosry Ahmed wrote:
> > On Mon, Sep 18, 2023 at 1:00 AM Daniel Gomez <[email protected]> wrote:
> > >
> > > On Fri, Sep 15, 2023 at 11:26:37AM -0700, Yosry Ahmed wrote:
> > > > On Fri, Sep 15, 2023 at 2:51 AM Daniel Gomez <[email protected]> wrote:
> > > > >
> > > > > Add large folio support for shmem write path matching the same high
> > > > > order preference mechanism used for iomap buffered IO path as used in
> > > > > __filemap_get_folio().
> > > > >
> > > > > Use the __folio_get_max_order to get a hint for the order of the folio
> > > > > based on file size which takes care of the mapping requirements.
> > > > >
> > > > > Swap does not support high order folios for now, so make it order 0 in
> > > > > case swap is enabled.
> > > >
> > > > I didn't take a close look at the series, but I am not sure I
> > > > understand the rationale here. Reclaim will split high order shmem
> > > > folios anyway, right?
> > >
> > > For context, this is part of the enablement of large block sizes (LBS)
> > > effort [1][2][3], so the assumption here is that the kernel will
> > > reclaim memory with the same (large) block sizes that were written to
> > > the device.
> > >
> > > I'll add more context in the V2.
> > >
> > > [1] https://protect2.fireeye.com/v1/url?k=a80aab33-c981be05-a80b207c-000babff9b5d-b656d8860b04562f&q=1&e=46666acf-d70d-4e8d-8d00-b027808ae400&u=https%3A%2F%2Fkernelnewbies.org%2FKernelProjects%2Flarge-block-size
> > > [2] https://protect2.fireeye.com/v1/url?k=3f753ca2-5efe2994-3f74b7ed-000babff9b5d-e678f885471555e3&q=1&e=46666acf-d70d-4e8d-8d00-b027808ae400&u=https%3A%2F%2Fdocs.google.com%2Fspreadsheets%2Fd%2Fe%2F2PACX-1vS7sQfw90S00l2rfOKm83Jlg0px8KxMQE4HHp_DKRGbAGcAV-xu6LITHBEc4xzVh9wLH6WM2lR0cZS8%2Fpubhtml%23
> > > [3] https://lore.kernel.org/all/[email protected]/
> > > >
> > > > It seems like we only enable high order folios if the "noswap" mount
> > > > option is used, which is fairly recent. I doubt it is widely used.
> > >
> > > For now, I skipped the swap path as it currently lacks support for
> > > high order folios. But I'm currently looking into it as part of the LBS
> > > effort (please check spreadsheet at [2] for that).
> >
> > Thanks for the context, but I am not sure I understand.
> >
> > IIUC we are skipping allocating large folios in shmem if swap is
> > enabled in this patch. Swap does not support swapping out large folios
> > as a whole (except THPs), but page reclaim will split those large
> > folios and swap them out as order-0 pages anyway. So I am not sure I
> > understand why we need to skip allocating large folios if swap is
> > enabled.
>
> I lifted noswap condition and retested it again on top of 230918 and
> there is some regression. So, based on the results I guess the initial
> requirement may be the way to go. But what do you think?
>
> Here the logs:
> * shmem-large-folios-swap: https://gitlab.com/-/snippets/3600360
> * shmem-baseline-swap : https://gitlab.com/-/snippets/3600362
>
> -Failures: generic/080 generic/126 generic/193 generic/633 generic/689
> -Failed 5 of 730 tests
> \ No newline at end of file
> +Failures: generic/080 generic/103 generic/126 generic/193 generic/285 generic/436 generic/619 generic/633 generic/689
> +Failed 9 of 730 tests
> \ No newline at end of file
> >
I am not really familiar with these tests so I cannot really tell
what's going on. I can see "swapfiles are not supported" in the logs
though, so it seems like we are seeing extra failures by just lifting
"noswap" even without actually swapping. I am curious if this is just
hiding a different issue, I would at least try to understand what's
happening.
Anyway, I don't have enough context here to be useful. I was just
making an observation about reclaim splitting shmem folios to swap
them out as order-0 pages, and asking why this is needed based on
that. I will leave it up to you and the reviewers to decide if there's
anything interesting here.
On Tue, Sep 19, 2023 at 2:47 PM Luis Chamberlain <[email protected]> wrote:
>
> On Tue, Sep 19, 2023 at 09:00:16AM -0700, Yosry Ahmed wrote:
> > On Tue, Sep 19, 2023 at 6:27 AM Daniel Gomez <[email protected]> wrote:
> > >
> > > On Mon, Sep 18, 2023 at 11:55:34AM -0700, Yosry Ahmed wrote:
> > > > On Mon, Sep 18, 2023 at 1:00 AM Daniel Gomez <[email protected]> wrote:
> > > > >
> > > > > On Fri, Sep 15, 2023 at 11:26:37AM -0700, Yosry Ahmed wrote:
> > > > > > On Fri, Sep 15, 2023 at 2:51 AM Daniel Gomez <[email protected]> wrote:
> > > > > > >
> > > > > > > Add large folio support for shmem write path matching the same high
> > > > > > > order preference mechanism used for iomap buffered IO path as used in
> > > > > > > __filemap_get_folio().
> > > > > > >
> > > > > > > Use the __folio_get_max_order to get a hint for the order of the folio
> > > > > > > based on file size which takes care of the mapping requirements.
> > > > > > >
> > > > > > > Swap does not support high order folios for now, so make it order 0 in
> > > > > > > case swap is enabled.
> > > > > >
> > > > > > I didn't take a close look at the series, but I am not sure I
> > > > > > understand the rationale here. Reclaim will split high order shmem
> > > > > > folios anyway, right?
> > > > >
> > > > > For context, this is part of the enablement of large block sizes (LBS)
> > > > > effort [1][2][3], so the assumption here is that the kernel will
> > > > > reclaim memory with the same (large) block sizes that were written to
> > > > > the device.
> > > > >
> > > > > I'll add more context in the V2.
> > > > >
> > > > > [1] https://protect2.fireeye.com/v1/url?k=a80aab33-c981be05-a80b207c-000babff9b5d-b656d8860b04562f&q=1&e=46666acf-d70d-4e8d-8d00-b027808ae400&u=https%3A%2F%2Fkernelnewbies.org%2FKernelProjects%2Flarge-block-size
> > > > > [2] https://protect2.fireeye.com/v1/url?k=3f753ca2-5efe2994-3f74b7ed-000babff9b5d-e678f885471555e3&q=1&e=46666acf-d70d-4e8d-8d00-b027808ae400&u=https%3A%2F%2Fdocs.google.com%2Fspreadsheets%2Fd%2Fe%2F2PACX-1vS7sQfw90S00l2rfOKm83Jlg0px8KxMQE4HHp_DKRGbAGcAV-xu6LITHBEc4xzVh9wLH6WM2lR0cZS8%2Fpubhtml%23
> > > > > [3] https://lore.kernel.org/all/[email protected]/
> > > > > >
> > > > > > It seems like we only enable high order folios if the "noswap" mount
> > > > > > option is used, which is fairly recent. I doubt it is widely used.
> > > > >
> > > > > For now, I skipped the swap path as it currently lacks support for
> > > > > high order folios. But I'm currently looking into it as part of the LBS
> > > > > effort (please check spreadsheet at [2] for that).
> > > >
> > > > Thanks for the context, but I am not sure I understand.
> > > >
> > > > IIUC we are skipping allocating large folios in shmem if swap is
> > > > enabled in this patch. Swap does not support swapping out large folios
> > > > as a whole (except THPs), but page reclaim will split those large
> > > > folios and swap them out as order-0 pages anyway. So I am not sure I
> > > > understand why we need to skip allocating large folios if swap is
> > > > enabled.
> > >
> > > I lifted noswap condition and retested it again on top of 230918 and
> > > there is some regression. So, based on the results I guess the initial
> > > requirement may be the way to go. But what do you think?
> > >
> > > Here the logs:
> > > * shmem-large-folios-swap: https://gitlab.com/-/snippets/3600360
> > > * shmem-baseline-swap : https://gitlab.com/-/snippets/3600362
> > >
> > > -Failures: generic/080 generic/126 generic/193 generic/633 generic/689
> > > -Failed 5 of 730 tests
> > > \ No newline at end of file
> > > +Failures: generic/080 generic/103 generic/126 generic/193 generic/285 generic/436 generic/619 generic/633 generic/689
> > > +Failed 9 of 730 tests
> > > \ No newline at end of file
> > > >
> >
> > I am not really familiar with these tests so I cannot really tell
> > what's going on. I can see "swapfiles are not supported" in the logs
> > though, so it seems like we are seeing extra failures by just lifting
> > "noswap" even without actually swapping. I am curious if this is just
> > hiding a different issue, I would at least try to understand what's
> > happening.
> >
> > Anyway, I don't have enough context here to be useful. I was just
> > making an observation about reclaim splitting shmem folios to swap
> > them out as order-0 pages, and asking why this is needed based on
> > that. I will leave it up to you and the reviewers to decide if there's
> > anything interesting here.
>
> The tests which are failing seem be related to permissions, I could not
> immediate decipher why, because as you suggest we'd just be doing the
> silly thing of splitting large folios on writepage.
>
> I'd prefer we don't require swap until those regressions would be fixed.
>
> Note that part of the rationale to enable this work is to eventually
> also extend swap code to support large order folios, so it is not like
> this would be left as-is. It is just that it may take time to resolve
> the kinks with swap.
>
> So I'd stick to nowap for now.
>
> The above tests also don't stress swap too, and if we do that I would
> imagine we might see some other undesirable failures.
>
> Luis
I thought we already have some notion of exercising swap with large
shmem folios from THPs, so this shouldn't be new, but perhaps I am
missing something.
On Tue, Sep 19, 2023 at 09:00:16AM -0700, Yosry Ahmed wrote:
> On Tue, Sep 19, 2023 at 6:27 AM Daniel Gomez <[email protected]> wrote:
> >
> > On Mon, Sep 18, 2023 at 11:55:34AM -0700, Yosry Ahmed wrote:
> > > On Mon, Sep 18, 2023 at 1:00 AM Daniel Gomez <[email protected]> wrote:
> > > >
> > > > On Fri, Sep 15, 2023 at 11:26:37AM -0700, Yosry Ahmed wrote:
> > > > > On Fri, Sep 15, 2023 at 2:51 AM Daniel Gomez <[email protected]> wrote:
> > > > > >
> > > > > > Add large folio support for shmem write path matching the same high
> > > > > > order preference mechanism used for iomap buffered IO path as used in
> > > > > > __filemap_get_folio().
> > > > > >
> > > > > > Use the __folio_get_max_order to get a hint for the order of the folio
> > > > > > based on file size which takes care of the mapping requirements.
> > > > > >
> > > > > > Swap does not support high order folios for now, so make it order 0 in
> > > > > > case swap is enabled.
> > > > >
> > > > > I didn't take a close look at the series, but I am not sure I
> > > > > understand the rationale here. Reclaim will split high order shmem
> > > > > folios anyway, right?
> > > >
> > > > For context, this is part of the enablement of large block sizes (LBS)
> > > > effort [1][2][3], so the assumption here is that the kernel will
> > > > reclaim memory with the same (large) block sizes that were written to
> > > > the device.
> > > >
> > > > I'll add more context in the V2.
> > > >
> > > > [1] https://protect2.fireeye.com/v1/url?k=a80aab33-c981be05-a80b207c-000babff9b5d-b656d8860b04562f&q=1&e=46666acf-d70d-4e8d-8d00-b027808ae400&u=https%3A%2F%2Fkernelnewbies.org%2FKernelProjects%2Flarge-block-size
> > > > [2] https://protect2.fireeye.com/v1/url?k=3f753ca2-5efe2994-3f74b7ed-000babff9b5d-e678f885471555e3&q=1&e=46666acf-d70d-4e8d-8d00-b027808ae400&u=https%3A%2F%2Fdocs.google.com%2Fspreadsheets%2Fd%2Fe%2F2PACX-1vS7sQfw90S00l2rfOKm83Jlg0px8KxMQE4HHp_DKRGbAGcAV-xu6LITHBEc4xzVh9wLH6WM2lR0cZS8%2Fpubhtml%23
> > > > [3] https://lore.kernel.org/all/[email protected]/
> > > > >
> > > > > It seems like we only enable high order folios if the "noswap" mount
> > > > > option is used, which is fairly recent. I doubt it is widely used.
> > > >
> > > > For now, I skipped the swap path as it currently lacks support for
> > > > high order folios. But I'm currently looking into it as part of the LBS
> > > > effort (please check spreadsheet at [2] for that).
> > >
> > > Thanks for the context, but I am not sure I understand.
> > >
> > > IIUC we are skipping allocating large folios in shmem if swap is
> > > enabled in this patch. Swap does not support swapping out large folios
> > > as a whole (except THPs), but page reclaim will split those large
> > > folios and swap them out as order-0 pages anyway. So I am not sure I
> > > understand why we need to skip allocating large folios if swap is
> > > enabled.
> >
> > I lifted noswap condition and retested it again on top of 230918 and
> > there is some regression. So, based on the results I guess the initial
> > requirement may be the way to go. But what do you think?
> >
> > Here the logs:
> > * shmem-large-folios-swap: https://gitlab.com/-/snippets/3600360
> > * shmem-baseline-swap : https://gitlab.com/-/snippets/3600362
> >
> > -Failures: generic/080 generic/126 generic/193 generic/633 generic/689
> > -Failed 5 of 730 tests
> > \ No newline at end of file
> > +Failures: generic/080 generic/103 generic/126 generic/193 generic/285 generic/436 generic/619 generic/633 generic/689
> > +Failed 9 of 730 tests
> > \ No newline at end of file
> > >
>
> I am not really familiar with these tests so I cannot really tell
> what's going on. I can see "swapfiles are not supported" in the logs
> though, so it seems like we are seeing extra failures by just lifting
> "noswap" even without actually swapping. I am curious if this is just
> hiding a different issue, I would at least try to understand what's
> happening.
>
> Anyway, I don't have enough context here to be useful. I was just
> making an observation about reclaim splitting shmem folios to swap
> them out as order-0 pages, and asking why this is needed based on
> that. I will leave it up to you and the reviewers to decide if there's
> anything interesting here.
The tests which are failing seem be related to permissions, I could not
immediate decipher why, because as you suggest we'd just be doing the
silly thing of splitting large folios on writepage.
I'd prefer we don't require swap until those regressions would be fixed.
Note that part of the rationale to enable this work is to eventually
also extend swap code to support large order folios, so it is not like
this would be left as-is. It is just that it may take time to resolve
the kinks with swap.
So I'd stick to nowap for now.
The above tests also don't stress swap too, and if we do that I would
imagine we might see some other undesirable failures.
Luis