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

Groups > linux.kernel > #1652907 > unrolled thread

Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Michal Hocko <mhocko@kernel.org> - 2017-05-30 09:50 +0200
    Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Mike Rapoport <rppt@linux.vnet.ibm.com> - 2017-05-30 12:20 +0200
      Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Michal Hocko <mhocko@kernel.org> - 2017-05-30 12:50 +0200
        Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Andrea Arcangeli <aarcange@redhat.com> - 2017-05-30 16:10 +0200
          Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Michal Hocko <mhocko@kernel.org> - 2017-05-30 16:50 +0200
            Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Michal Hocko <mhocko@kernel.org> - 2017-05-30 17:00 +0200
              Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Andrea Arcangeli <aarcange@redhat.com> - 2017-05-30 18:10 +0200
                Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Vlastimil Babka <vbabka@suse.cz> - 2017-05-31 08:40 +0200
                  Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Michal Hocko <mhocko@kernel.org> - 2017-05-31 10:30 +0200
                    Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Mike Rapoport <rppt@linux.vnet.ibm.com> - 2017-05-31 11:30 +0200
                      Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Michal Hocko <mhocko@kernel.org> - 2017-05-31 12:30 +0200
                    Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Michal Hocko <mhocko@kernel.org> - 2017-05-31 12:30 +0200
                    Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Mike Rapoport <rppt@linux.vnet.ibm.com> - 2017-06-01 13:10 +0200
                      Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Michal Hocko <mhocko@kernel.org> - 2017-06-01 14:30 +0200
            Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Andrea Arcangeli <aarcange@redhat.com> - 2017-05-30 17:50 +0200
              Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Michal Hocko <mhocko@kernel.org> - 2017-05-31 14:10 +0200
                Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Mike Rapoprt <rppt@linux.vnet.ibm.com> - 2017-05-31 14:40 +0200
                  Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Andrea Arcangeli <aarcange@redhat.com> - 2017-05-31 16:20 +0200
                    Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Michal Hocko <mhocko@kernel.org> - 2017-05-31 16:40 +0200
                      Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Andrea Arcangeli <aarcange@redhat.com> - 2017-05-31 17:50 +0200
                    Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Mike Rapoport <rppt@linux.vnet.ibm.com> - 2017-06-01 09:00 +0200
                  Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Michal Hocko <mhocko@kernel.org> - 2017-05-31 16:20 +0200
            Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Mike Rapoport <rppt@linux.vnet.ibm.com> - 2017-06-01 09:00 +0200
              Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Michal Hocko <mhocko@kernel.org> - 2017-06-01 10:10 +0200
                Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Mike Rapoport <rppt@linux.vnet.ibm.com> - 2017-06-01 10:40 +0200
                Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Andrea Arcangeli <aarcange@redhat.com> - 2017-06-01 15:50 +0200
                  Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Mike Rapoport <rppt@linux.vnet.ibm.com> - 2017-06-02 11:20 +0200
        Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Mike Rapoport <rppt@linux.vnet.ibm.com> - 2017-05-31 11:10 +0200
          Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Michal Hocko <mhocko@kernel.org> - 2017-05-31 14:10 +0200
            Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE Mike Rapoprt <rppt@linux.vnet.ibm.com> - 2017-05-31 14:30 +0200

Page 1 of 2  [1] 2  Next page →

#1652907 — Re: [PATCH] mm: introduce MADV_CLR_HUGEPAGE

On Wed 24-05-17 17:27:36, Mike Rapoport wrote:
> On Wed, May 24, 2017 at 01:18:00PM +0200, Michal Hocko wrote:
[...]
> > Why cannot khugepaged simply skip over all VMAs which have userfault
> > regions registered? This would sound like a less error prone approach to
> > me.
> 
> khugepaged does skip over VMAs which have userfault. We could register the
> regions with userfault before populating them to avoid collapses in the
> transition period.

Why cannot you register only post-copy regions and "manually" copy the
pre-copy parts?

> But then we'll have to populate these regions with
> UFFDIO_COPY which adds quite an overhead.

How big is the performance impact?
-- 
Michal Hocko
SUSE Labs

[toc] | [next] | [standalone]

#1653075

On Tue, May 30, 2017 at 09:44:08AM +0200, Michal Hocko wrote:
> On Wed 24-05-17 17:27:36, Mike Rapoport wrote:
> > On Wed, May 24, 2017 at 01:18:00PM +0200, Michal Hocko wrote:
> [...]
> > > Why cannot khugepaged simply skip over all VMAs which have userfault
> > > regions registered? This would sound like a less error prone approach to
> > > me.
> > 
> > khugepaged does skip over VMAs which have userfault. We could register the
> > regions with userfault before populating them to avoid collapses in the
> > transition period.
> 
> Why cannot you register only post-copy regions and "manually" copy the
> pre-copy parts?

We can register only post-copy regions, but this will cause VMA
fragmentation. Now we register the entire VMA with userfaultfd, no matter
how many pages were dirtied there since the pre-dump. If we register only
post-copy regions, we will split out the VMAs for those regions.
 
> > But then we'll have to populate these regions with
> > UFFDIO_COPY which adds quite an overhead.
> 
> How big is the performance impact?

I don't have the numbers handy, but for each post-copy range it means that
instead of memcpy() we will use ioctl(UFFDIO_COPY).

> -- 
> Michal Hocko
> SUSE Labs
 
--
Sincerely yours,
Mike.

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

#1653105

On Tue 30-05-17 13:19:22, Mike Rapoport wrote:
> On Tue, May 30, 2017 at 09:44:08AM +0200, Michal Hocko wrote:
> > On Wed 24-05-17 17:27:36, Mike Rapoport wrote:
> > > On Wed, May 24, 2017 at 01:18:00PM +0200, Michal Hocko wrote:
> > [...]
> > > > Why cannot khugepaged simply skip over all VMAs which have userfault
> > > > regions registered? This would sound like a less error prone approach to
> > > > me.
> > > 
> > > khugepaged does skip over VMAs which have userfault. We could register the
> > > regions with userfault before populating them to avoid collapses in the
> > > transition period.
> > 
> > Why cannot you register only post-copy regions and "manually" copy the
> > pre-copy parts?
> 
> We can register only post-copy regions, but this will cause VMA
> fragmentation. Now we register the entire VMA with userfaultfd, no matter
> how many pages were dirtied there since the pre-dump. If we register only
> post-copy regions, we will split out the VMAs for those regions.

Is this really a problem, though?

> > > But then we'll have to populate these regions with
> > > UFFDIO_COPY which adds quite an overhead.
> > 
> > How big is the performance impact?
> 
> I don't have the numbers handy, but for each post-copy range it means that
> instead of memcpy() we will use ioctl(UFFDIO_COPY).

It would be good to measure that though. You are proposing a new user
API and the THP api is quite convoluted already so there better be a
very good reason to add a new API. So far I can only see that it would
be more convinient to add another madvise command and that is rather
insufficient justification IMHO. Also do you expect somebody else would
use new madvise? What would be the usecase?
-- 
Michal Hocko
SUSE Labs

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

#1653263

On Tue, May 30, 2017 at 12:39:30PM +0200, Michal Hocko wrote:
> On Tue 30-05-17 13:19:22, Mike Rapoport wrote:
> > On Tue, May 30, 2017 at 09:44:08AM +0200, Michal Hocko wrote:
> > > On Wed 24-05-17 17:27:36, Mike Rapoport wrote:
> > > > On Wed, May 24, 2017 at 01:18:00PM +0200, Michal Hocko wrote:
> > > [...]
> > > > > Why cannot khugepaged simply skip over all VMAs which have userfault
> > > > > regions registered? This would sound like a less error prone approach to
> > > > > me.
> > > > 
> > > > khugepaged does skip over VMAs which have userfault. We could register the
> > > > regions with userfault before populating them to avoid collapses in the
> > > > transition period.
> > > 
> > > Why cannot you register only post-copy regions and "manually" copy the
> > > pre-copy parts?
> > 
> > We can register only post-copy regions, but this will cause VMA
> > fragmentation. Now we register the entire VMA with userfaultfd, no matter
> > how many pages were dirtied there since the pre-dump. If we register only
> > post-copy regions, we will split out the VMAs for those regions.
> 
> Is this really a problem, though?

