Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1472074 > unrolled thread

[PATCH v4 RESEND 0/2] Align mmap address for DAX pmd mappings

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 RESEND 0/2] Align mmap address for DAX pmd mappings Toshi Kani <toshi.kani@hpe.com> - 2016-08-29 21:20 +0200
    Re: [PATCH v4 RESEND 0/2] Align mmap address for DAX pmd mappings "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-08-29 22:50 +0200
      Re: [PATCH v4 RESEND 0/2] Align mmap address for DAX pmd mappings "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2016-08-29 23:40 +0200
        Re: [PATCH v4 RESEND 0/2] Align mmap address for DAX pmd mappings "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2016-08-30 00:40 +0200
          Re: [PATCH v4 RESEND 0/2] Align mmap address for DAX pmd mappings "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-09-08 13:00 +0200
            Re: [PATCH v4 RESEND 0/2] Align mmap address for DAX pmd mappings "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2016-09-08 15:50 +0200
              Re: [PATCH v4 RESEND 0/2] Align mmap address for DAX pmd mappings "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2016-09-09 01:40 +0200
                Re: [PATCH v4 RESEND 0/2] Align mmap address for DAX pmd mappings "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2016-09-09 14:40 +0200
                  Re: [PATCH v4 RESEND 0/2] Align mmap address for DAX pmd mappings "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2016-09-09 19:30 +0200
                    Re: [PATCH v4 RESEND 0/2] Align mmap address for DAX pmd mappings "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-09-09 20:40 +0200

#1472074 — [PATCH v4 RESEND 0/2] Align mmap address for DAX pmd mappings

When CONFIG_FS_DAX_PMD is set, DAX supports mmap() using pmd page
size.  This feature relies on both mmap virtual address and FS
block (i.e. physical address) to be aligned by the pmd page size.
Users can use mkfs options to specify FS to align block allocations.
However, aligning mmap address requires code changes to existing
applications for providing a pmd-aligned address to mmap().

For instance, fio with "ioengine=mmap" performs I/Os with mmap() [1].
It calls mmap() with a NULL address, which needs to be changed to
provide a pmd-aligned address for testing with DAX pmd mappings.
Changing all applications that call mmap() with NULL is undesirable.

This patch-set extends filesystems to align an mmap address for
a DAX file so that unmodified applications can use DAX pmd mappings.

[1]: https://github.com/axboe/fio/blob/master/engines/mmap.c

RESEND:
 - Rebased to 4.8.0-rc4, and drop blk as BLK_DEV_DAX was removed.

v4:
 - Use loff_t for offset and cast before shift (Jan Kara)
 - Remove redundant paranthesis (Jan Kara)
 - Allow integration with huge page cache support (Matthew Wilcox)
 - Prepare for PUD mapping support (Mike Kravetz, Matthew Wilcox)

v3:
 - Check overflow condition to offset + length. (Matthew Wilcox)
 - Remove indent by using gotos. (Matthew Wilcox)
 - Define dax_get_unmapped_area to NULL when CONFIG_FS_DAX is unset.
   (Matthew Wilcox)
 - Squash all filesystem patches together. (Matthew Wilcox)

v2:
 - Change filesystems to provide their get_unmapped_area().
   (Matthew Wilcox)
 - Add more description about the benefit. (Matthew Wilcox)

---
Toshi Kani (2):
 1/2 thp, dax: add thp_get_unmapped_area for pmd mappings
 2/2 ext2/4, xfs, blk: call thp_get_unmapped_area() for pmd mappings

---
 fs/ext2/file.c          |  1 +
 fs/ext4/file.c          |  1 +
 fs/xfs/xfs_file.c       |  1 +
 include/linux/huge_mm.h |  7 +++++++
 mm/huge_memory.c        | 43 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 53 insertions(+)

[toc] | [next] | [standalone]

#1472113

