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

Groups > linux.kernel > #1252380 > unrolled thread

[PATCH v2 0/4] hugetlbfs fallocate hole punch race with page faults

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/4] hugetlbfs fallocate hole punch race with page faults Mike Kravetz <mike.kravetz@oracle.com> - 2015-10-21 02:00 +0200
    [PATCH v2 2/4] mm/hugetlb: Setup hugetlb_falloc during fallocate hole punch Mike Kravetz <mike.kravetz@oracle.com> - 2015-10-21 02:00 +0200
      Re: [PATCH v2 2/4] mm/hugetlb: Setup hugetlb_falloc during fallocate  hole punch Dave Hansen <dave.hansen@intel.com> - 2015-10-21 02:20 +0200
        Re: [PATCH v2 2/4] mm/hugetlb: Setup hugetlb_falloc during fallocate  hole punch Mike Kravetz <mike.kravetz@oracle.com> - 2015-10-21 03:10 +0200
    Re: [PATCH v2 0/4] hugetlbfs fallocate hole punch race with page  faults Hugh Dickins <hughd@google.com> - 2015-10-28 04:40 +0100
      Re: [PATCH v2 0/4] hugetlbfs fallocate hole punch race with page  faults Mike Kravetz <mike.kravetz@oracle.com> - 2015-10-28 17:10 +0100
        Re: [PATCH v2 0/4] hugetlbfs fallocate hole punch race with page  faults Hugh Dickins <hughd@google.com> - 2015-10-28 22:10 +0100
          Re: [PATCH v2 0/4] hugetlbfs fallocate hole punch race with page  faults Mike Kravetz <mike.kravetz@oracle.com> - 2015-10-28 22:20 +0100
            Re: [PATCH v2 0/4] hugetlbfs fallocate hole punch race with page  faults Mike Kravetz <mike.kravetz@oracle.com> - 2015-10-29 01:30 +0100

#1252380 — [PATCH v2 0/4] hugetlbfs fallocate hole punch race with page faults

The hugetlbfs fallocate hole punch code can race with page faults.  The
result is that after a hole punch operation, pages may remain within the
hole.  No other side effects of this race were observed.

In preparation for adding userfaultfd support to hugetlbfs, it is desirable
to close the window of this race.  This patch set starts by using the same
mechanism employed in shmem (see commit f00cdc6df7).  This greatly reduces
the race window.  However, it is still possible for the race to occur.

The current hugetlbfs code to remove pages did not deal with pages that
were mapped (because of such a race).  This patch set also adds code to
unmap pages in this rare case.  This unmapping of a single page happens
under the hugetlb_fault_mutex, so it can not be faulted again until the
end of the operation.

v2:
  Incorporated Andrew Morton's cleanups and added suggested comments
  Added patch 4/4 to unmap single pages in remove_inode_hugepages

Mike Kravetz (4):
  mm/hugetlb: Define hugetlb_falloc structure for hole punch race
  mm/hugetlb: Setup hugetlb_falloc during fallocate hole punch
  mm/hugetlb: page faults check for fallocate hole punch in progress and
    wait
  mm/hugetlb: Unmap pages to remove if page fault raced with hole punch

 fs/hugetlbfs/inode.c    | 155 ++++++++++++++++++++++++++++--------------------
 include/linux/hugetlb.h |  10 ++++
 mm/hugetlb.c            |  39 ++++++++++++
 3 files changed, 141 insertions(+), 63 deletions(-)

-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1252381 — [PATCH v2 2/4] mm/hugetlb: Setup hugetlb_falloc during fallocate hole punch