It would eventually get -ENOMEM or at best create lots of unnecessary
vmas (at least UFFDIO_COPY would never risk to trigger -ENOMEM).

The only attractive alternative is to use UFFDIO_COPY for precopy too
after pre-registering the whole range in uffd (which would happen
later anyway to start postcopy).

> It would be good to measure that though. You are proposing a new user
> API and the THP api is quite convoluted already so there better be a
> very good reason to add a new API. So far I can only see that it would
> be more convinient to add another madvise command and that is rather
> insufficient justification IMHO. Also do you expect somebody else would
> use new madvise? What would be the usecase?

UFFDIO_COPY while not being a major slowdown for sure, it's likely
measurable at the microbenchmark level because it would add a
enter/exit kernel to every 4k memcpy. It's not hard to imagine that as
measurable. How that impacts the total precopy time I don't know, it
would need to be benchmarked to be sure. The main benefit of this
madvise is precisely to skip those enter/exit kernel that UFFDIO_COPY
would add. Even if the impact on the total precopy time wouldn't be
measurable (i.e. if it's network bound load), the madvise that allows
using memcpy after setting VM_NOHUGEPAGE, would free up some CPU
cycles in the destination that could be used by other processes.

About the proposed madvise, it just clear bits, but it doesn't change
at all how those bits are computed in THP code. So I don't see it as
convoluted.

If it would add new bits to be computed it would add to the
complexity. Just clearing the same bits that already exists without
altering how they're computed, doesn't move the needle in terms of
complexity. If it wasn't the case the "operational" part of the patch
wouldn't be just a one liner.

+               *vm_flags &= ~(VM_HUGEPAGE | VM_NOHUGEPAGE);

Thanks,
Andrea

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

#1653289

On Tue 30-05-17 16:04:56, Andrea Arcangeli wrote:
> On Tue, May 30, 2017 at 12:39:30PM +0200, Michal Hocko wrote:
> > On Tue 30-05-17 13:19:22, Mike Rapoport wrote:
> > > On Tue, May 30, 2017 at 09:44:08AM +0200, Michal Hocko wrote:
> > > > On Wed 24-05-17 17:27:36, Mike Rapoport wrote:
> > > > > On Wed, May 24, 2017 at 01:18:00PM +0200, Michal Hocko wrote:
> > > > [...]
> > > > > > Why cannot khugepaged simply skip over all VMAs which have userfault
> > > > > > regions registered? This would sound like a less error prone approach to
> > > > > > me.
> > > > > 
> > > > > khugepaged does skip over VMAs which have userfault. We could register the
> > > > > regions with userfault before populating them to avoid collapses in the
> > > > > transition period.
> > > > 
> > > > Why cannot you register only post-copy regions and "manually" copy the
> > > > pre-copy parts?
> > > 
> > > We can register only post-copy regions, but this will cause VMA
> > > fragmentation. Now we register the entire VMA with userfaultfd, no matter
> > > how many pages were dirtied there since the pre-dump. If we register only
> > > post-copy regions, we will split out the VMAs for those regions.
> > 
> > Is this really a problem, though?
> 
> It would eventually get -ENOMEM or at best create lots of unnecessary
> vmas (at least UFFDIO_COPY would never risk to trigger -ENOMEM).

I sysctl for the mapcount can be increased, right? I also assume that
those vmas will get merged after the post copy is done.

> The only attractive alternative is to use UFFDIO_COPY for precopy too
> after pre-registering the whole range in uffd (which would happen
> later anyway to start postcopy).
> 
> > It would be good to measure that though. You are proposing a new user
> > API and the THP api is quite convoluted already so there better be a
> > very good reason to add a new API. So far I can only see that it would
> > be more convinient to add another madvise command and that is rather
> > insufficient justification IMHO. Also do you expect somebody else would
> > use new madvise? What would be the usecase?
> 
> UFFDIO_COPY while not being a major slowdown for sure, it's likely
> measurable at the microbenchmark level because it would add a
> enter/exit kernel to every 4k memcpy. It's not hard to imagine that as
> measurable. How that impacts the total precopy time I don't know, it
> would need to be benchmarked to be sure.

Yes, please!

> The main benefit of this
> madvise is precisely to skip those enter/exit kernel that UFFDIO_COPY
> would add. Even if the impact on the total precopy time wouldn't be
> measurable (i.e. if it's network bound load), the madvise that allows
> using memcpy after setting VM_NOHUGEPAGE, would free up some CPU
> cycles in the destination that could be used by other processes.

I understand that part but it sounds awfully one purpose thing to me.
Are we going to add other MADVISE_RESET_$FOO to clear other flags just
because we can race in this specific use case?

> About the proposed madvise, it just clear bits, but it doesn't change
> at all how those bits are computed in THP code. So I don't see it as
> convoluted.

But we already have MADV_HUGEPAGE, MADV_NOHUGEPAGE and prctl to
enable/disable thp. Doesn't that sound little bit too much for a single
feature to you?
-- 
Michal Hocko
SUSE Labs

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

#1653292

On Tue 30-05-17 16:39:41, Michal Hocko wrote:
> On Tue 30-05-17 16:04:56, Andrea Arcangeli wrote:
[...]
> > About the proposed madvise, it just clear bits, but it doesn't change
> > at all how those bits are computed in THP code. So I don't see it as
> > convoluted.
> 
> But we already have MADV_HUGEPAGE, MADV_NOHUGEPAGE and prctl to
> enable/disable thp. Doesn't that sound little bit too much for a single
> feature to you?

And also I would argue that the prctl should be usable for this specific
usecase. The man page says
"
Setting this flag provides a method for disabling transparent huge pages
for jobs where the code cannot be modified
"

and that fits into the described case AFAIU. The thing that the current
implementation doesn't work is a mere detail. I would even argue that
it is non-intuitive if not buggy right away. Whoever calls this prctl
later in the process life time will simply not stop THP from creating.

So again, why cannot we fix that? There was some handwaving about
potential overhead but has anybody actually measured that?
-- 
Michal Hocko
SUSE Labs

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

#1653344

On Tue, May 30, 2017 at 04:56:33PM +0200, Michal Hocko wrote:
> On Tue 30-05-17 16:39:41, Michal Hocko wrote:
> > On Tue 30-05-17 16:04:56, Andrea Arcangeli wrote:
> [...]
> > > About the proposed madvise, it just clear bits, but it doesn't change
> > > at all how those bits are computed in THP code. So I don't see it as
> > > convoluted.
> > 
> > But we already have MADV_HUGEPAGE, MADV_NOHUGEPAGE and prctl to
> > enable/disable thp. Doesn't that sound little bit too much for a single
> > feature to you?
> 
> And also I would argue that the prctl should be usable for this specific
> usecase. The man page says
> "
> Setting this flag provides a method for disabling transparent huge pages
> for jobs where the code cannot be modified
> "
> 
> and that fits into the described case AFAIU. The thing that the current
> implementation doesn't work is a mere detail. I would even argue that
> it is non-intuitive if not buggy right away. Whoever calls this prctl
> later in the process life time will simply not stop THP from creating.
> 
> So again, why cannot we fix that? There was some handwaving about
> potential overhead but has anybody actually measured that?

I'm not sure if it should be considered a bug, the prctl is intended
to use normally by wrappers so it looks optimal as implemented this
way: affecting future vmas only, which will all be created after
execve executed by the wrapper.

What's the point of messing with the prctl so it mangles over the
wrapper process own vmas before exec? Messing with those vmas is pure
wasted CPUs for the wrapper use case which is what the prctl was
created for.

Furthermore there would be the risk a program that uses the prctl not
as a wrapper and then calls the prctl to clear VM_NOHUGEPAGE from
def_flags assuming the current kABI. The program could assume those
vmas that were instantiated before disabling the prctl are still with
VM_NOHUGEPAGE set (they would not after the change you propose).

Adding a scan of all vmas to PR_SET_THP_DISABLE to clear VM_NOHUGEPAGE
on existing vmas looks more complex too and less finegrined so
probably more complex for userland to manage, but ignoring all above
considerations it would be a functional alternative for CRIU's
needs. However if you didn't like the complexity of the new madvise
which is functionally a one-liner equivalent to MADV_NORMAL, I
wouldn't expect you to prefer to make the prctl even more complex with
a loop over all vmas that despite being fairly simple it'll still be
more than a trivial one liner.

Thanks,
Andrea

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

#1653876

On 05/30/2017 06:06 PM, Andrea Arcangeli wrote:
> 
> I'm not sure if it should be considered a bug, the prctl is intended
> to use normally by wrappers so it looks optimal as implemented this
> way: affecting future vmas only, which will all be created after
> execve executed by the wrapper.
> 
> What's the point of messing with the prctl so it mangles over the
> wrapper process own vmas before exec? Messing with those vmas is pure
> wasted CPUs for the wrapper use case which is what the prctl was
> created for.
> 
> Furthermore there would be the risk a program that uses the prctl not
> as a wrapper and then calls the prctl to clear VM_NOHUGEPAGE from
> def_flags assuming the current kABI. The program could assume those
> vmas that were instantiated before disabling the prctl are still with
> VM_NOHUGEPAGE set (they would not after the change you propose).
> 
> Adding a scan of all vmas to PR_SET_THP_DISABLE to clear VM_NOHUGEPAGE
> on existing vmas looks more complex too and less finegrined so
> probably more complex for userland to manage

I would expect the prctl wouldn't iterate all vma's, nor would it modify
def_flags anymore. It would just set a flag somewhere in mm struct that
would be considered in addition to the per-vma flags when deciding
whether to use THP. We could consider whether MADV_HUGEPAGE should be
able to override the prctl or not.

> but ignoring all above
> considerations it would be a functional alternative for CRIU's
> needs. However if you didn't like the complexity of the new madvise
> which is functionally a one-liner equivalent to MADV_NORMAL, I
> wouldn't expect you to prefer to make the prctl even more complex with
> a loop over all vmas that despite being fairly simple it'll still be
> more than a trivial one liner.
> 
> Thanks,
> Andrea
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 

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

#1653977

On Wed 31-05-17 08:30:08, Vlastimil Babka wrote:
> On 05/30/2017 06:06 PM, Andrea Arcangeli wrote:
> > 
> > I'm not sure if it should be considered a bug, the prctl is intended
> > to use normally by wrappers so it looks optimal as implemented this
> > way: affecting future vmas only, which will all be created after
> > execve executed by the wrapper.
> > 
> > What's the point of messing with the prctl so it mangles over the
> > wrapper process own vmas before exec? Messing with those vmas is pure
> > wasted CPUs for the wrapper use case which is what the prctl was
> > created for.
> > 
> > Furthermore there would be the risk a program that uses the prctl not
> > as a wrapper and then calls the prctl to clear VM_NOHUGEPAGE from
> > def_flags assuming the current kABI. The program could assume those
> > vmas that were instantiated before disabling the prctl are still with
> > VM_NOHUGEPAGE set (they would not after the change you propose).
> > 
> > Adding a scan of all vmas to PR_SET_THP_DISABLE to clear VM_NOHUGEPAGE
> > on existing vmas looks more complex too and less finegrined so
> > probably more complex for userland to manage
> 
> I would expect the prctl wouldn't iterate all vma's, nor would it modify
> def_flags anymore. It would just set a flag somewhere in mm struct that
> would be considered in addition to the per-vma flags when deciding
> whether to use THP.

Exactly. Something like the below (not even compile tested).

> We could consider whether MADV_HUGEPAGE should be
> able to override the prctl or not.

This should be a master override to any per vma setting.

---
diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index a3762d49ba39..9da053ced864 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -92,6 +92,7 @@ extern bool is_vma_temporary_stack(struct vm_area_struct *vma);
 	   (1<<TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG) &&			\
 	   ((__vma)->vm_flags & VM_HUGEPAGE))) &&			\
 	 !((__vma)->vm_flags & VM_NOHUGEPAGE) &&			\
