Re: [PATCH 10/13] x86: Move cond resched for copy_{from,to}_user into low level code 64bit
>> Hmm. I can do that, but wouldn't that make CONFIG_PREEMPT_VOLUNTARY
>> mostly equivalent to CONFIG_PREEMPT_NONE?
>
> According the the Kconfig help, PREEMPT_VOLUNTARY is about the
> *explicit* preemption points. And we do have a lot of them in
> "might_sleep()".
>
> And personally, I think it makes a *lot* more sense to have a
> "might_sleep()" in the MM allocators than it does to have it in
> copy_from_user().
AFAIK, MM allocation already does that.
struct page *
__alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
struct zonelist *zonelist, nodemask_t *nodemask)
{
(snip)
might_sleep_if(gfp_mask & __GFP_WAIT);
btw, Sorry for the very late response. I haven't noticed this thread.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/13] x86: Move cond resched for copy_{from,to}_user into low level code 64bit
Hmm. I can do that, but wouldn't that make CONFIG_PREEMPT_VOLUNTARY
mostly equivalent to CONFIG_PREEMPT_NONE?
According the the Kconfig help, PREEMPT_VOLUNTARY is about the
*explicit* preemption points. And we do have a lot of them in
might_sleep().
And personally, I think it makes a *lot* more sense to have a
might_sleep() in the MM allocators than it does to have it in
copy_from_user().
AFAIK, MM allocation already does that.
struct page *
__alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
struct zonelist *zonelist, nodemask_t *nodemask)
{
(snip)
might_sleep_if(gfp_mask __GFP_WAIT);
btw, Sorry for the very late response. I haven't noticed this thread.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/13] x86: Move cond resched for copy_{from,to}_user into low level code 64bit
On Sat, Aug 10, 2013 at 08:42:58AM -0700, Linus Torvalds wrote: > On Fri, Aug 9, 2013 at 4:04 PM, Andi Kleen wrote: > > From: Andi Kleen > > > > Move the cond_resched() check for CONFIG_PREEMPT_VOLUNTARY into > > the low level copy_*_user code. This avoids some code bloat and > > makes check much more efficient by avoiding unnecessary function calls. > > May I suggest going one step further, and just removing the > cond_resched() _entirely_, leaving just the debug test? > > There really is zero reason for doing a cond_resched() for user > accesses. If they take a page fault, then yes, by all means do that > (and maybe we should add one to the page fault trap if we don't have > it already), but without a page fault they really aren't that > expensive. > > We do many more expensive things without any cond_resched(), and doing > that cond_resched() really doesn't make much sense *unless* there's a > big expensive loop involved. > > Most of this series looks fine, but I really think that we > could/should just take that extra step, and say "no, user accesses > don't imply that we need to check for scheduling". > > Linus In fact we are doing exactly this since 3.11-rc1. -- MST -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/13] x86: Move cond resched for copy_{from,to}_user into low level code 64bit
On Sat, Aug 10, 2013 at 08:42:58AM -0700, Linus Torvalds wrote: On Fri, Aug 9, 2013 at 4:04 PM, Andi Kleen a...@firstfloor.org wrote: From: Andi Kleen a...@linux.intel.com Move the cond_resched() check for CONFIG_PREEMPT_VOLUNTARY into the low level copy_*_user code. This avoids some code bloat and makes check much more efficient by avoiding unnecessary function calls. May I suggest going one step further, and just removing the cond_resched() _entirely_, leaving just the debug test? There really is zero reason for doing a cond_resched() for user accesses. If they take a page fault, then yes, by all means do that (and maybe we should add one to the page fault trap if we don't have it already), but without a page fault they really aren't that expensive. We do many more expensive things without any cond_resched(), and doing that cond_resched() really doesn't make much sense *unless* there's a big expensive loop involved. Most of this series looks fine, but I really think that we could/should just take that extra step, and say no, user accesses don't imply that we need to check for scheduling. Linus In fact we are doing exactly this since 3.11-rc1. -- MST -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/13] x86: Move cond resched for copy_{from,to}_user into low level code 64bit
On Sat, 10 August 2013 20:23:09 +0200, Borislav Petkov wrote: > > Sounds like the debug aspect and the preemption point addition need > to be sorf-of split into two different functions/macros and each used > separately. > > Something like keep the current might_sleep and have debug_sleep or > similar which does only __might_sleep without the resched... I would argue for using "might_sleep" for the debug variant. Before reading this thread I wasn't even aware of the non-debug aspect. After all, might_sleep naturally reads like some assertion. "might_preempt" for the non-debug version? "cond_preempt"? Jörn -- It is the mark of an educated mind to be able to entertain a thought without accepting it. -- Aristotle -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/13] x86: Move cond resched for copy_{from,to}_user into low level code 64bit
On Sat, Aug 10, 2013 at 09:27:33AM -0700, Linus Torvalds wrote: > Now, the *debug* logic is entirely different, of course. Maybe the > problem is that we have mixed up the two so badly, and we have > "might_sleep()" that implies more of a debug issue than a preemption > issue, and then people add those because they want the debug coverage > (and then you *absolutely* want it even for a single-byte user > mode access). And then because the concept is tied together with > preemption, we end up doing preemption even for that single-byte > access despite the fact that it makes no sense what-so-ever. Sounds like the debug aspect and the preemption point addition need to be sorf-of split into two different functions/macros and each used separately. Something like keep the current might_sleep and have debug_sleep or similar which does only __might_sleep without the resched... -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/13] x86: Move cond resched for copy_{from,to}_user into low level code 64bit
On Sat, Aug 10, 2013 at 9:10 AM, Andi Kleen wrote: > > Hmm. I can do that, but wouldn't that make CONFIG_PREEMPT_VOLUNTARY > mostly equivalent to CONFIG_PREEMPT_NONE? According the the Kconfig help, PREEMPT_VOLUNTARY is about the *explicit* preemption points. And we do have a lot of them in "might_sleep()". And personally, I think it makes a *lot* more sense to have a "might_sleep()" in the MM allocators than it does to have it in copy_from_user(). But yes, it's obviously a gray area on exactly where you'd want to put them. But for a "get_user()" that can literally be just a few instructions? Why would we make that a preemption point? Or even a copy of few tens of bytes? I'd *much* rather have preemption points in places that actually loop, and that have real work associated with them. Now, the *debug* logic is entirely different, of course. Maybe the problem is that we have mixed up the two so badly, and we have "might_sleep()" that implies more of a debug issue than a preemption issue, and then people add those because they want the debug coverage (and then you *absolutely* want it even for a single-byte user mode access). And then because the concept is tied together with preemption, we end up doing preemption even for that single-byte access despite the fact that it makes no sense what-so-ever. So I think your patches are fine, but I do think we should take a deeper look at this. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/13] x86: Move cond resched for copy_{from,to}_user into low level code 64bit
> Most of this series looks fine, but I really think that we > could/should just take that extra step, and say "no, user accesses > don't imply that we need to check for scheduling". Hmm. I can do that, but wouldn't that make CONFIG_PREEMPT_VOLUNTARY mostly equivalent to CONFIG_PREEMPT_NONE? Need to check how many other reschedule tests are left then for VOLUNTARY. -Andi -- a...@linux.intel.com -- Speaking for myself only. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/13] x86: Move cond resched for copy_{from,to}_user into low level code 64bit
On Fri, Aug 9, 2013 at 4:04 PM, Andi Kleen wrote: > From: Andi Kleen > > Move the cond_resched() check for CONFIG_PREEMPT_VOLUNTARY into > the low level copy_*_user code. This avoids some code bloat and > makes check much more efficient by avoiding unnecessary function calls. May I suggest going one step further, and just removing the cond_resched() _entirely_, leaving just the debug test? There really is zero reason for doing a cond_resched() for user accesses. If they take a page fault, then yes, by all means do that (and maybe we should add one to the page fault trap if we don't have it already), but without a page fault they really aren't that expensive. We do many more expensive things without any cond_resched(), and doing that cond_resched() really doesn't make much sense *unless* there's a big expensive loop involved. Most of this series looks fine, but I really think that we could/should just take that extra step, and say "no, user accesses don't imply that we need to check for scheduling". Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/13] x86: Move cond resched for copy_{from,to}_user into low level code 64bit
On Fri, Aug 9, 2013 at 4:04 PM, Andi Kleen a...@firstfloor.org wrote: From: Andi Kleen a...@linux.intel.com Move the cond_resched() check for CONFIG_PREEMPT_VOLUNTARY into the low level copy_*_user code. This avoids some code bloat and makes check much more efficient by avoiding unnecessary function calls. May I suggest going one step further, and just removing the cond_resched() _entirely_, leaving just the debug test? There really is zero reason for doing a cond_resched() for user accesses. If they take a page fault, then yes, by all means do that (and maybe we should add one to the page fault trap if we don't have it already), but without a page fault they really aren't that expensive. We do many more expensive things without any cond_resched(), and doing that cond_resched() really doesn't make much sense *unless* there's a big expensive loop involved. Most of this series looks fine, but I really think that we could/should just take that extra step, and say no, user accesses don't imply that we need to check for scheduling. Linus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/13] x86: Move cond resched for copy_{from,to}_user into low level code 64bit
Most of this series looks fine, but I really think that we could/should just take that extra step, and say no, user accesses don't imply that we need to check for scheduling. Hmm. I can do that, but wouldn't that make CONFIG_PREEMPT_VOLUNTARY mostly equivalent to CONFIG_PREEMPT_NONE? Need to check how many other reschedule tests are left then for VOLUNTARY. -Andi -- a...@linux.intel.com -- Speaking for myself only. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/13] x86: Move cond resched for copy_{from,to}_user into low level code 64bit
On Sat, Aug 10, 2013 at 9:10 AM, Andi Kleen a...@firstfloor.org wrote: Hmm. I can do that, but wouldn't that make CONFIG_PREEMPT_VOLUNTARY mostly equivalent to CONFIG_PREEMPT_NONE? According the the Kconfig help, PREEMPT_VOLUNTARY is about the *explicit* preemption points. And we do have a lot of them in might_sleep(). And personally, I think it makes a *lot* more sense to have a might_sleep() in the MM allocators than it does to have it in copy_from_user(). But yes, it's obviously a gray area on exactly where you'd want to put them. But for a get_user() that can literally be just a few instructions? Why would we make that a preemption point? Or even a copy of few tens of bytes? I'd *much* rather have preemption points in places that actually loop, and that have real work associated with them. Now, the *debug* logic is entirely different, of course. Maybe the problem is that we have mixed up the two so badly, and we have might_sleep() that implies more of a debug issue than a preemption issue, and then people add those because they want the debug coverage (and then you *absolutely* want it even for a single-byte user mode access). And then because the concept is tied together with preemption, we end up doing preemption even for that single-byte access despite the fact that it makes no sense what-so-ever. So I think your patches are fine, but I do think we should take a deeper look at this. Linus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/13] x86: Move cond resched for copy_{from,to}_user into low level code 64bit
On Sat, Aug 10, 2013 at 09:27:33AM -0700, Linus Torvalds wrote: Now, the *debug* logic is entirely different, of course. Maybe the problem is that we have mixed up the two so badly, and we have might_sleep() that implies more of a debug issue than a preemption issue, and then people add those because they want the debug coverage (and then you *absolutely* want it even for a single-byte user mode access). And then because the concept is tied together with preemption, we end up doing preemption even for that single-byte access despite the fact that it makes no sense what-so-ever. Sounds like the debug aspect and the preemption point addition need to be sorf-of split into two different functions/macros and each used separately. Something like keep the current might_sleep and have debug_sleep or similar which does only __might_sleep without the resched... -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/13] x86: Move cond resched for copy_{from,to}_user into low level code 64bit
On Sat, 10 August 2013 20:23:09 +0200, Borislav Petkov wrote: Sounds like the debug aspect and the preemption point addition need to be sorf-of split into two different functions/macros and each used separately. Something like keep the current might_sleep and have debug_sleep or similar which does only __might_sleep without the resched... I would argue for using might_sleep for the debug variant. Before reading this thread I wasn't even aware of the non-debug aspect. After all, might_sleep naturally reads like some assertion. might_preempt for the non-debug version? cond_preempt? Jörn -- It is the mark of an educated mind to be able to entertain a thought without accepting it. -- Aristotle -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 10/13] x86: Move cond resched for copy_{from,to}_user into low level code 64bit
From: Andi Kleen
Move the cond_resched() check for CONFIG_PREEMPT_VOLUNTARY into
the low level copy_*_user code. This avoids some code bloat and
makes check much more efficient by avoiding unnecessary function calls.
This is currently only for the non __ variants.
For the sleep debug case the call is still done in the caller.
I did not do this for copy_in_user() or the nocache variants because there's
no obvious place to put the check, and those calls are comparatively rare.
Signed-off-by: Andi Kleen
---
arch/x86/include/asm/uaccess_64.h | 4 ++--
arch/x86/lib/copy_user_64.S | 5 +++--
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/uaccess_64.h
b/arch/x86/include/asm/uaccess_64.h
index 64476bb..b327057 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -58,7 +58,7 @@ static inline unsigned long __must_check copy_from_user(void
*to,
{
int sz = __compiletime_object_size(to);
- might_fault();
+ might_fault_debug_only();
if (likely(sz == -1 || sz >= n))
n = _copy_from_user(to, from, n);
#ifdef CONFIG_DEBUG_VM
@@ -71,7 +71,7 @@ static inline unsigned long __must_check copy_from_user(void
*to,
static __always_inline __must_check
int copy_to_user(void __user *dst, const void *src, unsigned size)
{
- might_fault();
+ might_fault_debug_only();
return _copy_to_user(dst, src, size);
}
diff --git a/arch/x86/lib/copy_user_64.S b/arch/x86/lib/copy_user_64.S
index a30ca15..7039fc9 100644
--- a/arch/x86/lib/copy_user_64.S
+++ b/arch/x86/lib/copy_user_64.S
@@ -18,6 +18,7 @@
#include
#include
#include
+#include "user-common.h"
/*
* By placing feature2 after feature1 in altinstructions section, we logically
@@ -73,7 +74,7 @@
/* Standard copy_to_user with segment limit checking */
ENTRY(_copy_to_user)
CFI_STARTPROC
- GET_THREAD_INFO(%rax)
+ GET_THREAD_AND_SCHEDULE %rax
movq %rdi,%rcx
addq %rdx,%rcx
jc bad_to_user
@@ -88,7 +89,7 @@ ENDPROC(_copy_to_user)
/* Standard copy_from_user with segment limit checking */
ENTRY(_copy_from_user)
CFI_STARTPROC
- GET_THREAD_INFO(%rax)
+ GET_THREAD_AND_SCHEDULE %rax
movq %rsi,%rcx
addq %rdx,%rcx
jc bad_from_user
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 10/13] x86: Move cond resched for copy_{from,to}_user into low level code 64bit
From: Andi Kleen a...@linux.intel.com
Move the cond_resched() check for CONFIG_PREEMPT_VOLUNTARY into
the low level copy_*_user code. This avoids some code bloat and
makes check much more efficient by avoiding unnecessary function calls.
This is currently only for the non __ variants.
For the sleep debug case the call is still done in the caller.
I did not do this for copy_in_user() or the nocache variants because there's
no obvious place to put the check, and those calls are comparatively rare.
Signed-off-by: Andi Kleen a...@linux.intel.com
---
arch/x86/include/asm/uaccess_64.h | 4 ++--
arch/x86/lib/copy_user_64.S | 5 +++--
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/uaccess_64.h
b/arch/x86/include/asm/uaccess_64.h
index 64476bb..b327057 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -58,7 +58,7 @@ static inline unsigned long __must_check copy_from_user(void
*to,
{
int sz = __compiletime_object_size(to);
- might_fault();
+ might_fault_debug_only();
if (likely(sz == -1 || sz = n))
n = _copy_from_user(to, from, n);
#ifdef CONFIG_DEBUG_VM
@@ -71,7 +71,7 @@ static inline unsigned long __must_check copy_from_user(void
*to,
static __always_inline __must_check
int copy_to_user(void __user *dst, const void *src, unsigned size)
{
- might_fault();
+ might_fault_debug_only();
return _copy_to_user(dst, src, size);
}
diff --git a/arch/x86/lib/copy_user_64.S b/arch/x86/lib/copy_user_64.S
index a30ca15..7039fc9 100644
--- a/arch/x86/lib/copy_user_64.S
+++ b/arch/x86/lib/copy_user_64.S
@@ -18,6 +18,7 @@
#include asm/alternative-asm.h
#include asm/asm.h
#include asm/smap.h
+#include user-common.h
/*
* By placing feature2 after feature1 in altinstructions section, we logically
@@ -73,7 +74,7 @@
/* Standard copy_to_user with segment limit checking */
ENTRY(_copy_to_user)
CFI_STARTPROC
- GET_THREAD_INFO(%rax)
+ GET_THREAD_AND_SCHEDULE %rax
movq %rdi,%rcx
addq %rdx,%rcx
jc bad_to_user
@@ -88,7 +89,7 @@ ENDPROC(_copy_to_user)
/* Standard copy_from_user with segment limit checking */
ENTRY(_copy_from_user)
CFI_STARTPROC
- GET_THREAD_INFO(%rax)
+ GET_THREAD_AND_SCHEDULE %rax
movq %rsi,%rcx
addq %rdx,%rcx
jc bad_from_user
--
1.8.3.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