When performing a fallocate hole punch, set up a hugetlb_falloc struct
and make i_private point to it.  i_private will point to this struct for
the duration of the operation.  At the end of the operation, wake up
anyone who faulted on the hole and is on the waitq.

Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 fs/hugetlbfs/inode.c | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 316adb9..719bbe0 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -507,6 +507,7 @@ static long hugetlbfs_punch_hole(struct inode *inode, loff_t offset, loff_t len)
 {
 	struct hstate *h = hstate_inode(inode);
 	loff_t hpage_size = huge_page_size(h);
+	unsigned long hpage_shift = huge_page_shift(h);
 	loff_t hole_start, hole_end;
 
 	/*
@@ -518,8 +519,30 @@ static long hugetlbfs_punch_hole(struct inode *inode, loff_t offset, loff_t len)
 
 	if (hole_end > hole_start) {
 		struct address_space *mapping = inode->i_mapping;
+		DECLARE_WAIT_QUEUE_HEAD_ONSTACK(hugetlb_falloc_waitq);
+		/*
+		 * Page faults on the area to be hole punched must be stopped
+		 * during the operation.  Initialize struct and have
+		 * inode->i_private point to it.
+		 */
+		struct hugetlb_falloc hugetlb_falloc = {
+			.waitq = &hugetlb_falloc_waitq,
+			.start = hole_start >> hpage_shift,
+			.end = hole_end >> hpage_shift
+		};
 
 		mutex_lock(&inode->i_mutex);
+
+		/*
+		 * inode->i_private will be checked in the page fault path.
+		 * The locking assures that all writes to the structure are
+		 * complete before assigning to i_private.  A fault on another
+		 * CPU will see the fully initialized structure.
+		 */
+		spin_lock(&inode->i_lock);
+		inode->i_private = &hugetlb_falloc;
+		spin_unlock(&inode->i_lock);
+
 		i_mmap_lock_write(mapping);
 		if (!RB_EMPTY_ROOT(&mapping->i_mmap))
 			hugetlb_vmdelete_list(&mapping->i_mmap,
@@ -527,6 +550,12 @@ static long hugetlbfs_punch_hole(struct inode *inode, loff_t offset, loff_t len)
 						hole_end  >> PAGE_SHIFT);
 		i_mmap_unlock_write(mapping);
 		remove_inode_hugepages(inode, hole_start, hole_end);
+
+		spin_lock(&inode->i_lock);
+		inode->i_private = NULL;
+		wake_up_all(&hugetlb_falloc_waitq);
+		spin_unlock(&inode->i_lock);
+
 		mutex_unlock(&inode->i_mutex);
 	}
 
@@ -647,9 +676,6 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
 	if (!(mode & FALLOC_FL_KEEP_SIZE) && offset + len > inode->i_size)
 		i_size_write(inode, offset + len);
 	inode->i_ctime = CURRENT_TIME;
-	spin_lock(&inode->i_lock);
-	inode->i_private = NULL;
-	spin_unlock(&inode->i_lock);
 out:
 	mutex_unlock(&inode->i_mutex);
 	return error;
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1252387 — Re: [PATCH v2 2/4] mm/hugetlb: Setup hugetlb_falloc during fallocate hole punch

On 10/20/2015 04:52 PM, Mike Kravetz wrote:
>  	if (hole_end > hole_start) {
>  		struct address_space *mapping = inode->i_mapping;
> +		DECLARE_WAIT_QUEUE_HEAD_ONSTACK(hugetlb_falloc_waitq);
> +		/*
> +		 * Page faults on the area to be hole punched must be stopped
> +		 * during the operation.  Initialize struct and have
> +		 * inode->i_private point to it.
> +		 */
> +		struct hugetlb_falloc hugetlb_falloc = {
> +			.waitq = &hugetlb_falloc_waitq,
> +			.start = hole_start >> hpage_shift,
> +			.end = hole_end >> hpage_shift
> +		};
...
> @@ -527,6 +550,12 @@ static long hugetlbfs_punch_hole(struct inode *inode, loff_t offset, loff_t len)
>  						hole_end  >> PAGE_SHIFT);
>  		i_mmap_unlock_write(mapping);
>  		remove_inode_hugepages(inode, hole_start, hole_end);
> +
> +		spin_lock(&inode->i_lock);
> +		inode->i_private = NULL;
> +		wake_up_all(&hugetlb_falloc_waitq);
> +		spin_unlock(&inode->i_lock);

I see the shmem code doing something similar.  But, in the end, we're
passing the stack-allocated 'hugetlb_falloc_waitq' over to the page
faulting thread.  Is there something subtle that keeps
'hugetlb_falloc_waitq' from becoming invalid while the other task is
sleeping?

That wake_up_all() obviously can't sleep, but it seems like the faulting
thread's finish_wait() *HAS* to run before wake_up_all() can return.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1252398 — Re: [PATCH v2 2/4] mm/hugetlb: Setup hugetlb_falloc during fallocate hole punch