+	 !test_bit(MMF_DISABLE_THP, &(__vma)->vm_mm->flags) &&		\
 	 !is_vma_temporary_stack(__vma))
 #define transparent_hugepage_use_zero_page()				\
 	(transparent_hugepage_flags &					\
diff --git a/include/linux/khugepaged.h b/include/linux/khugepaged.h
index 5d9a400af509..f0d7335336cd 100644
--- a/include/linux/khugepaged.h
+++ b/include/linux/khugepaged.h
@@ -48,7 +48,8 @@ static inline int khugepaged_enter(struct vm_area_struct *vma,
 	if (!test_bit(MMF_VM_HUGEPAGE, &vma->vm_mm->flags))
 		if ((khugepaged_always() ||
 		     (khugepaged_req_madv() && (vm_flags & VM_HUGEPAGE))) &&
-		    !(vm_flags & VM_NOHUGEPAGE))
+		    !(vm_flags & VM_NOHUGEPAGE) &&
+		    !test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
 			if (__khugepaged_enter(vma->vm_mm))
 				return -ENOMEM;
 	return 0;
diff --git a/include/linux/sched/coredump.h b/include/linux/sched/coredump.h
index 69eedcef8f03..2c07b244090a 100644
--- a/include/linux/sched/coredump.h
+++ b/include/linux/sched/coredump.h
@@ -68,6 +68,7 @@ static inline int get_dumpable(struct mm_struct *mm)
 #define MMF_OOM_SKIP		21	/* mm is of no interest for the OOM killer */
 #define MMF_UNSTABLE		22	/* mm is unstable for copy_from_user */
 #define MMF_HUGE_ZERO_PAGE	23      /* mm has ever used the global huge zero page */
+#define MMF_DISABLE_THP		24	/* disable THP for all VMAs */
 
 #define MMF_INIT_MASK		(MMF_DUMPABLE_MASK | MMF_DUMP_FILTER_MASK)
 
diff --git a/kernel/sys.c b/kernel/sys.c
index 8a94b4eabcaa..e48f0636c7fd 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2266,7 +2266,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 	case PR_GET_THP_DISABLE:
 		if (arg2 || arg3 || arg4 || arg5)
 			return -EINVAL;
-		error = !!(me->mm->def_flags & VM_NOHUGEPAGE);
+		error = !!test_bit(MMF_DISABLE_THP, &me->mm->flags);
 		break;
 	case PR_SET_THP_DISABLE:
 		if (arg3 || arg4 || arg5)
@@ -2274,9 +2274,9 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 		if (down_write_killable(&me->mm->mmap_sem))
 			return -EINTR;
 		if (arg2)
-			me->mm->def_flags |= VM_NOHUGEPAGE;
+			set_bit(MMF_DISABLE_THP, &me->mm->flags);
 		else
-			me->mm->def_flags &= ~VM_NOHUGEPAGE;
+			clear_bit(MMF_DISABLE_THP, &me->mm->flags);
 		up_write(&me->mm->mmap_sem);
 		break;
 	case PR_MPX_ENABLE_MANAGEMENT:
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index ce29e5cc7809..57e31f4752b3 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -818,7 +818,8 @@ khugepaged_alloc_page(struct page **hpage, gfp_t gfp, int node)
 static bool hugepage_vma_check(struct vm_area_struct *vma)
 {
 	if ((!(vma->vm_flags & VM_HUGEPAGE) && !khugepaged_always()) ||
-	    (vma->vm_flags & VM_NOHUGEPAGE))
+	    (vma->vm_flags & VM_NOHUGEPAGE) ||
+	    test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
 		return false;
 	if (shmem_file(vma->vm_file)) {
 		if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGE_PAGECACHE))
diff --git a/mm/shmem.c b/mm/shmem.c
index e67d6ba4e98e..27fe1bbf813b 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1977,10 +1977,11 @@ static int shmem_fault(struct vm_fault *vmf)
 	}
 
 	sgp = SGP_CACHE;
-	if (vma->vm_flags & VM_HUGEPAGE)
-		sgp = SGP_HUGE;
-	else if (vma->vm_flags & VM_NOHUGEPAGE)
+	
+	if ((vma->vm_flags & VM_NOHUGEPAGE) || test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
 		sgp = SGP_NOHUGE;
+	else if (vma->vm_flags & VM_HUGEPAGE)
+		sgp = SGP_HUGE;
 
 	error = shmem_getpage_gfp(inode, vmf->pgoff, &vmf->page, sgp,
 				  gfp, vma, vmf, &ret);
-- 
Michal Hocko
SUSE Labs

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

#1654044

On Wed, May 31, 2017 at 10:24:14AM +0200, Michal Hocko wrote:
> On Wed 31-05-17 08:30:08, Vlastimil Babka wrote:
> > On 05/30/2017 06:06 PM, Andrea Arcangeli wrote:
> > > 
> > > I'm not sure if it should be considered a bug, the prctl is intended
> > > to use normally by wrappers so it looks optimal as implemented this
> > > way: affecting future vmas only, which will all be created after
> > > execve executed by the wrapper.
> > > 
> > > What's the point of messing with the prctl so it mangles over the
> > > wrapper process own vmas before exec? Messing with those vmas is pure
> > > wasted CPUs for the wrapper use case which is what the prctl was
> > > created for.
> > > 
> > > Furthermore there would be the risk a program that uses the prctl not
> > > as a wrapper and then calls the prctl to clear VM_NOHUGEPAGE from
> > > def_flags assuming the current kABI. The program could assume those
> > > vmas that were instantiated before disabling the prctl are still with
> > > VM_NOHUGEPAGE set (they would not after the change you propose).
> > > 
> > > Adding a scan of all vmas to PR_SET_THP_DISABLE to clear VM_NOHUGEPAGE
> > > on existing vmas looks more complex too and less finegrined so
> > > probably more complex for userland to manage
> > 
> > I would expect the prctl wouldn't iterate all vma's, nor would it modify
> > def_flags anymore. It would just set a flag somewhere in mm struct that
> > would be considered in addition to the per-vma flags when deciding
> > whether to use THP.
> 
> Exactly. Something like the below (not even compile tested).

If we set aside the argument for keeping the kABI, this seems, hmm, a bit
more complex than new madvise() :)

It seems that for CRIU usecase such behaviour of prctl will work and it
probably will be even more convenient than madvise(). Nonetheless, I think
madvise() is the more elegant and correct solution.

> > We could consider whether MADV_HUGEPAGE should be
> > able to override the prctl or not.
> 
> This should be a master override to any per vma setting.

Currently, MADV_HUGEPAGE overrides the prctl(PR_SET_THP_DISABLE)...
AFAIU, the prctl was intended to work with applications unaware of THP and
for the cases where addition of MADV_*HUGEPAGE to the application was not
an option.
 
> ---
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index a3762d49ba39..9da053ced864 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -92,6 +92,7 @@ extern bool is_vma_temporary_stack(struct vm_area_struct *vma);
>  	   (1<<TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG) &&			\
>  	   ((__vma)->vm_flags & VM_HUGEPAGE))) &&			\
>  	 !((__vma)->vm_flags & VM_NOHUGEPAGE) &&			\
> +	 !test_bit(MMF_DISABLE_THP, &(__vma)->vm_mm->flags) &&		\
>  	 !is_vma_temporary_stack(__vma))
>  #define transparent_hugepage_use_zero_page()				\
>  	(transparent_hugepage_flags &					\
> diff --git a/include/linux/khugepaged.h b/include/linux/khugepaged.h
> index 5d9a400af509..f0d7335336cd 100644
> --- a/include/linux/khugepaged.h
> +++ b/include/linux/khugepaged.h
> @@ -48,7 +48,8 @@ static inline int khugepaged_enter(struct vm_area_struct *vma,
>  	if (!test_bit(MMF_VM_HUGEPAGE, &vma->vm_mm->flags))
>  		if ((khugepaged_always() ||
>  		     (khugepaged_req_madv() && (vm_flags & VM_HUGEPAGE))) &&
> -		    !(vm_flags & VM_NOHUGEPAGE))
> +		    !(vm_flags & VM_NOHUGEPAGE) &&
> +		    !test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
>  			if (__khugepaged_enter(vma->vm_mm))
>  				return -ENOMEM;
>  	return 0;
> diff --git a/include/linux/sched/coredump.h b/include/linux/sched/coredump.h
> index 69eedcef8f03..2c07b244090a 100644
> --- a/include/linux/sched/coredump.h
> +++ b/include/linux/sched/coredump.h
> @@ -68,6 +68,7 @@ static inline int get_dumpable(struct mm_struct *mm)
>  #define MMF_OOM_SKIP		21	/* mm is of no interest for the OOM killer */
>  #define MMF_UNSTABLE		22	/* mm is unstable for copy_from_user */
>  #define MMF_HUGE_ZERO_PAGE	23      /* mm has ever used the global huge zero page */
> +#define MMF_DISABLE_THP		24	/* disable THP for all VMAs */
> 
>  #define MMF_INIT_MASK		(MMF_DUMPABLE_MASK | MMF_DUMP_FILTER_MASK)
> 
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 8a94b4eabcaa..e48f0636c7fd 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2266,7 +2266,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>  	case PR_GET_THP_DISABLE:
>  		if (arg2 || arg3 || arg4 || arg5)
>  			return -EINVAL;
> -		error = !!(me->mm->def_flags & VM_NOHUGEPAGE);
> +		error = !!test_bit(MMF_DISABLE_THP, &me->mm->flags);
>  		break;
>  	case PR_SET_THP_DISABLE:
>  		if (arg3 || arg4 || arg5)
> @@ -2274,9 +2274,9 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>  		if (down_write_killable(&me->mm->mmap_sem))
>  			return -EINTR;
>  		if (arg2)
> -			me->mm->def_flags |= VM_NOHUGEPAGE;
> +			set_bit(MMF_DISABLE_THP, &me->mm->flags);
>  		else
> -			me->mm->def_flags &= ~VM_NOHUGEPAGE;
> +			clear_bit(MMF_DISABLE_THP, &me->mm->flags);
>  		up_write(&me->mm->mmap_sem);
>  		break;
>  	case PR_MPX_ENABLE_MANAGEMENT:
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index ce29e5cc7809..57e31f4752b3 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -818,7 +818,8 @@ khugepaged_alloc_page(struct page **hpage, gfp_t gfp, int node)
>  static bool hugepage_vma_check(struct vm_area_struct *vma)
>  {
>  	if ((!(vma->vm_flags & VM_HUGEPAGE) && !khugepaged_always()) ||
> -	    (vma->vm_flags & VM_NOHUGEPAGE))
> +	    (vma->vm_flags & VM_NOHUGEPAGE) ||
> +	    test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
>  		return false;
>  	if (shmem_file(vma->vm_file)) {
>  		if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGE_PAGECACHE))
> diff --git a/mm/shmem.c b/mm/shmem.c
> index e67d6ba4e98e..27fe1bbf813b 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1977,10 +1977,11 @@ static int shmem_fault(struct vm_fault *vmf)
>  	}
> 
>  	sgp = SGP_CACHE;
> -	if (vma->vm_flags & VM_HUGEPAGE)
> -		sgp = SGP_HUGE;
> -	else if (vma->vm_flags & VM_NOHUGEPAGE)
> +	
> +	if ((vma->vm_flags & VM_NOHUGEPAGE) || test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
>  		sgp = SGP_NOHUGE;
> +	else if (vma->vm_flags & VM_HUGEPAGE)
> +		sgp = SGP_HUGE;
> 
>  	error = shmem_getpage_gfp(inode, vmf->pgoff, &vmf->page, sgp,
>  				  gfp, vma, vmf, &ret);
> -- 
> Michal Hocko
> SUSE Labs
> 

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

#1654100

On Wed 31-05-17 12:27:00, Mike Rapoport wrote:
> On Wed, May 31, 2017 at 10:24:14AM +0200, Michal Hocko wrote:
> > On Wed 31-05-17 08:30:08, Vlastimil Babka wrote:
> > > On 05/30/2017 06:06 PM, Andrea Arcangeli wrote:
> > > > 
> > > > I'm not sure if it should be considered a bug, the prctl is intended
> > > > to use normally by wrappers so it looks optimal as implemented this
> > > > way: affecting future vmas only, which will all be created after
> > > > execve executed by the wrapper.
> > > > 
> > > > What's the point of messing with the prctl so it mangles over the
> > > > wrapper process own vmas before exec? Messing with those vmas is pure
> > > > wasted CPUs for the wrapper use case which is what the prctl was
> > > > created for.
> > > > 
> > > > Furthermore there would be the risk a program that uses the prctl not
> > > > as a wrapper and then calls the prctl to clear VM_NOHUGEPAGE from
> > > > def_flags assuming the current kABI. The program could assume those
> > > > vmas that were instantiated before disabling the prctl are still with
> > > > VM_NOHUGEPAGE set (they would not after the change you propose).
> > > > 
> > > > Adding a scan of all vmas to PR_SET_THP_DISABLE to clear VM_NOHUGEPAGE
> > > > on existing vmas looks more complex too and less finegrined so
> > > > probably more complex for userland to manage
> > > 
> > > I would expect the prctl wouldn't iterate all vma's, nor would it modify
> > > def_flags anymore. It would just set a flag somewhere in mm struct that
> > > would be considered in addition to the per-vma flags when deciding
> > > whether to use THP.
> > 
> > Exactly. Something like the below (not even compile tested).
> 
> If we set aside the argument for keeping the kABI, this seems, hmm, a bit
> more complex than new madvise() :)

Yes, code wise it is more LOC which is not all that great but semantic
wise it make much more sense than the current implementation of
PR_SET_THP_DISABLE.

> It seems that for CRIU usecase such behaviour of prctl will work and it
> probably will be even more convenient than madvise(). Nonetheless, I think
> madvise() is the more elegant and correct solution.
> 
> > > We could consider whether MADV_HUGEPAGE should be
> > > able to override the prctl or not.
> > 
> > This should be a master override to any per vma setting.
> 
> Currently, MADV_HUGEPAGE overrides the prctl(PR_SET_THP_DISABLE)...
> AFAIU, the prctl was intended to work with applications unaware of THP and
> for the cases where addition of MADV_*HUGEPAGE to the application was not
> an option.

which makes it even more weird API IMHO.
-- 
Michal Hocko
SUSE Labs

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

#1654103

On Wed 31-05-17 10:24:14, Michal Hocko wrote:
[...]

JFTR we also need to update MMF_INIT_MASK as well.
+#define MMF_INIT_MASK          (MMF_DUMPABLE_MASK | MMF_DUMP_FILTER_MASK | MMF_DISABLE_THP)

> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index a3762d49ba39..9da053ced864 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -92,6 +92,7 @@ extern bool is_vma_temporary_stack(struct vm_area_struct *vma);
>  	   (1<<TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG) &&			\
>  	   ((__vma)->vm_flags & VM_HUGEPAGE))) &&			\
>  	 !((__vma)->vm_flags & VM_NOHUGEPAGE) &&			\
> +	 !test_bit(MMF_DISABLE_THP, &(__vma)->vm_mm->flags) &&		\
>  	 !is_vma_temporary_stack(__vma))
>  #define transparent_hugepage_use_zero_page()				\
>  	(transparent_hugepage_flags &					\
> diff --git a/include/linux/khugepaged.h b/include/linux/khugepaged.h
> index 5d9a400af509..f0d7335336cd 100644
> --- a/include/linux/khugepaged.h
> +++ b/include/linux/khugepaged.h
> @@ -48,7 +48,8 @@ static inline int khugepaged_enter(struct vm_area_struct *vma,
>  	if (!test_bit(MMF_VM_HUGEPAGE, &vma->vm_mm->flags))
>  		if ((khugepaged_always() ||
>  		     (khugepaged_req_madv() && (vm_flags & VM_HUGEPAGE))) &&
> -		    !(vm_flags & VM_NOHUGEPAGE))
> +		    !(vm_flags & VM_NOHUGEPAGE) &&
> +		    !test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
>  			if (__khugepaged_enter(vma->vm_mm))
>  				return -ENOMEM;
>  	return 0;
> diff --git a/include/linux/sched/coredump.h b/include/linux/sched/coredump.h
> index 69eedcef8f03..2c07b244090a 100644
> --- a/include/linux/sched/coredump.h
> +++ b/include/linux/sched/coredump.h
> @@ -68,6 +68,7 @@ static inline int get_dumpable(struct mm_struct *mm)
>  #define MMF_OOM_SKIP		21	/* mm is of no interest for the OOM killer */
>  #define MMF_UNSTABLE		22	/* mm is unstable for copy_from_user */
>  #define MMF_HUGE_ZERO_PAGE	23      /* mm has ever used the global huge zero page */
> +#define MMF_DISABLE_THP		24	/* disable THP for all VMAs */
>  
>  #define MMF_INIT_MASK		(MMF_DUMPABLE_MASK | MMF_DUMP_FILTER_MASK)
>  
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 8a94b4eabcaa..e48f0636c7fd 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2266,7 +2266,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>  	case PR_GET_THP_DISABLE:
>  		if (arg2 || arg3 || arg4 || arg5)
>  			return -EINVAL;
> -		error = !!(me->mm->def_flags & VM_NOHUGEPAGE);
> +		error = !!test_bit(MMF_DISABLE_THP, &me->mm->flags);
>  		break;
>  	case PR_SET_THP_DISABLE:
>  		if (arg3 || arg4 || arg5)
> @@ -2274,9 +2274,9 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>  		if (down_write_killable(&me->mm->mmap_sem))
>  			return -EINTR;
>  		if (arg2)
> -			me->mm->def_flags |= VM_NOHUGEPAGE;
> +			set_bit(MMF_DISABLE_THP, &me->mm->flags);
>  		else
> -			me->mm->def_flags &= ~VM_NOHUGEPAGE;
> +			clear_bit(MMF_DISABLE_THP, &me->mm->flags);
>  		up_write(&me->mm->mmap_sem);
>  		break;
>  	case PR_MPX_ENABLE_MANAGEMENT:
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index ce29e5cc7809..57e31f4752b3 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -818,7 +818,8 @@ khugepaged_alloc_page(struct page **hpage, gfp_t gfp, int node)
>  static bool hugepage_vma_check(struct vm_area_struct *vma)
>  {
>  	if ((!(vma->vm_flags & VM_HUGEPAGE) && !khugepaged_always()) ||
> -	    (vma->vm_flags & VM_NOHUGEPAGE))
> +	    (vma->vm_flags & VM_NOHUGEPAGE) ||
> +	    test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
>  		return false;
>  	if (shmem_file(vma->vm_file)) {
>  		if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGE_PAGECACHE))
> diff --git a/mm/shmem.c b/mm/shmem.c
> index e67d6ba4e98e..27fe1bbf813b 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1977,10 +1977,11 @@ static int shmem_fault(struct vm_fault *vmf)
>  	}
>  
>  	sgp = SGP_CACHE;
> -	if (vma->vm_flags & VM_HUGEPAGE)
> -		sgp = SGP_HUGE;
> -	else if (vma->vm_flags & VM_NOHUGEPAGE)
> +	
> +	if ((vma->vm_flags & VM_NOHUGEPAGE) || test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
>  		sgp = SGP_NOHUGE;
> +	else if (vma->vm_flags & VM_HUGEPAGE)
> +		sgp = SGP_HUGE;
>  
>  	error = shmem_getpage_gfp(inode, vmf->pgoff, &vmf->page, sgp,
>  				  gfp, vma, vmf, &ret);
> -- 
> Michal Hocko
> SUSE Labs

-- 
Michal Hocko
SUSE Labs

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

#1655023

On Wed, May 31, 2017 at 10:24:14AM +0200, Michal Hocko wrote:
> On Wed 31-05-17 08:30:08, Vlastimil Babka wrote:
> > On 05/30/2017 06:06 PM, Andrea Arcangeli wrote:
> > > 
> > > I'm not sure if it should be considered a bug, the prctl is intended
> > > to use normally by wrappers so it looks optimal as implemented this
> > > way: affecting future vmas only, which will all be created after
> > > execve executed by the wrapper.
> > > 
> > > What's the point of messing with the prctl so it mangles over the
> > > wrapper process own vmas before exec? Messing with those vmas is pure
> > > wasted CPUs for the wrapper use case which is what the prctl was
> > > created for.
> > > 
> > > Furthermore there would be the risk a program that uses the prctl not
> > > as a wrapper and then calls the prctl to clear VM_NOHUGEPAGE from
> > > def_flags assuming the current kABI. The program could assume those
> > > vmas that were instantiated before disabling the prctl are still with
> > > VM_NOHUGEPAGE set (they would not after the change you propose).
> > > 
> > > Adding a scan of all vmas to PR_SET_THP_DISABLE to clear VM_NOHUGEPAGE
> > > on existing vmas looks more complex too and less finegrined so
> > > probably more complex for userland to manage
> > 
> > I would expect the prctl wouldn't iterate all vma's, nor would it modify
> > def_flags anymore. It would just set a flag somewhere in mm struct that
> > would be considered in addition to the per-vma flags when deciding
> > whether to use THP.
> 
> Exactly. Something like the below (not even compile tested).
 
I did a quick go with the patch, compiles just fine :)
It worked for my simple examples, the THP is enabled/disabled as expected
and the vma->vm_flags are indeed unaffected.

> > We could consider whether MADV_HUGEPAGE should be
> > able to override the prctl or not.
> 
> This should be a master override to any per vma setting.

Here you've introduced a change to the current behaviour. Consider the
following sequence:

{
	prctl(PR_SET_THP_DISABLE);
	address = mmap(...);
	madvise(address, len, MADV_HUGEPAGE);
}

Currently, for the vma that backs the address
transparent_hugepage_enabled(vma) will return true, and after your patch it
will return false.
The new behaviour may be more correct, I just wanted to bring the change to
attention. 
 
> ---
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index a3762d49ba39..9da053ced864 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -92,6 +92,7 @@ extern bool is_vma_temporary_stack(struct vm_area_struct *vma);
>  	   (1<<TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG) &&			\
>  	   ((__vma)->vm_flags & VM_HUGEPAGE))) &&			\
>  	 !((__vma)->vm_flags & VM_NOHUGEPAGE) &&			\
> +	 !test_bit(MMF_DISABLE_THP, &(__vma)->vm_mm->flags) &&		\
>  	 !is_vma_temporary_stack(__vma))
>  #define transparent_hugepage_use_zero_page()				\
>  	(transparent_hugepage_flags &					\
> diff --git a/include/linux/khugepaged.h b/include/linux/khugepaged.h
> index 5d9a400af509..f0d7335336cd 100644
> --- a/include/linux/khugepaged.h
> +++ b/include/linux/khugepaged.h
> @@ -48,7 +48,8 @@ static inline int khugepaged_enter(struct vm_area_struct *vma,
>  	if (!test_bit(MMF_VM_HUGEPAGE, &vma->vm_mm->flags))
>  		if ((khugepaged_always() ||
>  		     (khugepaged_req_madv() && (vm_flags & VM_HUGEPAGE))) &&
> -		    !(vm_flags & VM_NOHUGEPAGE))
> +		    !(vm_flags & VM_NOHUGEPAGE) &&
> +		    !test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
>  			if (__khugepaged_enter(vma->vm_mm))
>  				return -ENOMEM;
>  	return 0;
> diff --git a/include/linux/sched/coredump.h b/include/linux/sched/coredump.h
> index 69eedcef8f03..2c07b244090a 100644
> --- a/include/linux/sched/coredump.h
> +++ b/include/linux/sched/coredump.h
> @@ -68,6 +68,7 @@ static inline int get_dumpable(struct mm_struct *mm)
>  #define MMF_OOM_SKIP		21	/* mm is of no interest for the OOM killer */
>  #define MMF_UNSTABLE		22	/* mm is unstable for copy_from_user */
>  #define MMF_HUGE_ZERO_PAGE	23      /* mm has ever used the global huge zero page */
> +#define MMF_DISABLE_THP		24	/* disable THP for all VMAs */
> 
>  #define MMF_INIT_MASK		(MMF_DUMPABLE_MASK | MMF_DUMP_FILTER_MASK)
> 
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 8a94b4eabcaa..e48f0636c7fd 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2266,7 +2266,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>  	case PR_GET_THP_DISABLE:
>  		if (arg2 || arg3 || arg4 || arg5)
>  			return -EINVAL;
> -		error = !!(me->mm->def_flags & VM_NOHUGEPAGE);
> +		error = !!test_bit(MMF_DISABLE_THP, &me->mm->flags);
>  		break;
>  	case PR_SET_THP_DISABLE:
>  		if (arg3 || arg4 || arg5)
> @@ -2274,9 +2274,9 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>  		if (down_write_killable(&me->mm->mmap_sem))
>  			return -EINTR;
>  		if (arg2)
> -			me->mm->def_flags |= VM_NOHUGEPAGE;
> +			set_bit(MMF_DISABLE_THP, &me->mm->flags);
>  		else
> -			me->mm->def_flags &= ~VM_NOHUGEPAGE;
> +			clear_bit(MMF_DISABLE_THP, &me->mm->flags);
>  		up_write(&me->mm->mmap_sem);
>  		break;
>  	case PR_MPX_ENABLE_MANAGEMENT:
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index ce29e5cc7809..57e31f4752b3 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -818,7 +818,8 @@ khugepaged_alloc_page(struct page **hpage, gfp_t gfp, int node)
>  static bool hugepage_vma_check(struct vm_area_struct *vma)
>  {
>  	if ((!(vma->vm_flags & VM_HUGEPAGE) && !khugepaged_always()) ||
> -	    (vma->vm_flags & VM_NOHUGEPAGE))
> +	    (vma->vm_flags & VM_NOHUGEPAGE) ||
> +	    test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
>  		return false;
>  	if (shmem_file(vma->vm_file)) {
>  		if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGE_PAGECACHE))
> diff --git a/mm/shmem.c b/mm/shmem.c
> index e67d6ba4e98e..27fe1bbf813b 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1977,10 +1977,11 @@ static int shmem_fault(struct vm_fault *vmf)
>  	}
> 
>  	sgp = SGP_CACHE;
> -	if (vma->vm_flags & VM_HUGEPAGE)
> -		sgp = SGP_HUGE;
> -	else if (vma->vm_flags & VM_NOHUGEPAGE)
> +	
> +	if ((vma->vm_flags & VM_NOHUGEPAGE) || test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
>  		sgp = SGP_NOHUGE;
> +	else if (vma->vm_flags & VM_HUGEPAGE)
> +		sgp = SGP_HUGE;
> 
>  	error = shmem_getpage_gfp(inode, vmf->pgoff, &vmf->page, sgp,
>  				  gfp, vma, vmf, &ret);
> -- 
> Michal Hocko
> SUSE Labs
> 

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

#1655090

On Thu 01-06-17 14:00:48, Mike Rapoport wrote:
> On Wed, May 31, 2017 at 10:24:14AM +0200, Michal Hocko wrote:
> > On Wed 31-05-17 08:30:08, Vlastimil Babka wrote:
> > > On 05/30/2017 06:06 PM, Andrea Arcangeli wrote:
> > > > 
> > > > I'm not sure if it should be considered a bug, the prctl is intended
> > > > to use normally by wrappers so it looks optimal as implemented this
> > > > way: affecting future vmas only, which will all be created after
> > > > execve executed by the wrapper.
> > > > 
> > > > What's the point of messing with the prctl so it mangles over the
> > > > wrapper process own vmas before exec? Messing with those vmas is pure
> > > > wasted CPUs for the wrapper use case which is what the prctl was
> > > > created for.
> > > > 
> > > > Furthermore there would be the risk a program that uses the prctl not
> > > > as a wrapper and then calls the prctl to clear VM_NOHUGEPAGE from
> > > > def_flags assuming the current kABI. The program could assume those
> > > > vmas that were instantiated before disabling the prctl are still with
> > > > VM_NOHUGEPAGE set (they would not after the change you propose).
> > > > 
> > > > Adding a scan of all vmas to PR_SET_THP_DISABLE to clear VM_NOHUGEPAGE
> > > > on existing vmas looks more complex too and less finegrined so
> > > > probably more complex for userland to manage
> > > 
> > > I would expect the prctl wouldn't iterate all vma's, nor would it modify
> > > def_flags anymore. It would just set a flag somewhere in mm struct that
> > > would be considered in addition to the per-vma flags when deciding
> > > whether to use THP.
> > 
> > Exactly. Something like the below (not even compile tested).
>  
> I did a quick go with the patch, compiles just fine :)
> It worked for my simple examples, the THP is enabled/disabled as expected
> and the vma->vm_flags are indeed unaffected.
> 
> > > We could consider whether MADV_HUGEPAGE should be
> > > able to override the prctl or not.
> > 
> > This should be a master override to any per vma setting.
> 
> Here you've introduced a change to the current behaviour. Consider the
> following sequence:
> 
> {
> 	prctl(PR_SET_THP_DISABLE);
> 	address = mmap(...);
> 	madvise(address, len, MADV_HUGEPAGE);
> }
>
> Currently, for the vma that backs the address
> transparent_hugepage_enabled(vma) will return true, and after your patch it
> will return false.
> The new behaviour may be more correct, I just wanted to bring the change to
> attention. 

The system wide disable should override any VMA specific setting
IMHO. Why would we disable the THP for the whole process otherwise?
Anyway this needs to be discussed at linux-api mailing list. I will try
to make my change into a proper patch and post it there.
-- 
Michal Hocko
SUSE Labs

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

#1653329

On Tue, May 30, 2017 at 04:39:41PM +0200, Michal Hocko wrote:
> I sysctl for the mapcount can be increased, right? I also assume that
> those vmas will get merged after the post copy is done.

Assuming you enlarge the sysctl to the worst possible case, with 64bit
address space you can have billions of VMAs if you're migrating 4T of
RAM and you're unlucky and the address space gets fragmented. The
unswappable kernel memory overhead would be relatively large
(i.e. dozen gigabytes of RAM in vm_area_struct slab), and each
find_vma operation would need to walk ~40 steps across that large vma
rbtree. There's a reason the sysctl exist. Not to tell all those
unnecessary vma mangling operations would be protected by the mmap_sem
for writing.

Not creating a ton of vmas and enabling vma-less pte mangling with a
single large vma and only using mmap_sem for reading during all the
pte mangling, is one of the primary design motivations for
userfaultfd.

> I understand that part but it sounds awfully one purpose thing to me.
> Are we going to add other MADVISE_RESET_$FOO to clear other flags just
> because we can race in this specific use case?

Those already exists, see for example MADV_NORMAL, clearing
~VM_RAND_READ & ~VM_SEQ_READ after calling MADV_SEQUENTIAL or
MADV_RANDOM.

Or MADV_DOFORK after MADV_DONTFORK. MADV_DONTDUMP after MADV_DODUMP. Etc..

> But we already have MADV_HUGEPAGE, MADV_NOHUGEPAGE and prctl to
> enable/disable thp. Doesn't that sound little bit too much for a single
> feature to you?

MADV_NOHUGEPAGE doesn't mean clearing the flag set with
MADV_HUGEPAGE. MADV_NOHUGEPAGE disables THP on the region if the
global sysfs "enabled" tune is set to "always". MADV_HUGEPAGE enables
THP if the global "enabled" sysfs tune is set to "madvise". The two
MADV_NOHUGEPAGE and MADV_HUGEPAGE are needed to leverage the three-way
setting of "never" "madvise" "always" of the global tune.

The "madvise" global tune exists if you want to save RAM and you don't
care much about performance but still allowing apps like QEMU where no
memory is lost by enabling THP, to use THP.

There's no way to clear either of those two flags and bring back the
default behavior of the global sysfs tune, so it's not redundant at
the very least.

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

#1654171

On Tue 30-05-17 17:43:26, Andrea Arcangeli wrote:
> On Tue, May 30, 2017 at 04:39:41PM +0200, Michal Hocko wrote:
> > I sysctl for the mapcount can be increased, right? I also assume that
> > those vmas will get merged after the post copy is done.
> 
> Assuming you enlarge the sysctl to the worst possible case, with 64bit
> address space you can have billions of VMAs if you're migrating 4T of
> RAM and you're unlucky and the address space gets fragmented. The
> unswappable kernel memory overhead would be relatively large
> (i.e. dozen gigabytes of RAM in vm_area_struct slab), and each
> find_vma operation would need to walk ~40 steps across that large vma
> rbtree. There's a reason the sysctl exist. Not to tell all those
> unnecessary vma mangling operations would be protected by the mmap_sem
> for writing.
> 
> Not creating a ton of vmas and enabling vma-less pte mangling with a
> single large vma and only using mmap_sem for reading during all the
> pte mangling, is one of the primary design motivations for
> userfaultfd.

Yes, I am aware of fallouts of too many vmas. I was asking merely to
learn whether this will really happen under the the specific usecase
Mike is after.

> > I understand that part but it sounds awfully one purpose thing to me.
> > Are we going to add other MADVISE_RESET_$FOO to clear other flags just
> > because we can race in this specific use case?
> 
> Those already exists, see for example MADV_NORMAL, clearing
> ~VM_RAND_READ & ~VM_SEQ_READ after calling MADV_SEQUENTIAL or
> MADV_RANDOM.

I would argue that MADV_NORMAL is everything but a clear madvise
command. Why doesn't it clear all the sticky MADV* flags?

> Or MADV_DOFORK after MADV_DONTFORK. MADV_DONTDUMP after MADV_DODUMP. Etc..
>
> > But we already have MADV_HUGEPAGE, MADV_NOHUGEPAGE and prctl to
> > enable/disable thp. Doesn't that sound little bit too much for a single
> > feature to you?
> 
> MADV_NOHUGEPAGE doesn't mean clearing the flag set with
> MADV_HUGEPAGE. MADV_NOHUGEPAGE disables THP on the region if the
> global sysfs "enabled" tune is set to "always". MADV_HUGEPAGE enables
> THP if the global "enabled" sysfs tune is set to "madvise". The two
> MADV_NOHUGEPAGE and MADV_HUGEPAGE are needed to leverage the three-way
> setting of "never" "madvise" "always" of the global tune.
> 
> The "madvise" global tune exists if you want to save RAM and you don't
> care much about performance but still allowing apps like QEMU where no
> memory is lost by enabling THP, to use THP.
> 
> There's no way to clear either of those two flags and bring back the
> default behavior of the global sysfs tune, so it's not redundant at
> the very least.

Yes I am not a huge fan of the current MADV*HUGEPAGE semantic but I
would really like to see a strong usecase for adding another command on
top. From what Mike said a global disable THP for the whole process
while the post-copy is in progress is a better solution anyway.
-- 
Michal Hocko
SUSE Labs

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

#1654183

On May 31, 2017 3:08:22 PM GMT+03:00, Michal Hocko <mhocko@kernel.org> wrote:
>On Tue 30-05-17 17:43:26, Andrea Arcangeli wrote:
>> On Tue, May 30, 2017 at 04:39:41PM +0200, Michal Hocko wrote:
>> > I sysctl for the mapcount can be increased, right? I also assume
>that
>> > those vmas will get merged after the post copy is done.
>> 
>> Assuming you enlarge the sysctl to the worst possible case, with
>64bit
>> address space you can have billions of VMAs if you're migrating 4T of
>> RAM and you're unlucky and the address space gets fragmented. The
>> unswappable kernel memory overhead would be relatively large
>> (i.e. dozen gigabytes of RAM in vm_area_struct slab), and each
>> find_vma operation would need to walk ~40 steps across that large vma
>> rbtree. There's a reason the sysctl exist. Not to tell all those
>> unnecessary vma mangling operations would be protected by the
>mmap_sem
>> for writing.
>> 
>> Not creating a ton of vmas and enabling vma-less pte mangling with a
>> single large vma and only using mmap_sem for reading during all the
>> pte mangling, is one of the primary design motivations for
>> userfaultfd.
>
>Yes, I am aware of fallouts of too many vmas. I was asking merely to
>learn whether this will really happen under the the specific usecase
>Mike is after.

That depends on the application access pattern in the period between the pre-dump is finished and the application is frozen. If the accesses are random enough, the dirty pages that would be post copied could get spread all over the address space.

>> > I understand that part but it sounds awfully one purpose thing to
>me.
>> > Are we going to add other MADVISE_RESET_$FOO to clear other flags
>just
>> > because we can race in this specific use case?
>> 
>> Those already exists, see for example MADV_NORMAL, clearing
>> ~VM_RAND_READ & ~VM_SEQ_READ after calling MADV_SEQUENTIAL or
>> MADV_RANDOM.
>
>I would argue that MADV_NORMAL is everything but a clear madvise
>command. Why doesn't it clear all the sticky MADV* flags?

That would be helpful :)
Still, the problem here is more with the naming that with the action. If it was called MADV_DEFAULT_READ or something, it would be fine, wouldn't it?

