kprobes: get rid of distinct type warning

kprobes: get rid of distinct type warning

[Heiko Carstens]

From: Heiko Carstens <heiko.carstens@de.ibm.com>

Every time I see this:

kernel/kprobes.c: In function 'register_kretprobe':
kernel/kprobes.c:1038: warning: comparison of distinct pointer types lacks a cast

I'm wondering if something changed in common code and we need to do
something for s390. Apparently that's not the case.
Let's get rid of this annoying warning.

Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 kernel/kprobes.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1035,7 +1035,7 @@ int __kprobes register_kretprobe(struct 
 	/* Pre-allocate memory for max kretprobe instances */
 	if (rp->maxactive <= 0) {
 #ifdef CONFIG_PREEMPT
-		rp->maxactive = max(10, 2 * num_possible_cpus());
+		rp->maxactive = max(10U, 2 * num_possible_cpus());
 #else
 		rp->maxactive = num_possible_cpus();
 #endif

Re: kprobes: get rid of distinct type warning

[Ananth N Mavinakayanahalli]

On Mon, Dec 21, 2009 at 11:15:49AM +0100, Heiko Carstens wrote:
> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> Every time I see this:
> 
> kernel/kprobes.c: In function 'register_kretprobe':
> kernel/kprobes.c:1038: warning: comparison of distinct pointer types lacks a cast
> 
> I'm wondering if something changed in common code and we need to do
> something for s390. Apparently that's not the case.
> Let's get rid of this annoying warning.
> 
> Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>

Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>

Thanks Heiko. I'd tested this on x86 and powerpc and neither throws the
warning. No wonder I missed it.

Re: kprobes: get rid of distinct type warning

[Heiko Carstens]

On Mon, Dec 21, 2009 at 04:47:59PM +0530, Ananth N Mavinakayanahalli wrote:
> On Mon, Dec 21, 2009 at 11:15:49AM +0100, Heiko Carstens wrote:
> > From: Heiko Carstens <heiko.carstens@de.ibm.com>
> > 
> > Every time I see this:
> > 
> > kernel/kprobes.c: In function 'register_kretprobe':
> > kernel/kprobes.c:1038: warning: comparison of distinct pointer types lacks a cast
> > 
> > I'm wondering if something changed in common code and we need to do
> > something for s390. Apparently that's not the case.
> > Let's get rid of this annoying warning.
> > 
> > Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
> > Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
> 
> Thanks Heiko. I'd tested this on x86 and powerpc and neither throws the
> warning. No wonder I missed it.

Of course the patch wouldn't help for CONFIG_PREEMPT and !CONFIG_SMP since
we would have a comparison of a signed and and unsigned value again *sigh*.

So here's an updated version that should always work:

Subject: [PATCH] kprobes: get rid of distinct type warning

From: Heiko Carstens <heiko.carstens@de.ibm.com>

Every time I see this:

kernel/kprobes.c: In function 'register_kretprobe':
kernel/kprobes.c:1038: warning: comparison of distinct pointer types lacks a cast

I'm wondering if something changed in common code and we need to do
something for s390. Apparently that's not the case.
Let's get rid of this annoying warning.

Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 kernel/kprobes.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1035,7 +1035,7 @@ int __kprobes register_kretprobe(struct 
 	/* Pre-allocate memory for max kretprobe instances */
 	if (rp->maxactive <= 0) {
 #ifdef CONFIG_PREEMPT
-		rp->maxactive = max(10, 2 * num_possible_cpus());
+		rp->maxactive = max_t(unsigned int, 10, 2*num_possible_cpus());
 #else
 		rp->maxactive = num_possible_cpus();
 #endif

[tip:perf/urgent] kprobes: Fix distinct type warning

[tip-bot for Heiko Carstens]

Commit-ID:  c2ef6661ce62e26a8c0978e521fab646128a144b
Gitweb:     http://git.kernel.org/tip/c2ef6661ce62e26a8c0978e521fab646128a144b
Author:     Heiko Carstens <heiko.carstens@de.ibm.com>
AuthorDate: Mon, 21 Dec 2009 13:02:24 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Mon, 28 Dec 2009 10:25:31 +0100

kprobes: Fix distinct type warning

Every time I see this:

 kernel/kprobes.c: In function 'register_kretprobe':
 kernel/kprobes.c:1038: warning: comparison of distinct pointer types lacks a cast

I'm wondering if something changed in common code and we need to
do something for s390. Apparently that's not the case.
Let's get rid of this annoying warning.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Cc: Masami Hiramatsu <mhiramat@redhat.com>
LKML-Reference: <20091221120224.GA4471@osiris.boeblingen.de.ibm.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/kprobes.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index e5342a3..b7df302 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1035,7 +1035,7 @@ int __kprobes register_kretprobe(struct kretprobe *rp)
 	/* Pre-allocate memory for max kretprobe instances */
 	if (rp->maxactive <= 0) {
 #ifdef CONFIG_PREEMPT
-		rp->maxactive = max(10, 2 * num_possible_cpus());
+		rp->maxactive = max_t(unsigned int, 10, 2*num_possible_cpus());
 #else
 		rp->maxactive = num_possible_cpus();
 #endif

Re: kprobes: get rid of distinct type warning

[Andrew Morton]

On Mon, 21 Dec 2009 13:02:24 +0100
Heiko Carstens <heiko.carstens@de.ibm.com> wrote:

> Of course the patch wouldn't help for CONFIG_PREEMPT and !CONFIG_SMP since
> we would have a comparison of a signed and and unsigned value again *sigh*.

We should fix that, shouldn't we?  Rather than working around it at one
caller site.

: #if NR_CPUS > 1
: #define num_online_cpus()	cpumask_weight(cpu_online_mask)
: #define num_possible_cpus()	cpumask_weight(cpu_possible_mask)
: #define num_present_cpus()	cpumask_weight(cpu_present_mask)
: #define num_active_cpus()	cpumask_weight(cpu_active_mask)
: #define cpu_online(cpu)		cpumask_test_cpu((cpu), cpu_online_mask)
: #define cpu_possible(cpu)	cpumask_test_cpu((cpu), cpu_possible_mask)
: #define cpu_present(cpu)	cpumask_test_cpu((cpu), cpu_present_mask)
: #define cpu_active(cpu)		cpumask_test_cpu((cpu), cpu_active_mask)
: #else
: #define num_online_cpus()	1
: #define num_possible_cpus()	1
: #define num_present_cpus()	1
: #define num_active_cpus()	1
: #define cpu_online(cpu)		((cpu) == 0)
: #define cpu_possible(cpu)	((cpu) == 0)
: #define cpu_present(cpu)	((cpu) == 0)
: #define cpu_active(cpu)		((cpu) == 0)
: #endif

The num_*() "functions" return unsigned on SMP and int on UP.  This is
wrong.

The cpu_*() "functions" got lucky and return int in both cases.

Personally I think it's neatest if a quantity which can never be
negative is held in an unsigned type.  Than includes anything starting
with "num".  But for expediency's sake we could live with making these
things consistently return "int".

Alas, changing those four num_*() "functions" to return int on SMP is a
pretty wide-reaching change and will probably expose warts.

Re: kprobes: get rid of distinct type warning

[Heiko Carstens]

On Wed, Dec 30, 2009 at 01:29:45PM -0800, Andrew Morton wrote:
> On Mon, 21 Dec 2009 13:02:24 +0100
> Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> 
> > Of course the patch wouldn't help for CONFIG_PREEMPT and !CONFIG_SMP since
> > we would have a comparison of a signed and and unsigned value again *sigh*.
> 
> We should fix that, shouldn't we?  Rather than working around it at one
> caller site.
> 
> : #if NR_CPUS > 1
> : #define num_online_cpus()	cpumask_weight(cpu_online_mask)
> : #define num_possible_cpus()	cpumask_weight(cpu_possible_mask)
> : #define num_present_cpus()	cpumask_weight(cpu_present_mask)
> : #define num_active_cpus()	cpumask_weight(cpu_active_mask)
> : #define cpu_online(cpu)		cpumask_test_cpu((cpu), cpu_online_mask)
> : #define cpu_possible(cpu)	cpumask_test_cpu((cpu), cpu_possible_mask)
> : #define cpu_present(cpu)	cpumask_test_cpu((cpu), cpu_present_mask)
> : #define cpu_active(cpu)		cpumask_test_cpu((cpu), cpu_active_mask)
> : #else
> : #define num_online_cpus()	1
> : #define num_possible_cpus()	1
> : #define num_present_cpus()	1
> : #define num_active_cpus()	1
> : #define cpu_online(cpu)		((cpu) == 0)
> : #define cpu_possible(cpu)	((cpu) == 0)
> : #define cpu_present(cpu)	((cpu) == 0)
> : #define cpu_active(cpu)		((cpu) == 0)
> : #endif
> 
> The num_*() "functions" return unsigned on SMP and int on UP.  This is
> wrong.
> 
> The cpu_*() "functions" got lucky and return int in both cases.
> 
> Personally I think it's neatest if a quantity which can never be
> negative is held in an unsigned type.  Than includes anything starting
> with "num".  But for expediency's sake we could live with making these
> things consistently return "int".
> 
> Alas, changing those four num_*() "functions" to return int on SMP is a
> pretty wide-reaching change and will probably expose warts.

Looks like there are quite a lot of num_* function usages in the kernel.
Some seem to assume they return an int some assume an unsigned int.
Don't know if it's worth changing anything here.
Maybe Rusty has an opinion.

Re: kprobes: get rid of distinct type warning

[Rusty Russell]

On Tue, 5 Jan 2010 02:27:02 am Heiko Carstens wrote:
> On Wed, Dec 30, 2009 at 01:29:45PM -0800, Andrew Morton wrote:
> > On Mon, 21 Dec 2009 13:02:24 +0100
> > Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> > 
> > > Of course the patch wouldn't help for CONFIG_PREEMPT and !CONFIG_SMP since
> > > we would have a comparison of a signed and and unsigned value again *sigh*.
> > 
> > We should fix that, shouldn't we?  Rather than working around it at one
> > caller site.
> > 
> > : #if NR_CPUS > 1
> > : #define num_online_cpus()	cpumask_weight(cpu_online_mask)
> > : #define num_possible_cpus()	cpumask_weight(cpu_possible_mask)
> > : #define num_present_cpus()	cpumask_weight(cpu_present_mask)
> > : #define num_active_cpus()	cpumask_weight(cpu_active_mask)
> > : #define cpu_online(cpu)		cpumask_test_cpu((cpu), cpu_online_mask)
> > : #define cpu_possible(cpu)	cpumask_test_cpu((cpu), cpu_possible_mask)
> > : #define cpu_present(cpu)	cpumask_test_cpu((cpu), cpu_present_mask)
> > : #define cpu_active(cpu)		cpumask_test_cpu((cpu), cpu_active_mask)
> > : #else
> > : #define num_online_cpus()	1
> > : #define num_possible_cpus()	1
> > : #define num_present_cpus()	1
> > : #define num_active_cpus()	1
> > : #define cpu_online(cpu)		((cpu) == 0)
> > : #define cpu_possible(cpu)	((cpu) == 0)
> > : #define cpu_present(cpu)	((cpu) == 0)
> > : #define cpu_active(cpu)		((cpu) == 0)
> > : #endif
> > 
> > The num_*() "functions" return unsigned on SMP and int on UP.  This is
> > wrong.
> > 
> > The cpu_*() "functions" got lucky and return int in both cases.
> > 
> > Personally I think it's neatest if a quantity which can never be
> > negative is held in an unsigned type.  Than includes anything starting
> > with "num".  But for expediency's sake we could live with making these
> > things consistently return "int".
> > 
> > Alas, changing those four num_*() "functions" to return int on SMP is a
> > pretty wide-reaching change and will probably expose warts.
> 
> Looks like there are quite a lot of num_* function usages in the kernel.
> Some seem to assume they return an int some assume an unsigned int.
> Don't know if it's worth changing anything here.
> Maybe Rusty has an opinion.

If we have to go one way or the other, go with unsigned.

What does such a patch look like?

Thanks,
Rusty.

Re: kprobes: get rid of distinct type warning

[Heiko Carstens]

On Tue, Jan 05, 2010 at 09:15:41AM +1030, Rusty Russell wrote:
> On Tue, 5 Jan 2010 02:27:02 am Heiko Carstens wrote:
> > On Wed, Dec 30, 2009 at 01:29:45PM -0800, Andrew Morton wrote:
> > > The num_*() "functions" return unsigned on SMP and int on UP.  This is
> > > wrong.
> > > 
> > > The cpu_*() "functions" got lucky and return int in both cases.
> > > 
> > > Personally I think it's neatest if a quantity which can never be
> > > negative is held in an unsigned type.  Than includes anything starting
> > > with "num".  But for expediency's sake we could live with making these
> > > things consistently return "int".
> > > 
> > > Alas, changing those four num_*() "functions" to return int on SMP is a
> > > pretty wide-reaching change and will probably expose warts.
> > 
> > Looks like there are quite a lot of num_* function usages in the kernel.
> > Some seem to assume they return an int some assume an unsigned int.
> > Don't know if it's worth changing anything here.
> > Maybe Rusty has an opinion.
> 
> If we have to go one way or the other, go with unsigned.
> 
> What does such a patch look like?

Something like this, doesn't even trigger new warnings on an !SMP defconfig
build:

Subject: [PATCH] cpumask: let num_*_cpus() function always return unsigned values

From: Heiko Carstens <heiko.carstens@de.ibm.com>

Dependent on CONFIG_SMP the num_*_cpus() functions return unsigned or
signed values.
Let them always return unsigned values to avoid strange casts.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 include/linux/cpumask.h |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -90,10 +90,10 @@ extern const struct cpumask *const cpu_a
 #define cpu_present(cpu)	cpumask_test_cpu((cpu), cpu_present_mask)
 #define cpu_active(cpu)		cpumask_test_cpu((cpu), cpu_active_mask)
 #else
-#define num_online_cpus()	1
-#define num_possible_cpus()	1
-#define num_present_cpus()	1
-#define num_active_cpus()	1
+#define num_online_cpus()	1U
+#define num_possible_cpus()	1U
+#define num_present_cpus()	1U
+#define num_active_cpus()	1U
 #define cpu_online(cpu)		((cpu) == 0)
 #define cpu_possible(cpu)	((cpu) == 0)
 #define cpu_present(cpu)	((cpu) == 0)

Re: kprobes: get rid of distinct type warning

[Andrew Morton]

On Tue, 5 Jan 2010 09:40:27 +0100
Heiko Carstens <heiko.carstens@de.ibm.com> wrote:

> On Tue, Jan 05, 2010 at 09:15:41AM +1030, Rusty Russell wrote:
> > On Tue, 5 Jan 2010 02:27:02 am Heiko Carstens wrote:
> > > On Wed, Dec 30, 2009 at 01:29:45PM -0800, Andrew Morton wrote:
> > > > The num_*() "functions" return unsigned on SMP and int on UP.  This is
> > > > wrong.
> > > > 
> > > > The cpu_*() "functions" got lucky and return int in both cases.
> > > > 
> > > > Personally I think it's neatest if a quantity which can never be
> > > > negative is held in an unsigned type.  Than includes anything starting
> > > > with "num".  But for expediency's sake we could live with making these
> > > > things consistently return "int".
> > > > 
> > > > Alas, changing those four num_*() "functions" to return int on SMP is a
> > > > pretty wide-reaching change and will probably expose warts.
> > > 
> > > Looks like there are quite a lot of num_* function usages in the kernel.
> > > Some seem to assume they return an int some assume an unsigned int.
> > > Don't know if it's worth changing anything here.
> > > Maybe Rusty has an opinion.
> > 
> > If we have to go one way or the other, go with unsigned.
> > 
> > What does such a patch look like?
> 
> Something like this, doesn't even trigger new warnings on an !SMP defconfig
> build:
> 
> Subject: [PATCH] cpumask: let num_*_cpus() function always return unsigned values
> 
> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> Dependent on CONFIG_SMP the num_*_cpus() functions return unsigned or
> signed values.
> Let them always return unsigned values to avoid strange casts.
> 
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> ---
>  include/linux/cpumask.h |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -90,10 +90,10 @@ extern const struct cpumask *const cpu_a
>  #define cpu_present(cpu)	cpumask_test_cpu((cpu), cpu_present_mask)
>  #define cpu_active(cpu)		cpumask_test_cpu((cpu), cpu_active_mask)
>  #else
> -#define num_online_cpus()	1
> -#define num_possible_cpus()	1
> -#define num_present_cpus()	1
> -#define num_active_cpus()	1
> +#define num_online_cpus()	1U
> +#define num_possible_cpus()	1U
> +#define num_present_cpus()	1U
> +#define num_active_cpus()	1U
>  #define cpu_online(cpu)		((cpu) == 0)
>  #define cpu_possible(cpu)	((cpu) == 0)
>  #define cpu_present(cpu)	((cpu) == 0)

I assume that this fixes the kprobes warning, so 

Commit-ID:  c2ef6661ce62e26a8c0978e521fab646128a144b
Gitweb:     http://git.kernel.org/tip/c2ef6661ce62e26a8c0978e521fab646128a144b
Author:     Heiko Carstens <heiko.carstens@de.ibm.com>
AuthorDate: Mon, 21 Dec 2009 13:02:24 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Mon, 28 Dec 2009 10:25:31 +0100

becomes unneeded?

Re: kprobes: get rid of distinct type warning

[Heiko Carstens]

On Fri, Jan 08, 2010 at 04:18:48PM -0800, Andrew Morton wrote:
> On Tue, 5 Jan 2010 09:40:27 +0100
> Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> > --- a/include/linux/cpumask.h
> > +++ b/include/linux/cpumask.h
> > @@ -90,10 +90,10 @@ extern const struct cpumask *const cpu_a
> >  #define cpu_present(cpu)	cpumask_test_cpu((cpu), cpu_present_mask)
> >  #define cpu_active(cpu)		cpumask_test_cpu((cpu), cpu_active_mask)
> >  #else
> > -#define num_online_cpus()	1
> > -#define num_possible_cpus()	1
> > -#define num_present_cpus()	1
> > -#define num_active_cpus()	1
> > +#define num_online_cpus()	1U
> > +#define num_possible_cpus()	1U
> > +#define num_present_cpus()	1U
> > +#define num_active_cpus()	1U
> >  #define cpu_online(cpu)		((cpu) == 0)
> >  #define cpu_possible(cpu)	((cpu) == 0)
> >  #define cpu_present(cpu)	((cpu) == 0)
> 
> I assume that this fixes the kprobes warning, so 
> 
> Commit-ID:  c2ef6661ce62e26a8c0978e521fab646128a144b
> Gitweb:     http://git.kernel.org/tip/c2ef6661ce62e26a8c0978e521fab646128a144b
> Author:     Heiko Carstens <heiko.carstens@de.ibm.com>
> AuthorDate: Mon, 21 Dec 2009 13:02:24 +0100
> Committer:  Ingo Molnar <mingo@elte.hu>
> CommitDate: Mon, 28 Dec 2009 10:25:31 +0100
> 
> becomes unneeded?

Nah, that would be too easy. The code still compares an int (10) and
an unsigned int. The only thing that could be changed with this patch
is that max_t isn't needed anymore, so that the patch below would
work as well (10U).
But I don't think it's worth it.

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index b7df302..85222a9 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1035,7 +1035,7 @@ int __kprobes register_kretprobe(struct kretprobe *rp)
 	/* Pre-allocate memory for max kretprobe instances */
 	if (rp->maxactive <= 0) {
 #ifdef CONFIG_PREEMPT
-		rp->maxactive = max_t(unsigned int, 10, 2*num_possible_cpus());
+		rp->maxactive = max(10U, 2*num_possible_cpus());
 #else
 		rp->maxactive = num_possible_cpus();
 #endif