On 10/20/2015 05:11 PM, Dave Hansen wrote:
> On 10/20/2015 04:52 PM, Mike Kravetz wrote:
>>  	if (hole_end > hole_start) {
>>  		struct address_space *mapping = inode->i_mapping;
>> +		DECLARE_WAIT_QUEUE_HEAD_ONSTACK(hugetlb_falloc_waitq);
>> +		/*
>> +		 * Page faults on the area to be hole punched must be stopped
>> +		 * during the operation.  Initialize struct and have
>> +		 * inode->i_private point to it.
>> +		 */
>> +		struct hugetlb_falloc hugetlb_falloc = {
>> +			.waitq = &hugetlb_falloc_waitq,
>> +			.start = hole_start >> hpage_shift,
>> +			.end = hole_end >> hpage_shift
>> +		};
> ...
>> @@ -527,6 +550,12 @@ static long hugetlbfs_punch_hole(struct inode *inode, loff_t offset, loff_t len)
>>  						hole_end  >> PAGE_SHIFT);
>>  		i_mmap_unlock_write(mapping);
>>  		remove_inode_hugepages(inode, hole_start, hole_end);
>> +
>> +		spin_lock(&inode->i_lock);
>> +		inode->i_private = NULL;
>> +		wake_up_all(&hugetlb_falloc_waitq);
>> +		spin_unlock(&inode->i_lock);
> 
> I see the shmem code doing something similar.  But, in the end, we're
> passing the stack-allocated 'hugetlb_falloc_waitq' over to the page
> faulting thread.  Is there something subtle that keeps
> 'hugetlb_falloc_waitq' from becoming invalid while the other task is
> sleeping?
> 
> That wake_up_all() obviously can't sleep, but it seems like the faulting
> thread's finish_wait() *HAS* to run before wake_up_all() can return.
> 

The 'trick' is noted in the comment in the shmem_fault code:

                        /*
                         * shmem_falloc_waitq points into the
shmem_fallocate()
                         * stack of the hole-punching task:
shmem_falloc_waitq
                         * is usually invalid by the time we reach here, but
                         * finish_wait() does not dereference it in that
case;
                         * though i_lock needed lest racing with
wake_up_all().
                         */

The faulting thread is removed from the waitq when awakened with
wake_up_all().  See the DEFINE_WAIT() and supporting code in the
faulting thread.  Because of this, when the faulting thread calls
finish_wait() it does not access the waitq that was/is on the stack.

At least I've convinced myself it works this way. :)

-- 
Mike Kravetz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1257640 — Re: [PATCH v2 0/4] hugetlbfs fallocate hole punch race with page faults

On Tue, 20 Oct 2015, Mike Kravetz wrote:

> The hugetlbfs fallocate hole punch code can race with page faults.  The
> result is that after a hole punch operation, pages may remain within the
> hole.  No other side effects of this race were observed.
> 
> In preparation for adding userfaultfd support to hugetlbfs, it is desirable
> to close the window of this race.  This patch set starts by using the same
> mechanism employed in shmem (see commit f00cdc6df7).  This greatly reduces
> the race window.  However, it is still possible for the race to occur.
> 
> The current hugetlbfs code to remove pages did not deal with pages that
> were mapped (because of such a race).  This patch set also adds code to
> unmap pages in this rare case.  This unmapping of a single page happens
> under the hugetlb_fault_mutex, so it can not be faulted again until the
> end of the operation.
> 
> v2:
>   Incorporated Andrew Morton's cleanups and added suggested comments
>   Added patch 4/4 to unmap single pages in remove_inode_hugepages
> 
> Mike Kravetz (4):
>   mm/hugetlb: Define hugetlb_falloc structure for hole punch race
>   mm/hugetlb: Setup hugetlb_falloc during fallocate hole punch
>   mm/hugetlb: page faults check for fallocate hole punch in progress and
>     wait
>   mm/hugetlb: Unmap pages to remove if page fault raced with hole punch
> 
>  fs/hugetlbfs/inode.c    | 155 ++++++++++++++++++++++++++++--------------------
>  include/linux/hugetlb.h |  10 ++++
>  mm/hugetlb.c            |  39 ++++++++++++
>  3 files changed, 141 insertions(+), 63 deletions(-)

With the addition of i_mmap_lock_write() around hugetlb_vmdelete_list()
in 4/4 (that you already sent a patch for), and two very minor and
inessential mods to the test in 3/4 that I'll suggest in reply to that,
this all looks correct to me.

And yet, and yet...

... I have to say that it looks like a bunch of unnecessary complexity:
for which the only justification you give is above, not in any of the
patches: "In preparation for adding userfaultfd support to hugetlbfs,
it is desirable to close the window of this race" (pages faulted into
the hole during holepunch may be left there afterwards).

