Re: [PATCH v4 1/2] thp, dax: add thp_get_unmapped_area for pmd mappings

From: Toshi Kani
Date: Mon Apr 25 2016 - 12:15:13 EST


On Mon, 2016-04-25 at 01:50 +0300, Kirill A. Shutemov wrote:
> On Fri, Apr 22, 2016 at 06:21:22PM -0600, Toshi Kani wrote:
> >
Â:
> > +unsigned long __thp_get_unmapped_area(struct file *filp, unsigned long
> > len,
> > + loff_t off, unsigned long flags, unsigned long size)
> > +{
> > + unsigned long addr;
> > + loff_t off_end = off + len;
> > + loff_t off_align = round_up(off, size);
> > + unsigned long len_pad;
> > +
> > + if (off_end <= off_align || (off_end - off_align) < size)
> > + return 0;
> > +
> > + len_pad = len + size;
> > + if (len_pad < len || (off + len_pad) < off)
> > + return 0;
> > +
> > + addr = current->mm->get_unmapped_area(filp, 0, len_pad,
> > + ÂÂÂÂÂÂoff >> PAGE_SHIFT,
> > flags);
> > + if (IS_ERR_VALUE(addr))
> > + return 0;
> > +
> > + addr += (off - addr) & (size - 1);
> > + return addr;
>
> Hugh has more sanity checks before and after call to get_unmapped_area().
> Please, consider borrowing them.

This function only checks if the request is qualified for THP mappings. It
tries not to step into the implementation of the allocation code current-
>mm->get_unmapped_area(), such asÂarch_get_unmapped_area_topdown() on x86.

Let me walk thru Hugh's checks to make sure I am not missing something:

---(Hugh's checks)---
| + if (len > TASK_SIZE)
| + return -ENOMEM;

This check is made by arch_get_unmapped_area_topdown().

| +
| + get_area = current->mm->get_unmapped_area;
| + addr = get_area(file, uaddr, len, pgoff, flags);
| +
| + if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE))
| + return addr;

thp_get_unmapped_area() is defined to NULL in this case.

| + if (IS_ERR_VALUE(addr))
| + return addr;

Checked in my patch.

| + if (addr & ~PAGE_MASK)
| + return addr;

arch_get_unmapped_area_topdown() aligns 'addr' unless MAP_FIXED is set. No
need to check in this func.

| + if (addr > TASK_SIZE - len)
| + return addr;

The allocation code needs to assure this case.

| + if (shmem_huge == SHMEM_HUGE_DENY)
| + return addr;

This check is specific to Hugh's patch.

| + if (len < HPAGE_PMD_SIZE)
| + return addr;

Checked in my patch.

| + if (flags & MAP_FIXED)
| + return addr;

Checked by arch_get_unmapped_area_topdown().

| + /*
| + Â* Our priority is to support MAP_SHARED mapped hugely;
| + Â* and support MAP_PRIVATE mapped hugely too, until it is COWed.
| + Â* But if caller specified an address hint, respect that as
before.
| + Â*/
| + if (uaddr)
| + return addr;

Checked in my patch.

(cut)

| + offset = (pgoff << PAGE_SHIFT) & (HPAGE_PMD_SIZE-1);
| + if (offset && offset + len < 2 * HPAGE_PMD_SIZE)
| + return addr;

Checked in my patch.

| + if ((addr & (HPAGE_PMD_SIZE-1)) == offset)
| + return addr;

This is a lucky case, i.e. the 1st get_unmapped_area() call returned an
aligned addr. Not applicable to my patch.

| +
| + inflated_len = len + HPAGE_PMD_SIZE - PAGE_SIZE;
| + if (inflated_len > TASK_SIZE)
| + return addr;

Checked by arch_get_unmapped_area_topdown().

| + if (inflated_len < len)
| + return addr;

Checked in my patch.

| + inflated_addr = get_area(NULL, 0, inflated_len, 0, flags);

Not sure why passing 'filp' and 'off' as NULL here.

| + if (IS_ERR_VALUE(inflated_addr))
| + return addr;

Checked in my patch.

| + if (inflated_addr & ~PAGE_MASK)
| + return addr;

Hmm... if this happens, it is a bug in the allocation code. I do not think
this check is necessary.

| + inflated_offset = inflated_addr & (HPAGE_PMD_SIZE-1);
| + inflated_addr += offset - inflated_offset;
| + if (inflated_offset > offset)
| + inflated_addr += HPAGE_PMD_SIZE;
| +
| + if (inflated_addr > TASK_SIZE - len)
| + return addr;

The allocation code needs to assure this.

| + return inflated_addr;

> >
> > +}
> > +
> > +unsigned long thp_get_unmapped_area(struct file *filp, unsigned long
> > addr,
> > + unsigned long len, unsigned long pgoff, unsigned long
> > flags)
> > +{
> > + loff_t off = (loff_t)pgoff << PAGE_SHIFT;
> > +
> > + if (addr)
> > + goto out;
>
> I think it's too strong reaction to hint, isn't it?
> We definately need this for MAP_FIXED. But in general? Maybe.

It calls arch's get_unmapped_area() to proceed with the original args when
'addr' is passed. The arch's get_unmapped_are() then handles 'addr' as a
hint when MAP_FIXED is not set. This can be used as a hint to avoid using
THP mappings if a non-aligned address is passed. Hugh's code handles it in
the same way as well.

Thanks,
-Toshi