On Mon, Aug 29, 2016 at 01:11:19PM -0600, Toshi Kani wrote:
> When CONFIG_FS_DAX_PMD is set, DAX supports mmap() using pmd page
> size.  This feature relies on both mmap virtual address and FS
> block (i.e. physical address) to be aligned by the pmd page size.
> Users can use mkfs options to specify FS to align block allocations.
> However, aligning mmap address requires code changes to existing
> applications for providing a pmd-aligned address to mmap().
> 
> For instance, fio with "ioengine=mmap" performs I/Os with mmap() [1].
> It calls mmap() with a NULL address, which needs to be changed to
> provide a pmd-aligned address for testing with DAX pmd mappings.
> Changing all applications that call mmap() with NULL is undesirable.
> 
> This patch-set extends filesystems to align an mmap address for
> a DAX file so that unmodified applications can use DAX pmd mappings.

+Hugh

Can we get it used for shmem/tmpfs too?
I don't think we should duplicate essentially the same functionality in
multiple places.

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1472128

On Mon, 2016-08-29 at 23:48 +0300, Kirill A. Shutemov wrote:
> On Mon, Aug 29, 2016 at 01:11:19PM -0600, Toshi Kani wrote:
> > 
> > When CONFIG_FS_DAX_PMD is set, DAX supports mmap() using pmd page
> > size.  This feature relies on both mmap virtual address and FS
> > block (i.e. physical address) to be aligned by the pmd page size.
> > Users can use mkfs options to specify FS to align block
> > allocations. However, aligning mmap address requires code changes
> > to existing applications for providing a pmd-aligned address to
> > mmap().
> > 
> > For instance, fio with "ioengine=mmap" performs I/Os with mmap()
> > [1]. It calls mmap() with a NULL address, which needs to be changed
> > to provide a pmd-aligned address for testing with DAX pmd mappings.
> > Changing all applications that call mmap() with NULL is
> > undesirable.
> > 
> > This patch-set extends filesystems to align an mmap address for
> > a DAX file so that unmodified applications can use DAX pmd
> > mappings.
> 
> +Hugh
> 
> Can we get it used for shmem/tmpfs too?
> I don't think we should duplicate essentially the same functionality
> in multiple places.

Here is my brief analysis when I had looked at the Hugh's patch last
time (before shmem_get_unmapped_area() was accepted).
https://patchwork.kernel.org/patch/8916741/

Besides some differences in the logic, ex. shmem_get_unmapped_area()
always calls current->mm->get_unmapped_area twice, yes, they basically
provide the same functionality.

I think one issue is that shmem_get_unmapped_area() checks with its
static flag 'shmem_huge', and additinally deals with SHMEM_HUGE_DENY
and SHMEM_HUGE_FORCE cases.  It also handles non-file case for
!SHMEM_HUGE_FORCE.

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1472142

On Mon, 2016-08-29 at 15:31 -0600, Kani, Toshimitsu wrote:
> On Mon, 2016-08-29 at 23:48 +0300, Kirill A. Shutemov wrote:
> > 
> > On Mon, Aug 29, 2016 at 01:11:19PM -0600, Toshi Kani wrote:
> > > 
> > > 
> > > When CONFIG_FS_DAX_PMD is set, DAX supports mmap() using pmd page
> > > size.  This feature relies on both mmap virtual address and FS
> > > block (i.e. physical address) to be aligned by the pmd page size.
> > > Users can use mkfs options to specify FS to align block
> > > allocations. However, aligning mmap address requires code changes
> > > to existing applications for providing a pmd-aligned address to
> > > mmap().
> > > 
> > > For instance, fio with "ioengine=mmap" performs I/Os with mmap()
> > > [1]. It calls mmap() with a NULL address, which needs to be
> > > changed to provide a pmd-aligned address for testing with DAX pmd
> > > mappings. Changing all applications that call mmap() with NULL is
> > > undesirable.
> > > 
> > > This patch-set extends filesystems to align an mmap address for
> > > a DAX file so that unmodified applications can use DAX pmd
> > > mappings.
> > 
> > +Hugh
> > 
> > Can we get it used for shmem/tmpfs too?
> > I don't think we should duplicate essentially the same
> > functionality in multiple places.
> 
> Here is my brief analysis when I had looked at the Hugh's patch last
> time (before shmem_get_unmapped_area() was accepted).
> https://patchwork.kernel.org/patch/8916741/
> 
> Besides some differences in the logic, ex. shmem_get_unmapped_area()
> always calls current->mm->get_unmapped_area twice, yes, they
> basically provide the same functionality.
> 
> I think one issue is that shmem_get_unmapped_area() checks with its
> static flag 'shmem_huge', and additinally deals with SHMEM_HUGE_DENY
> and SHMEM_HUGE_FORCE cases.  It also handles non-file case for
> !SHMEM_HUGE_FORCE.