Of course, the code you've sampled from shmem.c is superb ;) and it
all should work as you have it (and I wouldn't want to tinker with it,
to try and make it simpler here, it's just to easy to get it wrong);
but it went into shmem.c for very different reasons.

I don't know whether you studied the sequence of 1aac1400319d before,
then f00cdc6df7d7 which you cite, then 8e205f779d14 which corrected it,
then b1a366500bd5 which plugged the remaining holes: I had to refresh
my memory of how it came to be like this.

The driving reasons for it in shmem.c were my aversion to making the
shmem inode any bigger for such an offbeat case (but hugetlbfs wouldn't
expect a large number of tiny files, to justify such space saving); my
aversion to adding further locking into the fast path of shmem_fault();
the fact that we already had this fallocate range checking mechanism
from the earliest of those commits; and the CVE number that meant that
the starvation issue had to be taken more seriously than it deserved.

One of the causes of that starvation issue was the fallback unmapping
of single pages within the holepunch loop: your 4/4 actually introduces
that into hugetlbfs for the first time.  Another cause of the starvation
issue was the "for ( ; ; )" pincer loop that I reverted in the last of
those commits above: whereas hugetlbfs just has a straightforward start
to end loop there.

It's inherent in the nature of holepunching, that immediately after
the holepunch someone may fault into the hole: I wouldn't have bothered
to make those changes in shmem.c, if there hadn't been the CVE starvation
issue - which I doubt hugetlbfs has before your changes.

But I've found some linux-mm mail from you to Andrea on Sep 30, in a
00/12 userfaultfd thread, where you say "I would like a mechanism to
catch all new huge page allocations as a result of page faults" and
"They would like to 'catch' any tasks that (incorrectly) fault in a
page after hole punch".

I confess I've not done my userfaultfd homework: perhaps it does give
you very good reason for this hugetlbfs holepunch-fault race series, but
I'd really like you to explain that in more detail, before I can Ack it.

But it sounds to me more as if the holes you want punched are not
quite like on other filesystems, and you want to be able to police
them afterwards with userfaultfd, to prevent them from being refilled.

Can't userfaultfd be used just slightly earlier, to prevent them from
being filled while doing the holepunch?  Then no need for this patchset?

Hugh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1258280 — Re: [PATCH v2 0/4] hugetlbfs fallocate hole punch race with page faults

On 10/27/2015 08:34 PM, Hugh Dickins wrote:
> On Tue, 20 Oct 2015, Mike Kravetz wrote:
> 
>> The hugetlbfs fallocate hole punch code can race with page faults.  The
>> result is that after a hole punch operation, pages may remain within the
>> hole.  No other side effects of this race were observed.
>>
>> In preparation for adding userfaultfd support to hugetlbfs, it is desirable
>> to close the window of this race.  This patch set starts by using the same
>> mechanism employed in shmem (see commit f00cdc6df7).  This greatly reduces
>> the race window.  However, it is still possible for the race to occur.
>>
>> The current hugetlbfs code to remove pages did not deal with pages that
>> were mapped (because of such a race).  This patch set also adds code to
>> unmap pages in this rare case.  This unmapping of a single page happens
>> under the hugetlb_fault_mutex, so it can not be faulted again until the
>> end of the operation.
>>
>> v2:
>>   Incorporated Andrew Morton's cleanups and added suggested comments
>>   Added patch 4/4 to unmap single pages in remove_inode_hugepages
>>
>> Mike Kravetz (4):
>>   mm/hugetlb: Define hugetlb_falloc structure for hole punch race
>>   mm/hugetlb: Setup hugetlb_falloc during fallocate hole punch
>>   mm/hugetlb: page faults check for fallocate hole punch in progress and
>>     wait
>>   mm/hugetlb: Unmap pages to remove if page fault raced with hole punch
>>
>>  fs/hugetlbfs/inode.c    | 155 ++++++++++++++++++++++++++++--------------------
>>  include/linux/hugetlb.h |  10 ++++
>>  mm/hugetlb.c            |  39 ++++++++++++
>>  3 files changed, 141 insertions(+), 63 deletions(-)
> 
> With the addition of i_mmap_lock_write() around hugetlb_vmdelete_list()
> in 4/4 (that you already sent a patch for), and two very minor and
> inessential mods to the test in 3/4 that I'll suggest in reply to that,
> this all looks correct to me.

Thanks for the detailed response Hugh.  I will try to address your questions
and provide more reasoning behind the use case and need for this code.

> 
> And yet, and yet...
> 
> ... I have to say that it looks like a bunch of unnecessary complexity:
> for which the only justification you give is above, not in any of the
> patches: "In preparation for adding userfaultfd support to hugetlbfs,
> it is desirable to close the window of this race" (pages faulted into
> the hole during holepunch may be left there afterwards).
> 
> Of course, the code you've sampled from shmem.c is superb ;)

