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

[Toshi Kani]

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