Looking further, these shmem_huge handlings only check pre-conditions.
 So, we should be able to make shmem_get_unmapped_area() as a wrapper,
which checks such shmem-specific conitions, and then
call __thp_get_unmapped_area() for the actual work.  All DAX-specific
checks are performed in thp_get_unmapped_area() as well.  We can make
 __thp_get_unmapped_area() as a common function.

I'd prefer to make such change as a separate item, but I can include it
to this patch series if needed. 

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1479087

On Mon, Aug 29, 2016 at 10:00:43PM +0000, Kani, Toshimitsu wrote:
> On Mon, 2016-08-29 at 15:31 -0600, Kani, Toshimitsu wrote:
> > On Mon, 2016-08-29 at 23:48 +0300, Kirill A. Shutemov wrote:
> > > 
> > > On Mon, Aug 29, 2016 at 01:11:19PM -0600, Toshi Kani wrote:
> > > > 
> > > > 
> > > > When CONFIG_FS_DAX_PMD is set, DAX supports mmap() using pmd page
> > > > size.  This feature relies on both mmap virtual address and FS
> > > > block (i.e. physical address) to be aligned by the pmd page size.
> > > > Users can use mkfs options to specify FS to align block
> > > > allocations. However, aligning mmap address requires code changes
> > > > to existing applications for providing a pmd-aligned address to
> > > > mmap().
> > > > 
> > > > For instance, fio with "ioengine=mmap" performs I/Os with mmap()
> > > > [1]. It calls mmap() with a NULL address, which needs to be
> > > > changed to provide a pmd-aligned address for testing with DAX pmd
> > > > mappings. Changing all applications that call mmap() with NULL is
> > > > undesirable.
> > > > 
> > > > This patch-set extends filesystems to align an mmap address for
> > > > a DAX file so that unmodified applications can use DAX pmd
> > > > mappings.
> > > 
> > > +Hugh
> > > 
> > > Can we get it used for shmem/tmpfs too?
> > > I don't think we should duplicate essentially the same
> > > functionality in multiple places.
> > 
> > Here is my brief analysis when I had looked at the Hugh's patch last
> > time (before shmem_get_unmapped_area() was accepted).
> > https://patchwork.kernel.org/patch/8916741/
> > 
> > Besides some differences in the logic, ex. shmem_get_unmapped_area()
> > always calls current->mm->get_unmapped_area twice, yes, they
> > basically provide the same functionality.
> > 
> > I think one issue is that shmem_get_unmapped_area() checks with its
> > static flag 'shmem_huge', and additinally deals with SHMEM_HUGE_DENY
> > and SHMEM_HUGE_FORCE cases.  It also handles non-file case for
> > !SHMEM_HUGE_FORCE.
> 
> Looking further, these shmem_huge handlings only check pre-conditions.
>  So, we should be able to make shmem_get_unmapped_area() as a wrapper,
> which checks such shmem-specific conitions, and then
> call __thp_get_unmapped_area() for the actual work.  All DAX-specific
> checks are performed in thp_get_unmapped_area() as well.  We can make
>  __thp_get_unmapped_area() as a common function.
> 
> I'd prefer to make such change as a separate item,

Do you have plan to submit such change?

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1479243

On Thu, 2016-09-08 at 13:57 +0300, Kirill A. Shutemov wrote:
> On Mon, Aug 29, 2016 at 10:00:43PM +0000, Kani, Toshimitsu wrote:
> > 
> > On Mon, 2016-08-29 at 15:31 -0600, Kani, Toshimitsu wrote:
> > > 
> > > On Mon, 2016-08-29 at 23:48 +0300, Kirill A. Shutemov wrote:
> > > > 
 :