>> Or MADV_DOFORK after MADV_DONTFORK. MADV_DONTDUMP after MADV_DODUMP.
>Etc..
>>
>> > But we already have MADV_HUGEPAGE, MADV_NOHUGEPAGE and prctl to
>> > enable/disable thp. Doesn't that sound little bit too much for a
>single
>> > feature to you?
>> 
>> MADV_NOHUGEPAGE doesn't mean clearing the flag set with
>> MADV_HUGEPAGE. MADV_NOHUGEPAGE disables THP on the region if the
>> global sysfs "enabled" tune is set to "always". MADV_HUGEPAGE enables
>> THP if the global "enabled" sysfs tune is set to "madvise". The two
>> MADV_NOHUGEPAGE and MADV_HUGEPAGE are needed to leverage the
>three-way
>> setting of "never" "madvise" "always" of the global tune.
>> 
>> The "madvise" global tune exists if you want to save RAM and you
>don't
>> care much about performance but still allowing apps like QEMU where
>no
>> memory is lost by enabling THP, to use THP.
>> 
>> There's no way to clear either of those two flags and bring back the
>> default behavior of the global sysfs tune, so it's not redundant at
>> the very least.
>
>Yes I am not a huge fan of the current MADV*HUGEPAGE semantic but I
>would really like to see a strong usecase for adding another command on
>top. 