That goes without saying. :)

>                                                              and it
> all should work as you have it (and I wouldn't want to tinker with it,
> to try and make it simpler here, it's just to easy to get it wrong);
> but it went into shmem.c for very different reasons.
> 
> I don't know whether you studied the sequence of 1aac1400319d before,
> then f00cdc6df7d7 which you cite, then 8e205f779d14 which corrected it,
> then b1a366500bd5 which plugged the remaining holes: I had to refresh
> my memory of how it came to be like this.

To be honest, no I did not study the evolution of this code.  I only
looked at the final result.

> 
> The driving reasons for it in shmem.c were my aversion to making the
> shmem inode any bigger for such an offbeat case (but hugetlbfs wouldn't
> expect a large number of tiny files, to justify such space saving); my

I did not seriously consider the option of expanding the size of
hugetlbfs inodes, simply based on your comments in shmem.  It would
certainly be easier to use something like a rw_semaphore to handle this
situation.  There is already a table of mutexes that are used (and taken)
during hugetlb faults.  Another mutex might not be too much additional
overhead in the fault path.

I would not claim to have enough experience to say whether or not the
additional space in hugetlbfs inodes would be worth the simplicity of
of code.  If you and others think this is the case, I would certainly
be open to take this path.

> aversion to adding further locking into the fast path of shmem_fault();
> the fact that we already had this fallocate range checking mechanism
> from the earliest of those commits; and the CVE number that meant that
> the starvation issue had to be taken more seriously than it deserved.
> 
> One of the causes of that starvation issue was the fallback unmapping
> of single pages within the holepunch loop: your 4/4 actually introduces
> that into hugetlbfs for the first time.  Another cause of the starvation
> issue was the "for ( ; ; )" pincer loop that I reverted in the last of
> those commits above: whereas hugetlbfs just has a straightforward start
> to end loop there.
> 
> It's inherent in the nature of holepunching, that immediately after
> the holepunch someone may fault into the hole: I wouldn't have bothered
> to make those changes in shmem.c, if there hadn't been the CVE starvation
> issue - which I doubt hugetlbfs has before your changes.
> 

Yes.  In the current hugetlbfs hole punch code, it knows that such
situations
exist.  It even 'cheats' and takes advantage of this for simpler code.  If
it notices a page fault during a hole punch, it simply leaves the page as
is and continues on with other pages in the hole.

> But I've found some linux-mm mail from you to Andrea on Sep 30, in a
> 00/12 userfaultfd thread, where you say "I would like a mechanism to
> catch all new huge page allocations as a result of page faults" and
> "They would like to 'catch' any tasks that (incorrectly) fault in a
> page after hole punch".
> 
> I confess I've not done my userfaultfd homework: perhaps it does give
> you very good reason for this hugetlbfs holepunch-fault race series, but
> I'd really like you to explain that in more detail, before I can Ack it.

Ok, here is a bit more explanation of the proposed use case.  It all
revolves around a DB's use of hugetlbfs and the desire for more control
over the underlying memory.  This additional control is achieved by
adding existing fallocate and userfaultfd semantics to hugetlbfs.

In this use case there is a single process that manages hugetlbfs files
and the underlying memory resources.  It pre-allocates/initializes these
files.

In addition, there are many other processes which access (rw mode) these
files.  They will simply mmap the files.  It is expected that they will
not fault in any new pages.  Rather, all pages would have been pre-allocated
by the management process.

At some time, the management process determines that specific ranges of
pages within the hugetlbfs files are no longer needed.  It will then punch
holes in the files.  These 'free' pages within the holes may then be used
for other purposes.  For applications like this (sophisticated DBs), huge
pages are reserved at system init time and closely managed by the
application.
Hence, the desire for this additional control.

So, when a hole containing N huge pages is punched, the management process
wants to know that it really has N huge pages for other purposes.  Ideally,
none of the other processes mapping this file/area would access the hole.
This is an application error, and it can be 'caught' with  userfaultfd.

Since these other (non-management) processes will never fault in pages,
they would simply set up userfaultfd to catch any page faults immediately
after mmaping the hugetlbfs file.

> 
> But it sounds to me more as if the holes you want punched are not
> quite like on other filesystems, and you want to be able to police
> them afterwards with userfaultfd, to prevent them from being refilled.

I am not sure if they are any different.

One could argue that a hole punch operation must always result in all
pages within the hole being deallocated.  As you point out, this could
race with a fault.  Previously, there would be no way to determine if
all pages had been deallocated because user space could not detect this
race.  Now, userfaultfd allows user space to catch page faults.  So,
it is now possible to catch/depend on hole punch deallocating all pages
within the hole.

> 
> Can't userfaultfd be used just slightly earlier, to prevent them from
> being filled while doing the holepunch?  Then no need for this patchset?

I do not think so, at least with current userfaultfd semantics.  The hole
needs to be punched before being caught with UFFDIO_REGISTER_MODE_MISSING.

> 
> Hugh
> 

-- 
Mike Kravetz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1258425 — Re: [PATCH v2 0/4] hugetlbfs fallocate hole punch race with page faults

On Wed, 28 Oct 2015, Mike Kravetz wrote:
> On 10/27/2015 08:34 PM, Hugh Dickins wrote:
> 
> Thanks for the detailed response Hugh.  I will try to address your questions
> and provide more reasoning behind the use case and need for this code.

And thank you for your detailed response, Mike: that helped a lot.

> Ok, here is a bit more explanation of the proposed use case.  It all
> revolves around a DB's use of hugetlbfs and the desire for more control
> over the underlying memory.  This additional control is achieved by
> adding existing fallocate and userfaultfd semantics to hugetlbfs.
> 
> In this use case there is a single process that manages hugetlbfs files
> and the underlying memory resources.  It pre-allocates/initializes these
> files.
> 
> In addition, there are many other processes which access (rw mode) these
> files.  They will simply mmap the files.  It is expected that they will
> not fault in any new pages.  Rather, all pages would have been pre-allocated
> by the management process.
> 
> At some time, the management process determines that specific ranges of
> pages within the hugetlbfs files are no longer needed.  It will then punch
> holes in the files.  These 'free' pages within the holes may then be used
> for other purposes.  For applications like this (sophisticated DBs), huge
> pages are reserved at system init time and closely managed by the
> application.
> Hence, the desire for this additional control.
> 
> So, when a hole containing N huge pages is punched, the management process
> wants to know that it really has N huge pages for other purposes.  Ideally,
> none of the other processes mapping this file/area would access the hole.
> This is an application error, and it can be 'caught' with  userfaultfd.
> 
> Since these other (non-management) processes will never fault in pages,
> they would simply set up userfaultfd to catch any page faults immediately
> after mmaping the hugetlbfs file.
> 
> > 
> > But it sounds to me more as if the holes you want punched are not
> > quite like on other filesystems, and you want to be able to police
> > them afterwards with userfaultfd, to prevent them from being refilled.
> 
> I am not sure if they are any different.
> 
> One could argue that a hole punch operation must always result in all
> pages within the hole being deallocated.  As you point out, this could
> race with a fault.  Previously, there would be no way to determine if
> all pages had been deallocated because user space could not detect this
> race.  Now, userfaultfd allows user space to catch page faults.  So,
> it is now possible to catch/depend on hole punch deallocating all pages
> within the hole.
> 
> > 
> > Can't userfaultfd be used just slightly earlier, to prevent them from
> > being filled while doing the holepunch?  Then no need for this patchset?
> 
> I do not think so, at least with current userfaultfd semantics.  The hole
> needs to be punched before being caught with UFFDIO_REGISTER_MODE_MISSING.

Great, that makes sense.

I was worried that you needed some kind of atomic treatment of the whole
extent punched, but all you need is to close the hole/fault race one
hugepage at a time.

Throw away all of 1/4, 2/4, 3/4: I think all you need is your 4/4
(plus i_mmap_lock_write around the hugetlb_vmdelete_list of course).

There you already do the single hugepage hugetlb_vmdelete_list()
under mutex_lock(&hugetlb_fault_mutex_table[hash]).

And it should come as no surprise that hugetlb_fault() does most
of its work under that same mutex.

So once remove_inode_hugepages() unlocks the mutex, that page is gone
from the file, and userfaultfd UFFDIO_REGISTER_MODE_MISSING will do
what you want, won't it?

I don't think "my" code buys you anything at all: you're not in danger of
shmem's starvation livelock issue, partly because remove_inode_hugepages()
uses the simple loop from start to end, and partly because hugetlb_fault()
already takes the serializing mutex (no equivalent in shmem_fault()).

Or am I dreaming?

Hugh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1258434 — Re: [PATCH v2 0/4] hugetlbfs fallocate hole punch race with page faults

On 10/28/2015 02:00 PM, Hugh Dickins wrote:
> On Wed, 28 Oct 2015, Mike Kravetz wrote:
>> On 10/27/2015 08:34 PM, Hugh Dickins wrote:
>>
>> Thanks for the detailed response Hugh.  I will try to address your questions
>> and provide more reasoning behind the use case and need for this code.
> 
> And thank you for your detailed response, Mike: that helped a lot.
> 
>> Ok, here is a bit more explanation of the proposed use case.  It all
>> revolves around a DB's use of hugetlbfs and the desire for more control
>> over the underlying memory.  This additional control is achieved by
>> adding existing fallocate and userfaultfd semantics to hugetlbfs.
>>
>> In this use case there is a single process that manages hugetlbfs files
>> and the underlying memory resources.  It pre-allocates/initializes these
>> files.
>>
>> In addition, there are many other processes which access (rw mode) these
>> files.  They will simply mmap the files.  It is expected that they will
>> not fault in any new pages.  Rather, all pages would have been pre-allocated
>> by the management process.
>>
>> At some time, the management process determines that specific ranges of
>> pages within the hugetlbfs files are no longer needed.  It will then punch
>> holes in the files.  These 'free' pages within the holes may then be used
>> for other purposes.  For applications like this (sophisticated DBs), huge
>> pages are reserved at system init time and closely managed by the
>> application.
>> Hence, the desire for this additional control.
>>
>> So, when a hole containing N huge pages is punched, the management process
>> wants to know that it really has N huge pages for other purposes.  Ideally,
>> none of the other processes mapping this file/area would access the hole.
>> This is an application error, and it can be 'caught' with  userfaultfd.
>>
>> Since these other (non-management) processes will never fault in pages,
>> they would simply set up userfaultfd to catch any page faults immediately
>> after mmaping the hugetlbfs file.
>>
>>>
>>> But it sounds to me more as if the holes you want punched are not
>>> quite like on other filesystems, and you want to be able to police
>>> them afterwards with userfaultfd, to prevent them from being refilled.
>>
>> I am not sure if they are any different.
>>
>> One could argue that a hole punch operation must always result in all
>> pages within the hole being deallocated.  As you point out, this could
>> race with a fault.  Previously, there would be no way to determine if
>> all pages had been deallocated because user space could not detect this
>> race.  Now, userfaultfd allows user space to catch page faults.  So,
>> it is now possible to catch/depend on hole punch deallocating all pages
>> within the hole.
>>
>>>
>>> Can't userfaultfd be used just slightly earlier, to prevent them from
>>> being filled while doing the holepunch?  Then no need for this patchset?
>>
>> I do not think so, at least with current userfaultfd semantics.  The hole
>> needs to be punched before being caught with UFFDIO_REGISTER_MODE_MISSING.
> 
> Great, that makes sense.
> 
> I was worried that you needed some kind of atomic treatment of the whole
> extent punched, but all you need is to close the hole/fault race one
> hugepage at a time.
> 
> Throw away all of 1/4, 2/4, 3/4: I think all you need is your 4/4
> (plus i_mmap_lock_write around the hugetlb_vmdelete_list of course).
> 
> There you already do the single hugepage hugetlb_vmdelete_list()
> under mutex_lock(&hugetlb_fault_mutex_table[hash]).
> 
> And it should come as no surprise that hugetlb_fault() does most
> of its work under that same mutex.
> 
> So once remove_inode_hugepages() unlocks the mutex, that page is gone
> from the file, and userfaultfd UFFDIO_REGISTER_MODE_MISSING will do
> what you want, won't it?
> 
> I don't think "my" code buys you anything at all: you're not in danger of
> shmem's starvation livelock issue, partly because remove_inode_hugepages()
> uses the simple loop from start to end, and partly because hugetlb_fault()
> already takes the serializing mutex (no equivalent in shmem_fault()).
> 
> Or am I dreaming?

I don't think you are dreaming.

I should have stepped back and thought about this more before before pulling
in the shmem code.  It really is only a 'page at a time' operation, and we
can use the fault mutex table for that.

I'll code it up with just the changes needed for 4/4 and put it through some
stress testing.

Thanks,
-- 
Mike Kravetz

> 
> Hugh
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1258499 — Re: [PATCH v2 0/4] hugetlbfs fallocate hole punch race with page faults

On 10/28/2015 02:13 PM, Mike Kravetz wrote:
> On 10/28/2015 02:00 PM, Hugh Dickins wrote:
>> On Wed, 28 Oct 2015, Mike Kravetz wrote:
>>> On 10/27/2015 08:34 PM, Hugh Dickins wrote:
>>>
>>> Thanks for the detailed response Hugh.  I will try to address your questions
>>> and provide more reasoning behind the use case and need for this code.
>>
>> And thank you for your detailed response, Mike: that helped a lot.
>>
>>> Ok, here is a bit more explanation of the proposed use case.  It all
>>> revolves around a DB's use of hugetlbfs and the desire for more control
>>> over the underlying memory.  This additional control is achieved by
>>> adding existing fallocate and userfaultfd semantics to hugetlbfs.
>>>
>>> In this use case there is a single process that manages hugetlbfs files
>>> and the underlying memory resources.  It pre-allocates/initializes these
>>> files.
>>>
>>> In addition, there are many other processes which access (rw mode) these
>>> files.  They will simply mmap the files.  It is expected that they will
>>> not fault in any new pages.  Rather, all pages would have been pre-allocated
>>> by the management process.
>>>
>>> At some time, the management process determines that specific ranges of
>>> pages within the hugetlbfs files are no longer needed.  It will then punch
>>> holes in the files.  These 'free' pages within the holes may then be used
>>> for other purposes.  For applications like this (sophisticated DBs), huge
>>> pages are reserved at system init time and closely managed by the
>>> application.
>>> Hence, the desire for this additional control.
>>>
>>> So, when a hole containing N huge pages is punched, the management process
>>> wants to know that it really has N huge pages for other purposes.  Ideally,
>>> none of the other processes mapping this file/area would access the hole.
>>> This is an application error, and it can be 'caught' with  userfaultfd.
>>>
>>> Since these other (non-management) processes will never fault in pages,
>>> they would simply set up userfaultfd to catch any page faults immediately
>>> after mmaping the hugetlbfs file.
>>>
>>>>
>>>> But it sounds to me more as if the holes you want punched are not
>>>> quite like on other filesystems, and you want to be able to police
>>>> them afterwards with userfaultfd, to prevent them from being refilled.
>>>
>>> I am not sure if they are any different.
>>>
>>> One could argue that a hole punch operation must always result in all
>>> pages within the hole being deallocated.  As you point out, this could
>>> race with a fault.  Previously, there would be no way to determine if
>>> all pages had been deallocated because user space could not detect this
>>> race.  Now, userfaultfd allows user space to catch page faults.  So,
>>> it is now possible to catch/depend on hole punch deallocating all pages
>>> within the hole.
>>>
>>>>
>>>> Can't userfaultfd be used just slightly earlier, to prevent them from
>>>> being filled while doing the holepunch?  Then no need for this patchset?
>>>
>>> I do not think so, at least with current userfaultfd semantics.  The hole
>>> needs to be punched before being caught with UFFDIO_REGISTER_MODE_MISSING.
>>
>> Great, that makes sense.
>>
>> I was worried that you needed some kind of atomic treatment of the whole
>> extent punched, but all you need is to close the hole/fault race one
>> hugepage at a time.
>>
>> Throw away all of 1/4, 2/4, 3/4: I think all you need is your 4/4
>> (plus i_mmap_lock_write around the hugetlb_vmdelete_list of course).
>>
>> There you already do the single hugepage hugetlb_vmdelete_list()
>> under mutex_lock(&hugetlb_fault_mutex_table[hash]).
>>
>> And it should come as no surprise that hugetlb_fault() does most
>> of its work under that same mutex.
>>
>> So once remove_inode_hugepages() unlocks the mutex, that page is gone
>> from the file, and userfaultfd UFFDIO_REGISTER_MODE_MISSING will do
>> what you want, won't it?
>>
>> I don't think "my" code buys you anything at all: you're not in danger of
>> shmem's starvation livelock issue, partly because remove_inode_hugepages()
>> uses the simple loop from start to end, and partly because hugetlb_fault()
>> already takes the serializing mutex (no equivalent in shmem_fault()).
>>
>> Or am I dreaming?
> 
> I don't think you are dreaming.
> 
> I should have stepped back and thought about this more before before pulling
> in the shmem code.  It really is only a 'page at a time' operation, and we
> can use the fault mutex table for that.
> 
> I'll code it up with just the changes needed for 4/4 and put it through some
> stress testing.

Thanks again Hugh.  Testing was successful:  current hugetlbfs fallocate
stress testing and testing with "in development" hugetlbfs userfaultfd code.

Andrew, would you like a single patch that includes 4/4 of the series
and i_mmap_lock_write?  You could then throw away the previous patches
and the log would look nicer.

-- 
Mike Kravetz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web