> > Looking further, these shmem_huge handlings only check pre-
> > conditions.  So, we should 
> > be able to make shmem_get_unmapped_area() as a wrapper, which
> > checks such shmem-specific conitions, and
> > then call __thp_get_unmapped_area() for the actual work.  All DAX-
> > specific checks are performed in thp_get_unmapped_area() as well.
> >  We can make  __thp_get_unmapped_area() as a common function.
> > 
> > I'd prefer to make such change as a separate item,
> 
> Do you have plan to submit such change?

Yes, I will submit the change once I finish testing.

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1479579

On Thu, 2016-09-08 at 07:48 -0600, Kani, Toshimitsu wrote:
> On Thu, 2016-09-08 at 13:57 +0300, Kirill A. Shutemov wrote:
> > 
> > On Mon, Aug 29, 2016 at 10:00:43PM +0000, Kani, Toshimitsu wrote:
 :
> > > 
> > > Looking further, these shmem_huge handlings only check pre-
> > > conditions.  So, we should be able to make shmem_get_unmapped_are
> > > a() as a wrapper, which checks such shmem-specific conitions, and
> > > then call __thp_get_unmapped_area() for the actual work.  All
> > > DAX-specific checks are performed in thp_get_unmapped_area() as
> > > well.  We can make  __thp_get_unmapped_area() as a common
> > > function.
> > > 
> > > I'd prefer to make such change as a separate item,
> > 
> > Do you have plan to submit such change?
> 
> Yes, I will submit the change once I finish testing.

I found a bug in the current code, and need some clarification.  The
if-statement below is reverted.

===
diff --git a/mm/shmem.c b/mm/shmem.c
index fd8b2b5..aec5b49 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1980,7 +1980,7 @@ unsigned long shmem_get_unmapped_area(struct file
*file,
                                return addr;
                        sb = shm_mnt->mnt_sb;
                }
-               if (SHMEM_SB(sb)->huge != SHMEM_HUGE_NEVER)
+               if (SHMEM_SB(sb)->huge == SHMEM_HUGE_NEVER)
                        return addr;
        }
===

Because of this bug, mounting tmpfs with "huge=never" enables huge page
mappings, and "huge=always" or others disables it...

The above simple change will change the default behavior, though.  When
"huge=" option is not specified, SHMEM_SB(sb)->huge is set to zero,
which is SHMEM_HUGE_NEVER.  Therefore, huge page mappings are enabled
by default because of this bug.

What's the intended default behavior of this feature?

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1479943

On Thu, Sep 08, 2016 at 11:21:46PM +0000, Kani, Toshimitsu wrote:
> On Thu, 2016-09-08 at 07:48 -0600, Kani, Toshimitsu wrote:
> > On Thu, 2016-09-08 at 13:57 +0300, Kirill A. Shutemov wrote:
> > > 
> > > On Mon, Aug 29, 2016 at 10:00:43PM +0000, Kani, Toshimitsu wrote:
>  :
> > > > 
> > > > Looking further, these shmem_huge handlings only check pre-
> > > > conditions.  So, we should be able to make shmem_get_unmapped_are
> > > > a() as a wrapper, which checks such shmem-specific conitions, and
> > > > then call __thp_get_unmapped_area() for the actual work.  All
> > > > DAX-specific checks are performed in thp_get_unmapped_area() as
> > > > well.  We can make  __thp_get_unmapped_area() as a common
> > > > function.
> > > > 
> > > > I'd prefer to make such change as a separate item,
> > > 
> > > Do you have plan to submit such change?
> > 
> > Yes, I will submit the change once I finish testing.
> 
> I found a bug in the current code, and need some clarification.  The
> if-statement below is reverted.

<two-hands-facepalm>