Well, another command makes the semantic a bit better, IMHO...

> From what Mike said a global disable THP for the whole process
>while the post-copy is in progress is a better solution anyway.

For the CRIU usecase, disabling THP for a while and re-enabling it back will do the trick, provided VMAs flags are not affected, like in the patch you've sent.
Moreover, we may even get away with ioctl(UFFDIO_COPY) if it's overhead shows to be negligible​.
Still, I believe that MADV_RESET_HUGEPAGE (or some better named) command has the value on its own.
--
Sincerely yours,
Mike.

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

#1654298

On Wed, May 31, 2017 at 03:39:22PM +0300, Mike Rapoport wrote:
> For the CRIU usecase, disabling THP for a while and re-enabling it
> back will do the trick, provided VMAs flags are not affected, like
> in the patch you've sent.  Moreover, we may even get away with

Are you going to check uname -r to know when the kABI changed in your
favor (so CRIU cannot ever work with enterprise backports unless you
expand the uname -r coverage), or how do you know the patch is
applied?

Optimistically assuming people is going to run new CRIU code only on
new kernels looks very risky, it would leads to silent random memory
corruption, so I doubt you can get away without a uname -r check.

This is fairly simple change too, its main cons is that it adds a
branch to the page fault fast path, the old behavior of the prctl and
the new madvise were both zero cost.

