lkml.org 
[lkml]   [2016]   [Apr]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 1/5] dax: add dax_get_unmapped_area for pmd mappings
From
Date
On Tue, 2016-04-12 at 23:18 -0400, Matthew Wilcox wrote:
> On Tue, Apr 12, 2016 at 02:39:15PM -0600, Toshi Kani wrote:
> >
> > + * When the target file is not a DAX file, @addr is specified, the
> > + * request is not suitable for pmd mappings, or mm-
> > >get_unmapped_area()
> > + * failed with extended @len, it simply calls the default handler,
> > + * mm->get_unmapped_area(), with the original arguments.
>
> I think you can lose this paragraph.  It describes what the function
> does, not why it does it ... and we can see what the function does from
> reading the code.

Agreed.  I will remove this paragraph.

> I think this is one of those functions which is actually improved with
> gotos, purely to reduce the indentation level.
>
> unsigned long dax_get_unmapped_area(struct file *filp, unsigned long
> addr,
> unsigned long len, unsigned long pgoff, unsigned long
> flags)
> {
> unsigned long off, off_end, off_pmd, len_pmd, addr_pmd;
>
> if (!IS_ENABLED(CONFIG_FS_DAX_PMD) ||
>     !filp || addr || !IS_DAX(filp->f_mapping->host))
> goto out;
>
> off = pgoff << PAGE_SHIFT;
> off_end = off + len;
> off_pmd = round_up(off, PMD_SIZE);
> if ((off_end <= off_pmd) || ((off_end - off_pmd) < PMD_SIZE))
> goto out;
>
> len_pmd = len + PMD_SIZE;
> addr_pmd = current->mm->get_unmapped_area(filp, addr, len_pmd,
> pgoff, flags);
>
> if (!IS_ERR_VALUE(addr_pmd)) {
> addr_pmd += (off - addr_pmd) & (PMD_SIZE - 1);
> return addr_pmd;
> }
>
>  out:
> return current->mm->get_unmapped_area(filp, addr, len, pgoff,
> flags);
> }

Sounds good.

> Now ... back to the original version, some questions:
>
> >
> > +unsigned long dax_get_unmapped_area(struct file *filp, unsigned long
> > addr,
> > + unsigned long len, unsigned long pgoff, unsigned long
> > flags)
> > +{
> > + unsigned long off, off_end, off_pmd, len_pmd, addr_pmd;
> > +
> > + if (IS_ENABLED(CONFIG_FS_DAX_PMD) &&
> > +     filp && !addr && IS_DAX(filp->f_mapping->host)) {
> > + off = pgoff << PAGE_SHIFT;
> > + off_end = off + len;
>
> Can off + len wrap here, or did that get checked earlier?

Yes, do_mmap() has checked this condition earlier.  But, I think I need to
check it for (off + len_pmd).

> >
> > + off_pmd = round_up(off, PMD_SIZE);
> > +
> > + if ((off_end > off_pmd) && ((off_end - off_pmd) >=
> > PMD_SIZE)) {
>
> We're only going to look for a PMD-aligned mapping if the mapping is at
> least double PMD_SIZE?  I don't understand that decision.  Seems to me
> that if I ask to map offset 4MB, length 2MB, I should get a PMD-aligned
> mapping.

It checks if this request can be covered by a PMD page.  'off_pmd' is the
first PMD-aligned offset.  There needs to be at least 2MB from this offset
to the end, 'off_end', in order to cover with a PMD page.

In your example, 'off_pmd' is still 4MB, which has 2MB to the end.  So, so
this request can be covered by a PMD page.

Another example, say, offset 4KB and length 2MB.  'off_pmd' is 2MB, which
has only 4KB to the end.  So, this request cannot be covered by a PMD page.

> Speaking of offset, we don't have any checks that offset is a multiple
> of PMD_SIZE.  I know that theoretically we could map offset 1.5MB, length
> 3MB and see the first 0.5MB filled with small pages, then the next 2MB
> filled with one large page, and the tail filled with small pages, but I
> think we're better off only looking for PMD-alignment if the user asked
> for an aligned offset in the file.

Yes, that's what it checks.  In this case, 'off_pmd' is set to 2MB, which
has 2.5MB to the end.  So, it can be covered by a PMD page.  The offset
itself does not have to be aligned by 2MB.


> > + len_pmd = len + PMD_SIZE;
> > +
> > + addr_pmd = current->mm->get_unmapped_area(
> > + filp, addr, len_pmd, pgoff,
> > flags);
> > +
> > + if (!IS_ERR_VALUE(addr_pmd)) {
> > + addr_pmd += (off - addr_pmd) &
> > (PMD_SIZE - 1);
>
> ... then you wouldn't need this calculation ;-)

Applications should not be required to provide a 2MB-aligned offset.

Thanks,
-Toshi

\
 
 \ /
  Last update: 2016-04-13 17:21    [W:0.045 / U:0.592 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site