Re: [PATCH 3/3] smp/ipi:Remove check around csd lock in handler for smp_call_function variants
On Sun, Jul 07, 2013 at 09:53:48PM +0530, Preeti U Murthy wrote: > > " > > /* > > > > * Unlocked CSDs are valid through generic_exec_single(): > > > > */ > > I don't understand this comment. All callers of generic_exec_single() > take the csd lock. So where is the scenario of csds being unlocked in > generic_exec_single() before the call to > arch_send_call_function_single_ipi() is made? > Rather what is the above comment trying to say? I have given the answer to this question in last reply. I don't know whether it is right to make a assumption through this way that what you do currently: Find all the current api users, and drop all the robust codes, despite the unpredictable future users. Ok, I know the balance between "robust" vs "performance", robust check codes will bring performance penalty in fastest code path, but the "penalty" is neglectable sometimes for modern CPU. I decide to respect the MAINTAINER's decision to accept this change or not. Thanks. -- 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 3/3] smp/ipi:Remove check around csd lock in handler for smp_call_function variants
Hi Wang, On 07/06/2013 07:51 PM, Wang YanQing wrote: > On Sat, Jul 06, 2013 at 01:36:27PM +0530, Preeti U Murthy wrote: >> Ideally it should be under a WARN_ON(). csd_unlock() has that WARN_ON(). >> Unlocking a parameter which is not locked should be seen as a bug, which >> the above code is not doing. In fact it avoids it being reported as a bug. > > Although I know what's your meaning, but just like the comment in code: > > " > /* > > * Unlocked CSDs are valid through generic_exec_single(): > > */ I don't understand this comment. All callers of generic_exec_single() take the csd lock. So where is the scenario of csds being unlocked in generic_exec_single() before the call to arch_send_call_function_single_ipi() is made? Rather what is the above comment trying to say? > " > > If the csd don't come from generic_exec_single, then > Unlocked CSDs maybe are not valid. So we check CSD_FLAG_LOCK > to avoid trigger the WARN_ON in csd_unlock. > > Genric_exec_single's name imply it is a generic version, > you know, maybe we will have "special" version. > > Thanks. > Regards Preeti U Murthy -- 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 3/3] smp/ipi:Remove check around csd lock in handler for smp_call_function variants
Hi Wang, On 07/06/2013 07:51 PM, Wang YanQing wrote: On Sat, Jul 06, 2013 at 01:36:27PM +0530, Preeti U Murthy wrote: Ideally it should be under a WARN_ON(). csd_unlock() has that WARN_ON(). Unlocking a parameter which is not locked should be seen as a bug, which the above code is not doing. In fact it avoids it being reported as a bug. Although I know what's your meaning, but just like the comment in code: /* * Unlocked CSDs are valid through generic_exec_single(): */ I don't understand this comment. All callers of generic_exec_single() take the csd lock. So where is the scenario of csds being unlocked in generic_exec_single() before the call to arch_send_call_function_single_ipi() is made? Rather what is the above comment trying to say? If the csd don't come from generic_exec_single, then Unlocked CSDs maybe are not valid. So we check CSD_FLAG_LOCK to avoid trigger the WARN_ON in csd_unlock. Genric_exec_single's name imply it is a generic version, you know, maybe we will have special version. Thanks. Regards Preeti U Murthy -- 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 3/3] smp/ipi:Remove check around csd lock in handler for smp_call_function variants
On Sun, Jul 07, 2013 at 09:53:48PM +0530, Preeti U Murthy wrote: /* * Unlocked CSDs are valid through generic_exec_single(): */ I don't understand this comment. All callers of generic_exec_single() take the csd lock. So where is the scenario of csds being unlocked in generic_exec_single() before the call to arch_send_call_function_single_ipi() is made? Rather what is the above comment trying to say? I have given the answer to this question in last reply. I don't know whether it is right to make a assumption through this way that what you do currently: Find all the current api users, and drop all the robust codes, despite the unpredictable future users. Ok, I know the balance between robust vs performance, robust check codes will bring performance penalty in fastest code path, but the penalty is neglectable sometimes for modern CPU. I decide to respect the MAINTAINER's decision to accept this change or not. Thanks. -- 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 3/3] smp/ipi:Remove check around csd lock in handler for smp_call_function variants
On Sat, Jul 06, 2013 at 01:36:27PM +0530, Preeti U Murthy wrote: > Ideally it should be under a WARN_ON(). csd_unlock() has that WARN_ON(). > Unlocking a parameter which is not locked should be seen as a bug, which > the above code is not doing. In fact it avoids it being reported as a bug. Although I know what's your meaning, but just like the comment in code: " /* * Unlocked CSDs are valid through generic_exec_single(): */ " If the csd don't come from generic_exec_single, then Unlocked CSDs maybe are not valid. So we check CSD_FLAG_LOCK to avoid trigger the WARN_ON in csd_unlock. Genric_exec_single's name imply it is a generic version, you know, maybe we will have "special" version. Thanks. -- 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 3/3] smp/ipi:Remove check around csd lock in handler for smp_call_function variants
On 07/06/2013 11:15 AM, Wang YanQing wrote:
> On Fri, Jul 05, 2013 at 09:57:21PM +0530, Preeti U Murthy wrote:
>> call_single_data is always locked by all callers of
>> arch_send_call_function_single_ipi() or
>> arch_send_call_function_ipi_mask() which results in execution of
>> generic_call_function_interrupt() handler.
>>
>> Hence remove the check for lock on csd in generic_call_function_interrupt()
>> handler, before unlocking it.
>
> I can't find where is the generic_call_function_interrupt :)
Sorry about this error :)
>
>> Signed-off-by: Preeti U Murthy
>> Cc: Peter Zijlstra
>> Cc: Ingo Molnar
>> Cc: Xiao Guangrong
>> Cc: srivatsa.b...@linux.vnet.ibm.com
>> Cc: Paul E. McKenney
>> Cc: Steven Rostedt
>> Cc: Rusty Russell > ---
>>
>> kernel/smp.c | 14 +-
>> 1 file changed, 1 insertion(+), 13 deletions(-)
>>
>> diff --git a/kernel/smp.c b/kernel/smp.c
>> index b6981ae..d37581a 100644
>> --- a/kernel/smp.c
>> +++ b/kernel/smp.c
>> @@ -181,25 +181,13 @@ void generic_smp_call_function_single_interrupt(void)
>>
>> while (!list_empty()) {
>> struct call_single_data *csd;
>> -unsigned int csd_flags;
>>
>> csd = list_entry(list.next, struct call_single_data, list);
>> list_del(>list);
>>
>> -/*
>> - * 'csd' can be invalid after this call if flags == 0
>> - * (when called through generic_exec_single()),
>> - * so save them away before making the call:
>> - */
>> -csd_flags = csd->flags;
>> -
>
> You haven't mention this change in the ChangeLog, don't do it.
Right, I will include it in the changelog.
> I can't see any harm to remove csd_flags, but I hope others
> check it again.
>
>> csd->func(csd->info);
>>
>> -/*
>> - * Unlocked CSDs are valid through generic_exec_single():
>> - */
>> -if (csd_flags & CSD_FLAG_LOCK)
>> -csd_unlock(csd);
>> +csd_unlock(csd);
>
> I don't like this change, I think check CSD_FLAG_LOCK
> to make sure we really need csd_unlock is good.
Ideally it should be under a WARN_ON(). csd_unlock() has that WARN_ON().
Unlocking a parameter which is not locked should be seen as a bug, which
the above code is not doing. In fact it avoids it being reported as a bug.
>
> Just like you can't know who and how people will use the
> API, so some robust check code is good.
>
Regards
Preeti U Murthy
--
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 3/3] smp/ipi:Remove check around csd lock in handler for smp_call_function variants
On 07/06/2013 11:15 AM, Wang YanQing wrote:
On Fri, Jul 05, 2013 at 09:57:21PM +0530, Preeti U Murthy wrote:
call_single_data is always locked by all callers of
arch_send_call_function_single_ipi() or
arch_send_call_function_ipi_mask() which results in execution of
generic_call_function_interrupt() handler.
Hence remove the check for lock on csd in generic_call_function_interrupt()
handler, before unlocking it.
I can't find where is the generic_call_function_interrupt :)
Sorry about this error :)
Signed-off-by: Preeti U Murthy pre...@linux.vnet.ibm.com
Cc: Peter Zijlstra a.p.zijls...@chello.nl
Cc: Ingo Molnar mi...@elte.hu
Cc: Xiao Guangrong xiaoguangr...@cn.fujitsu.com
Cc: srivatsa.b...@linux.vnet.ibm.com
Cc: Paul E. McKenney paul...@linux.vnet.ibm.com
Cc: Steven Rostedt rost...@goodmis.org
Cc: Rusty Russell ru...@rustcorp.com.au
---
kernel/smp.c | 14 +-
1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/kernel/smp.c b/kernel/smp.c
index b6981ae..d37581a 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -181,25 +181,13 @@ void generic_smp_call_function_single_interrupt(void)
while (!list_empty(list)) {
struct call_single_data *csd;
-unsigned int csd_flags;
csd = list_entry(list.next, struct call_single_data, list);
list_del(csd-list);
-/*
- * 'csd' can be invalid after this call if flags == 0
- * (when called through generic_exec_single()),
- * so save them away before making the call:
- */
-csd_flags = csd-flags;
-
You haven't mention this change in the ChangeLog, don't do it.
Right, I will include it in the changelog.
I can't see any harm to remove csd_flags, but I hope others
check it again.
csd-func(csd-info);
-/*
- * Unlocked CSDs are valid through generic_exec_single():
- */
-if (csd_flags CSD_FLAG_LOCK)
-csd_unlock(csd);
+csd_unlock(csd);
I don't like this change, I think check CSD_FLAG_LOCK
to make sure we really need csd_unlock is good.
Ideally it should be under a WARN_ON(). csd_unlock() has that WARN_ON().
Unlocking a parameter which is not locked should be seen as a bug, which
the above code is not doing. In fact it avoids it being reported as a bug.
Just like you can't know who and how people will use the
API, so some robust check code is good.
Regards
Preeti U Murthy
--
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 3/3] smp/ipi:Remove check around csd lock in handler for smp_call_function variants
On Sat, Jul 06, 2013 at 01:36:27PM +0530, Preeti U Murthy wrote: Ideally it should be under a WARN_ON(). csd_unlock() has that WARN_ON(). Unlocking a parameter which is not locked should be seen as a bug, which the above code is not doing. In fact it avoids it being reported as a bug. Although I know what's your meaning, but just like the comment in code: /* * Unlocked CSDs are valid through generic_exec_single(): */ If the csd don't come from generic_exec_single, then Unlocked CSDs maybe are not valid. So we check CSD_FLAG_LOCK to avoid trigger the WARN_ON in csd_unlock. Genric_exec_single's name imply it is a generic version, you know, maybe we will have special version. Thanks. -- 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 3/3] smp/ipi:Remove check around csd lock in handler for smp_call_function variants
On Fri, Jul 05, 2013 at 09:57:21PM +0530, Preeti U Murthy wrote:
> call_single_data is always locked by all callers of
> arch_send_call_function_single_ipi() or
> arch_send_call_function_ipi_mask() which results in execution of
> generic_call_function_interrupt() handler.
>
> Hence remove the check for lock on csd in generic_call_function_interrupt()
> handler, before unlocking it.
I can't find where is the generic_call_function_interrupt :)
> Signed-off-by: Preeti U Murthy
> Cc: Peter Zijlstra
> Cc: Ingo Molnar
> Cc: Xiao Guangrong
> Cc: srivatsa.b...@linux.vnet.ibm.com
> Cc: Paul E. McKenney
> Cc: Steven Rostedt
> Cc: Rusty Russell ---
>
> kernel/smp.c | 14 +-
> 1 file changed, 1 insertion(+), 13 deletions(-)
>
> diff --git a/kernel/smp.c b/kernel/smp.c
> index b6981ae..d37581a 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -181,25 +181,13 @@ void generic_smp_call_function_single_interrupt(void)
>
> while (!list_empty()) {
> struct call_single_data *csd;
> - unsigned int csd_flags;
>
> csd = list_entry(list.next, struct call_single_data, list);
> list_del(>list);
>
> - /*
> - * 'csd' can be invalid after this call if flags == 0
> - * (when called through generic_exec_single()),
> - * so save them away before making the call:
> - */
> - csd_flags = csd->flags;
> -
You haven't mention this change in the ChangeLog, don't do it.
I can't see any harm to remove csd_flags, but I hope others
check it again.
> csd->func(csd->info);
>
> - /*
> - * Unlocked CSDs are valid through generic_exec_single():
> - */
> - if (csd_flags & CSD_FLAG_LOCK)
> - csd_unlock(csd);
> + csd_unlock(csd);
I don't like this change, I think check CSD_FLAG_LOCK
to make sure we really need csd_unlock is good.
Just like you can't know who and how people will use the
API, so some robust check code is good.
--
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 3/3] smp/ipi:Remove check around csd lock in handler for smp_call_function variants
call_single_data is always locked by all callers of arch_send_call_function_single_ipi() or arch_send_call_function_ipi_mask() which results in execution of generic_call_function_interrupt() handler. Hence remove the check for lock on csd in generic_call_function_interrupt() handler, before unlocking it. Signed-off-by: Preeti U Murthy Cc: Peter Zijlstra Cc: Ingo Molnar Cc: Xiao Guangrong Cc: srivatsa.b...@linux.vnet.ibm.com Cc: Paul E. McKenney Cc: Steven Rostedt Cc: Rusty Russell list); - /* -* 'csd' can be invalid after this call if flags == 0 -* (when called through generic_exec_single()), -* so save them away before making the call: -*/ - csd_flags = csd->flags; - csd->func(csd->info); - /* -* Unlocked CSDs are valid through generic_exec_single(): -*/ - if (csd_flags & CSD_FLAG_LOCK) - csd_unlock(csd); + csd_unlock(csd); } } -- 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 3/3] smp/ipi:Remove check around csd lock in handler for smp_call_function variants
call_single_data is always locked by all callers of
arch_send_call_function_single_ipi() or
arch_send_call_function_ipi_mask() which results in execution of
generic_call_function_interrupt() handler.
Hence remove the check for lock on csd in generic_call_function_interrupt()
handler, before unlocking it.
Signed-off-by: Preeti U Murthy pre...@linux.vnet.ibm.com
Cc: Peter Zijlstra a.p.zijls...@chello.nl
Cc: Ingo Molnar mi...@elte.hu
Cc: Xiao Guangrong xiaoguangr...@cn.fujitsu.com
Cc: srivatsa.b...@linux.vnet.ibm.com
Cc: Paul E. McKenney paul...@linux.vnet.ibm.com
Cc: Steven Rostedt rost...@goodmis.org
Cc: Rusty Russell ru...@rustcorp.com.au
---
kernel/smp.c | 14 +-
1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/kernel/smp.c b/kernel/smp.c
index b6981ae..d37581a 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -181,25 +181,13 @@ void generic_smp_call_function_single_interrupt(void)
while (!list_empty(list)) {
struct call_single_data *csd;
- unsigned int csd_flags;
csd = list_entry(list.next, struct call_single_data, list);
list_del(csd-list);
- /*
-* 'csd' can be invalid after this call if flags == 0
-* (when called through generic_exec_single()),
-* so save them away before making the call:
-*/
- csd_flags = csd-flags;
-
csd-func(csd-info);
- /*
-* Unlocked CSDs are valid through generic_exec_single():
-*/
- if (csd_flags CSD_FLAG_LOCK)
- csd_unlock(csd);
+ csd_unlock(csd);
}
}
--
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 3/3] smp/ipi:Remove check around csd lock in handler for smp_call_function variants
On Fri, Jul 05, 2013 at 09:57:21PM +0530, Preeti U Murthy wrote:
call_single_data is always locked by all callers of
arch_send_call_function_single_ipi() or
arch_send_call_function_ipi_mask() which results in execution of
generic_call_function_interrupt() handler.
Hence remove the check for lock on csd in generic_call_function_interrupt()
handler, before unlocking it.
I can't find where is the generic_call_function_interrupt :)
Signed-off-by: Preeti U Murthy pre...@linux.vnet.ibm.com
Cc: Peter Zijlstra a.p.zijls...@chello.nl
Cc: Ingo Molnar mi...@elte.hu
Cc: Xiao Guangrong xiaoguangr...@cn.fujitsu.com
Cc: srivatsa.b...@linux.vnet.ibm.com
Cc: Paul E. McKenney paul...@linux.vnet.ibm.com
Cc: Steven Rostedt rost...@goodmis.org
Cc: Rusty Russell ru...@rustcorp.com.au
---
kernel/smp.c | 14 +-
1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/kernel/smp.c b/kernel/smp.c
index b6981ae..d37581a 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -181,25 +181,13 @@ void generic_smp_call_function_single_interrupt(void)
while (!list_empty(list)) {
struct call_single_data *csd;
- unsigned int csd_flags;
csd = list_entry(list.next, struct call_single_data, list);
list_del(csd-list);
- /*
- * 'csd' can be invalid after this call if flags == 0
- * (when called through generic_exec_single()),
- * so save them away before making the call:
- */
- csd_flags = csd-flags;
-
You haven't mention this change in the ChangeLog, don't do it.
I can't see any harm to remove csd_flags, but I hope others
check it again.
csd-func(csd-info);
- /*
- * Unlocked CSDs are valid through generic_exec_single():
- */
- if (csd_flags CSD_FLAG_LOCK)
- csd_unlock(csd);
+ csd_unlock(csd);
I don't like this change, I think check CSD_FLAG_LOCK
to make sure we really need csd_unlock is good.
Just like you can't know who and how people will use the
API, so some robust check code is good.
--
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/