Yeah. It was repored by Hillf[1]. The fixup got lost. :(

Could you post a proper patch with the fix?

I would be nice to credit Hillf there too.

[1] http://lkml.kernel.org/r/054f01d1c86f$2994d5c0$7cbe8140$@alibaba-inc.com

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1480216

On Fri, 2016-09-09 at 15:36 +0300, Kirill A. Shutemov wrote:
> On Thu, Sep 08, 2016 at 11:21:46PM +0000, Kani, Toshimitsu wrote:
> > 
> > On Thu, 2016-09-08 at 07:48 -0600, Kani, Toshimitsu wrote:
> > > 
> > > On Thu, 2016-09-08 at 13:57 +0300, Kirill A. Shutemov wrote:
> > > > 
> > > > On Mon, Aug 29, 2016 at 10:00:43PM +0000, Kani, Toshimitsu
> > > > wrote:
> >  :
> > > > > Looking further, these shmem_huge handlings only check pre-
> > > > > conditions.  So, we should be able to make shmem_get_unmapped
> > > > > _are a() as a wrapper, which checks such shmem-specific
> > > > > conitions, and then call __thp_get_unmapped_area() for the
> > > > > actual work.  All DAX-specific checks are performed in
> > > > > thp_get_unmapped_area() as well.  We can make
> > > > >  __thp_get_unmapped_area() as a common
> > > > > function.
> > > > > 
> > > > > I'd prefer to make such change as a separate item,
> > > > 
> > > > Do you have plan to submit such change?
> > > 
> > > Yes, I will submit the change once I finish testing.
> > 
> > I found a bug in the current code, and need some clarification.
> >  The if-statement below is reverted.
> 
> <two-hands-facepalm>
> 
> Yeah. It was repored by Hillf[1]. The fixup got lost. :(
> 
> Could you post a proper patch with the fix?
>
> I would be nice to credit Hillf there too.
> 
> [1] http://lkml.kernel.org/r/054f01d1c86f$2994d5c0$7cbe8140$@alibaba-
> inc.com

Yes, I will submit the fix as well.

I will not change the default value of sbinfo->huge in this fix.  So,
user will have to specify "huge=" option to enable huge page mappings.
 If this is not desireable, we will need a separate patch.

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1480249

On Fri, Sep 09, 2016 at 05:21:40PM +0000, Kani, Toshimitsu wrote:
> On Fri, 2016-09-09 at 15:36 +0300, Kirill A. Shutemov wrote:
> > On Thu, Sep 08, 2016 at 11:21:46PM +0000, Kani, Toshimitsu wrote:
> > > 
> > > On Thu, 2016-09-08 at 07:48 -0600, Kani, Toshimitsu wrote:
> > > > 
> > > > On Thu, 2016-09-08 at 13:57 +0300, Kirill A. Shutemov wrote:
> > > > > 
> > > > > On Mon, Aug 29, 2016 at 10:00:43PM +0000, Kani, Toshimitsu
> > > > > wrote:
> > >  :
> > > > > > Looking further, these shmem_huge handlings only check pre-
> > > > > > conditions.  So, we should be able to make shmem_get_unmapped
> > > > > > _are a() as a wrapper, which checks such shmem-specific
> > > > > > conitions, and then call __thp_get_unmapped_area() for the
> > > > > > actual work.  All DAX-specific checks are performed in
> > > > > > thp_get_unmapped_area() as well.  We can make
> > > > > >  __thp_get_unmapped_area() as a common
> > > > > > function.
> > > > > > 
> > > > > > I'd prefer to make such change as a separate item,
> > > > > 
> > > > > Do you have plan to submit such change?
> > > > 
> > > > Yes, I will submit the change once I finish testing.
> > > 
> > > I found a bug in the current code, and need some clarification.
> > >  The if-statement below is reverted.
> > 
> > <two-hands-facepalm>
> > 
> > Yeah. It was repored by Hillf[1]. The fixup got lost. :(
> > 
> > Could you post a proper patch with the fix?
> >
> > I would be nice to credit Hillf there too.
> > 
> > [1] http://lkml.kernel.org/r/054f01d1c86f$2994d5c0$7cbe8140$@alibaba-
> > inc.com
> 
> Yes, I will submit the fix as well.
> 
> I will not change the default value of sbinfo->huge in this fix.  So,
> user will have to specify "huge=" option to enable huge page mappings.
>  If this is not desireable, we will need a separate patch.

That's okay. I only screwed up part wich make vma aligned. Allocation of
huge page happens under right condition.

-- 
 Kirill A. Shutemov

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web