Still if the prctl is preferred despite the added branch, to avoid
uname -r clashes, to me it sounds better to add a new prctl ID and
keep the old one too. The old one could be implemented the same way as
the new one if you want to save a few bytes of .text. But the old one
should probably do a printk_once to print a deprecation warning so the
old ID with weaker (zero runtime cost) semantics can be removed later.

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

#1654317

On Wed 31-05-17 16:18:09, Andrea Arcangeli wrote:
> On Wed, May 31, 2017 at 03:39:22PM +0300, Mike Rapoport wrote:
> > For the CRIU usecase, disabling THP for a while and re-enabling it
> > back will do the trick, provided VMAs flags are not affected, like
> > in the patch you've sent.  Moreover, we may even get away with
> 
> Are you going to check uname -r to know when the kABI changed in your
> favor (so CRIU cannot ever work with enterprise backports unless you
> expand the uname -r coverage), or how do you know the patch is
> applied?

I would assume such a patch would be backported to stable trees because
to me it sounds like the current semantic is simply broken and needs
fixing anyway but it shouldn't be much different from any other bugs.

This is far from ideal from the "guarantee POV" of course.

> Optimistically assuming people is going to run new CRIU code only on
> new kernels looks very risky, it would leads to silent random memory
> corruption, so I doubt you can get away without a uname -r check.
> 
> This is fairly simple change too, its main cons is that it adds a
> branch to the page fault fast path, the old behavior of the prctl and
> the new madvise were both zero cost.
> 
> Still if the prctl is preferred despite the added branch, to avoid
> uname -r clashes, to me it sounds better to add a new prctl ID and
> keep the old one too. The old one could be implemented the same way as
> the new one if you want to save a few bytes of .text. But the old one
> should probably do a printk_once to print a deprecation warning so the
> old ID with weaker (zero runtime cost) semantics can be removed later.

this would be an option as well although it adds to the mess...

-- 
Michal Hocko
SUSE Labs

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

#1654370

On Wed, May 31, 2017 at 04:32:17PM +0200, Michal Hocko wrote:
> I would assume such a patch would be backported to stable trees because
> to me it sounds like the current semantic is simply broken and needs
> fixing anyway but it shouldn't be much different from any other bugs.

So the program would need then to check also for the -stable minor
number where the patch was backported to in addition of any enterprise
kernel backport versioning.

> This is far from ideal from the "guarantee POV" of course.

Agree it's far from ideal and lack of guarantee at least for CRIU
means silent random memory corruption.

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web