Re: [PATCH] Fix the x86 specializations of atomic_[set|clear]_mask

[Patrick Palka]

On Thu, Oct 16, 2014 at 4:24 PM, Peter Zijlstra  wrote:
> On Thu, Oct 16, 2014 at 12:49:43PM -0400, Patrick Palka wrote:
>> On Oct 16, 2014 4:02 AM, "Peter Zijlstra"  wrote:
>> >
>> > On Wed, Oct 15, 2014 at 04:38:01PM -0400, Patrick Palka wrote:
>> > > This patch fixes a number of issues with these specializations:
>> > >
>> > >   1. The memory operand inside the asm specification is erroneously
>> > >   declared read-only instead of read-write.
>> > >
>> > >   2. There is no reason to require the 1st operand of andl/orl to be
>> > >   inside a register; the 1st operand could also be an immediate operand.
>> > >   So change its specification from "r" to "ir".
>> > >
>> > >   3. Since addr is supposed to be an atomic_t *, the memory operand
>> > >   should be addr->counter and not *addr.
>> > >
>> > >   4. These specializations should be inline functions instead of macros.
>> > >
>> > >   5. Finally, the "memory" clobbers are unnecessary, so they should be
>> > >   removed.  (This is in line with the other atomic functions such as
>> > >   atomic_add and atomic_sub, the likes of which do not have a "memory"
>> > >   clobber.)
>> >
>> > No real problem with this, but I'm going to kill off these functions
>> > when I get a little time :)
>>
>> Hmm why's that?
>
> because they're odd (inconsistent with the rest of the atomic
> interfaces) and not implemented by all archs.
>
> See: https://lkml.org/lkml/2014/2/6/196
>
> 3.18 will include up to 4/5 of that series and when I get a spare moment
> I need cleanup/post the next arch sweep that will get us that 5/5 thing.
>
>

Cool! Perhaps atomic_inc_short() should be killed off too. It
currently has no callers and, despite what its name suggests, it is
not even atomic..
--
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] Fix the x86 specializations of atomic_[set|clear]_mask

[Patrick Palka]

This patch fixes a number of issues with these specializations:

  1. The memory operand inside the asm specification is erroneously
  declared read-only instead of read-write.

  2. There is no reason to require the 1st operand of andl/orl to be
  inside a register; the 1st operand could also be an immediate operand.
  So change its specification from "r" to "ir".

  3. Since addr is supposed to be an atomic_t *, the memory operand
  should be addr->counter and not *addr.

  4. These specializations should be inline functions instead of macros.

  5. Finally, the "memory" clobbers are unnecessary, so they should be
  removed.  (This is in line with the other atomic functions such as
  atomic_add and atomic_sub, the likes of which do not have a "memory"
  clobber.)

Cc: Thomas Gleixner 
Cc: Ingo Molnar 
Cc: "H. Peter Anvin" 
Cc: x...@kernel.org
Cc: Peter Zijlstra 
Cc: Hans-Christian Egtvedt 
Cc: "Paul E. McKenney" 
Cc: Pranith Kumar 
Signed-off-by: Patrick Palka 
---
 arch/x86/include/asm/atomic.h | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/atomic.h b/arch/x86/include/asm/atomic.h
index 5e5cd12..83ae239 100644
--- a/arch/x86/include/asm/atomic.h
+++ b/arch/x86/include/asm/atomic.h
@@ -219,15 +219,19 @@ static inline short int atomic_inc_short(short int *v)
return *v;
 }
 
-/* These are x86-specific, used by some header files */
-#define atomic_clear_mask(mask, addr)  \
-   asm volatile(LOCK_PREFIX "andl %0,%1"   \
-: : "r" (~(mask)), "m" (*(addr)) : "memory")
+static inline void atomic_clear_mask(int mask, atomic_t *v)
+{
+   asm volatile(LOCK_PREFIX "andl %1, %0"
+: "+m" (v->counter)
+: "ir" (~mask));
+}
 
-#define atomic_set_mask(mask, addr)\
-   asm volatile(LOCK_PREFIX "orl %0,%1"\
-: : "r" ((unsigned)(mask)), "m" (*(addr))  \
-: "memory")
+static inline void atomic_set_mask(int mask, atomic_t *v)
+{
+   asm volatile(LOCK_PREFIX "orl %1, %0"
+: "+m" (v->counter)
+: "ir" (mask));
+}
 
 #ifdef CONFIG_X86_32
 # include 
-- 
2.1.2.443.g670a3c1

--
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] ring-buffer: use atomic_[set|clear]_mask in place of a cmpxchg loop

[Patrick Palka]

On Wed, Oct 15, 2014 at 3:08 PM, Patrick Palka  wrote:
> On Wed, Oct 15, 2014 at 9:41 AM, Patrick Palka  wrote:
>> Instead of open-coding the equivalent cmpxchg loop we can just use
>> atomic_set_mask and atomic_clear_mask to atomically OR or AND
>> the value in &buffer->record_disabled.
>>
>> Cc: Steven Rostedt 
>> Cc: Ingo Molnar 
>> Signed-off-by: Patrick Palka 
>> ---
>>
>> NB: I have only tested this patch on x86_64 by booting with
>> CONFIG_RING_BUFFER_STARTUP_TEST=y.
>>
>>  kernel/trace/ring_buffer.c | 16 ++--
>>  1 file changed, 2 insertions(+), 14 deletions(-)
>>
>> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
>> index 2d75c94..e15ce2a 100644
>> --- a/kernel/trace/ring_buffer.c
>> +++ b/kernel/trace/ring_buffer.c
>> @@ -3050,13 +3050,7 @@ EXPORT_SYMBOL_GPL(ring_buffer_record_enable);
>>   */
>>  void ring_buffer_record_off(struct ring_buffer *buffer)
>>  {
>> -   unsigned int rd;
>> -   unsigned int new_rd;
>> -
>> -   do {
>> -   rd = atomic_read(&buffer->record_disabled);
>> -   new_rd = rd | RB_BUFFER_OFF;
>> -   } while (atomic_cmpxchg(&buffer->record_disabled, rd, new_rd) != rd);
>> +   atomic_set_mask(RB_BUFFER_OFF, &buffer->record_disabled);
>>  }
>>  EXPORT_SYMBOL_GPL(ring_buffer_record_off);
>>
>> @@ -3073,13 +3067,7 @@ EXPORT_SYMBOL_GPL(ring_buffer_record_off);
>>   */
>>  void ring_buffer_record_on(struct ring_buffer *buffer)
>>  {
>> -   unsigned int rd;
>> -   unsigned int new_rd;
>> -
>> -   do {
>> -   rd = atomic_read(&buffer->record_disabled);
>> -   new_rd = rd & ~RB_BUFFER_OFF;
>> -   } while (atomic_cmpxchg(&buffer->record_disabled, rd, new_rd) != rd);
>> +   atomic_clear_mask(RB_BUFFER_OFF, &buffer->record_disabled);
>>  }
>>  EXPORT_SYMBOL_GPL(ring_buffer_record_on);
>>
>> --
>> 2.1.2.443.g670a3c1
>>
>
> It just occurred to me that atomic_clear_mask and atomic_set_mask
> expect to take an int *, not an atomic_t *, so this patch is bogus...
> Sorry for the noise.

Uh, actually, the generic implementations of these functions (defined
in include/asm-generic/atomic.h) _do_ take an atomic_t *. It's the
specialized x86 definitions of these functions (defined in
arch/x86/include/asm/atomic.h) that take (expect) an int *. So this
patch is not bogus; the x86 specializations for atomic_clear_mask and
atomic_set_mask are. I will try to fix this in a separate patch.
--
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] ring-buffer: use atomic_[set|clear]_mask in place of a cmpxchg loop

[Patrick Palka]

On Wed, Oct 15, 2014 at 9:41 AM, Patrick Palka  wrote:
> Instead of open-coding the equivalent cmpxchg loop we can just use
> atomic_set_mask and atomic_clear_mask to atomically OR or AND
> the value in &buffer->record_disabled.
>
> Cc: Steven Rostedt 
> Cc: Ingo Molnar 
> Signed-off-by: Patrick Palka 
> ---
>
> NB: I have only tested this patch on x86_64 by booting with
> CONFIG_RING_BUFFER_STARTUP_TEST=y.
>
>  kernel/trace/ring_buffer.c | 16 ++--
>  1 file changed, 2 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 2d75c94..e15ce2a 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -3050,13 +3050,7 @@ EXPORT_SYMBOL_GPL(ring_buffer_record_enable);
>   */
>  void ring_buffer_record_off(struct ring_buffer *buffer)
>  {
> -   unsigned int rd;
> -   unsigned int new_rd;
> -
> -   do {
> -   rd = atomic_read(&buffer->record_disabled);
> -   new_rd = rd | RB_BUFFER_OFF;
> -   } while (atomic_cmpxchg(&buffer->record_disabled, rd, new_rd) != rd);
> +   atomic_set_mask(RB_BUFFER_OFF, &buffer->record_disabled);
>  }
>  EXPORT_SYMBOL_GPL(ring_buffer_record_off);
>
> @@ -3073,13 +3067,7 @@ EXPORT_SYMBOL_GPL(ring_buffer_record_off);
>   */
>  void ring_buffer_record_on(struct ring_buffer *buffer)
>  {
> -   unsigned int rd;
> -   unsigned int new_rd;
> -
> -   do {
> -   rd = atomic_read(&buffer->record_disabled);
> -   new_rd = rd & ~RB_BUFFER_OFF;
> -   } while (atomic_cmpxchg(&buffer->record_disabled, rd, new_rd) != rd);
> +   atomic_clear_mask(RB_BUFFER_OFF, &buffer->record_disabled);
>  }
>  EXPORT_SYMBOL_GPL(ring_buffer_record_on);
>
> --
> 2.1.2.443.g670a3c1
>

It just occurred to me that atomic_clear_mask and atomic_set_mask
expect to take an int *, not an atomic_t *, so this patch is bogus...
Sorry for the noise.
--
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] ring-buffer: use atomic_[set|clear]_mask in place of a cmpxchg loop

[Patrick Palka]

Instead of open-coding the equivalent cmpxchg loop we can just use
atomic_set_mask and atomic_clear_mask to atomically OR or AND
the value in &buffer->record_disabled.

Cc: Steven Rostedt 
Cc: Ingo Molnar 
Signed-off-by: Patrick Palka 
---

NB: I have only tested this patch on x86_64 by booting with
CONFIG_RING_BUFFER_STARTUP_TEST=y.

 kernel/trace/ring_buffer.c | 16 ++--
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 2d75c94..e15ce2a 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -3050,13 +3050,7 @@ EXPORT_SYMBOL_GPL(ring_buffer_record_enable);
  */
 void ring_buffer_record_off(struct ring_buffer *buffer)
 {
-   unsigned int rd;
-   unsigned int new_rd;
-
-   do {
-   rd = atomic_read(&buffer->record_disabled);
-   new_rd = rd | RB_BUFFER_OFF;
-   } while (atomic_cmpxchg(&buffer->record_disabled, rd, new_rd) != rd);
+   atomic_set_mask(RB_BUFFER_OFF, &buffer->record_disabled);
 }
 EXPORT_SYMBOL_GPL(ring_buffer_record_off);
 
@@ -3073,13 +3067,7 @@ EXPORT_SYMBOL_GPL(ring_buffer_record_off);
  */
 void ring_buffer_record_on(struct ring_buffer *buffer)
 {
-   unsigned int rd;
-   unsigned int new_rd;
-
-   do {
-   rd = atomic_read(&buffer->record_disabled);
-   new_rd = rd & ~RB_BUFFER_OFF;
-   } while (atomic_cmpxchg(&buffer->record_disabled, rd, new_rd) != rd);
+   atomic_clear_mask(RB_BUFFER_OFF, &buffer->record_disabled);
 }
 EXPORT_SYMBOL_GPL(ring_buffer_record_on);
 
-- 
2.1.2.443.g670a3c1

--
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] Fix faulty logic in the case of recursive printk

[Patrick Palka]

Thanks for reviewing!  I have put back the call to strlen() and adjusted
the commit message accordingly.

Patrick

-- >8 --

We shouldn't set text_len in the code path that detects printk recursion
because text_len corresponds to the length of the string inside textbuf.
A few lines down from the line

text_len = strlen(recursion_msg);

is the line

text_len += vscnprintf(text + text_len, ...);

So if printk detects recursion, it sets text_len to 29 (the length of
recursion_msg) and logs an error.  Then the message supplied by the
caller of printk is stored inside textbuf but offset by 29 bytes.  This
means that the output of the recursive call to printk will contain 29
bytes of garbage in front of it.

This defect is caused by commit 458df9fd ("printk: remove separate
printk_sched buffers and use printk buf instead") which turned the line

text_len = vscnprintf(text, ...);

into

text_len += vscnprintf(text + text_len, ...);

To fix this, this patch avoids setting text_len when logging the printk
recursion error.  This patch also marks unlikely() the branch leading
up to this code.

Signed-off-by: Patrick Palka 
---
 kernel/printk/printk.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index e04c455..1ce7706 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1665,15 +1665,15 @@ asmlinkage int vprintk_emit(int facility, int level,
raw_spin_lock(&logbuf_lock);
logbuf_cpu = this_cpu;
 
-   if (recursion_bug) {
+   if (unlikely(recursion_bug)) {
static const char recursion_msg[] =
"BUG: recent printk recursion!";
 
recursion_bug = 0;
-   text_len = strlen(recursion_msg);
/* emit KERN_CRIT message */
printed_len += log_store(0, 2, LOG_PREFIX|LOG_NEWLINE, 0,
-NULL, 0, recursion_msg, text_len);
+NULL, 0, recursion_msg,
+strlen(recursion_msg));
}
 
/*
-- 
2.1.0

--
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] Fix faulty logic in the case of recursive printk

[Patrick Palka]

We shouldn't set text_len in the code path that detects printk recursion
because text_len corresponds to the length of the string inside textbuf.
A few lines down from the line

text_len = strlen(recursion_msg);

is the line

text_len += vscnprintf(text + text_len, ...);

So if printk detects recursion, it sets text_len to 29 and logs an
error.  Then the message supplied by the caller of printk is stored
inside textbuf but offset by 29 bytes.  This means that the output of
the recursive call to printk will contain 29 bytes of garbage in front
of it.

This defect is caused by commit 458df9fd ("printk: remove separate
printk_sched buffers and use printk buf instead") which turned

text_len = vscnprintf(text + text_len, ...);

into

text_len += vscnprintf(text + text_len, ...);

To fix this, this patch avoids setting text_len when logging the printk
recursion error.  This patch also performs a couple of local
micro-optimizations (use unlikely() and ARRAY_SIZE()).

Signed-off-by: Patrick Palka 
---
 kernel/printk/printk.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index e04c455..c101ec2 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1665,15 +1665,15 @@ asmlinkage int vprintk_emit(int facility, int level,
raw_spin_lock(&logbuf_lock);
logbuf_cpu = this_cpu;
 
-   if (recursion_bug) {
+   if (unlikely(recursion_bug)) {
static const char recursion_msg[] =
"BUG: recent printk recursion!";
 
recursion_bug = 0;
-   text_len = strlen(recursion_msg);
/* emit KERN_CRIT message */
printed_len += log_store(0, 2, LOG_PREFIX|LOG_NEWLINE, 0,
-NULL, 0, recursion_msg, text_len);
+NULL, 0, recursion_msg,
+ARRAY_SIZE(recursion_msg) - 1);
}
 
/*
-- 
2.1.0

--
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/

Requesting help in understanding commit 7cccd8, i.e. disabling preemption in slub.c:slab_alloc_node

[Patrick Palka]

Hi everybody,

I am trying to figure out the race condition that commit 7cccd8 fixes.
The commit disables preemption in between the retrieval of the per-cpu
slab and the subsequent read of the slab's tid. According to the
commit message, this change helps avoid allocating from the wrong node
in slab_alloc. But try as I might, I can't see how allocating from the
wrong node, let alone the wrong cpu, could ever happen with or without
preemption. Isn't the globally-unique per-cpu tid the mechanism that's
supposed to guard against allocating from the wrong cpu or node? In
what way does this mechanism fail in slab_alloc_node, and how does
disabling preemption during the retrieval of the tid mitigate this
failure? Would really appreciate if somebody took the time to explain
this to a newbie like me.

Thanks,
Patrick
--
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 -tip 1/4] perf/probe: Improve error message for unknown member of data structure

[Patrick Palka]

On Fri, Jun 6, 2014 at 3:13 AM, Masami Hiramatsu
 wrote:
> Improve the error message if we can not find given member in
> the given structure. Currently perf probe shows a wrong error
> message as below.
>
>   -
>   # perf probe getname_flags:65 "result->BOGUS"
>   result(type:filename) has no member BOGUS.
>   Failed to find 'result' in this function.
> Error: Failed to add events. (-22)
>   -
>
> The first message is correct, but the second one is not, since
> we didn't fail to find a variable but fails to find the member
> of given variable.
>
>   -
>   # perf probe getname_flags:65 "result->BOGUS"
>   result(type:filename) has no member BOGUS.
> Error: Failed to add events. (-22)
>   -
>
> With this patch, the error message shows only the first one.
> And if we really failed to find given variable, it tells us so.
>
>   -
>   # perf probe getname_flags:65 "BOGUS"
>   Failed to find 'BOGUS' in this function.
> Error: Failed to add events. (-2)
>   -
>
> Signed-off-by: Masami Hiramatsu 
> Reported-by: Arnaldo Carvalho de Melo 
> Cc: Peter Zijlstra 
> Cc: Paul Mackerras 
> Cc: Ingo Molnar 
> Cc: Namhyung Kim 
> Cc: Jiri Olsa 
> ---
>  tools/perf/util/probe-finder.c |5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> index 9d8eb26..ce8faf4 100644
> --- a/tools/perf/util/probe-finder.c
> +++ b/tools/perf/util/probe-finder.c
> @@ -573,14 +573,13 @@ static int find_variable(Dwarf_Die *sc_die, struct 
> probe_finder *pf)
> if (!die_find_variable_at(sc_die, pf->pvar->var, pf->addr, &vr_die)) {
> /* Search again in global variables */
> if (!die_find_variable_at(&pf->cu_die, pf->pvar->var, 0, 
> &vr_die))
> +   pr_warning("Failed to find '%s' in this function.\n",
> +  pf->pvar->var);
> ret = -ENOENT;

It looks like you're missing a pair of braces here.
--
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/

[tip:perf/urgent] perf bench: Fix NULL pointer dereference in " perf bench all"

[tip-bot for Patrick Palka]

Commit-ID:  6eeefccdcfc2cc9697562e740bfe6c35fddd4e1c
Gitweb: http://git.kernel.org/tip/6eeefccdcfc2cc9697562e740bfe6c35fddd4e1c
Author: Patrick Palka 
AuthorDate: Wed, 12 Mar 2014 18:40:51 -0400
Committer:  Arnaldo Carvalho de Melo 
CommitDate: Fri, 14 Mar 2014 13:45:54 -0300

perf bench: Fix NULL pointer dereference in "perf bench all"

The for_each_bench() macro must check that the "benchmarks" field of a
collection is not NULL before dereferencing it because the "all"
collection in particular has a NULL "benchmarks" field (signifying that
it has no benchmarks to iterate over).

This fixes this NULL pointer dereference when running "perf bench all":

  [root@ssdandy ~]# perf bench all
  

  # Running mem/memset benchmark...
  # Copying 1MB Bytes ...

 2.453675 GB/Sec
12.056327 GB/Sec (with prefault)

  Segmentation fault (core dumped)
  [root@ssdandy ~]#

Signed-off-by: Patrick Palka 
Cc: Ingo Molnar 
Cc: Paul Mackerras 
Cc: Peter Zijlstra 
Link: 
http://lkml.kernel.org/r/1394664051-6037-1-git-send-email-patr...@parcs.ath.cx
Signed-off-by: Arnaldo Carvalho de Melo 
---
 tools/perf/builtin-bench.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-bench.c b/tools/perf/builtin-bench.c
index e47f90c..8a987d2 100644
--- a/tools/perf/builtin-bench.c
+++ b/tools/perf/builtin-bench.c
@@ -76,7 +76,7 @@ static struct collection collections[] = {
 
 /* Iterate over all benchmarks within a collection: */
 #define for_each_bench(coll, bench) \
-   for (bench = coll->benchmarks; bench->name; bench++)
+   for (bench = coll->benchmarks; bench && bench->name; bench++)
 
 static void dump_benchmarks(struct collection *coll)
 {
--
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] perf bench: Fix NULL pointer dereference in "perf bench all"

[Patrick Palka]

On Thu, Mar 13, 2014 at 10:34 AM, Arnaldo Carvalho de Melo
 wrote:
> Em Wed, Mar 12, 2014 at 06:40:51PM -0400, Patrick Palka escreveu:
>> for_each_bench() must check that the "benchmarks" field of a collection
>> is not NULL before dereferencing it because the "all" collection in
>> particular has a NULL "benchmarks" field (signifying that it has no
>> benchmarks to iterate over).
>>
>> This fixes a NULL pointer dereference when running "perf bench all".
>
> Can you please mention against which tree you're fixing things? I just
> tried to reproduce it here on perf/urgent and this problem is not
> reproducible by simply running:
>
>   perf bench all
>
> So now I'm going to try on perf/core, tip/master, etc :-\

I'm fixing this on top on Linus' tree, which is currently on ac9dc67b7.

Without the patch, the output of "./perf bench all" looks like:

# Running sched/messaging benchmark...
# 20 sender and receiver processes per group
# 10 groups == 400 processes run

 Total time: 0.623 [sec]

# Running sched/pipe benchmark...
# Executed 100 pipe operations between two processes

 Total time: 32.199 [sec]

  32.199419 usecs/op
  31056 ops/sec

# Running mem/memcpy benchmark...
# Copying 1MB Bytes ...

   1.247206 GB/Sec
   1.268263 GB/Sec (with prefault)

# Running mem/memset benchmark...
# Copying 1MB Bytes ...

   1.429813 GB/Sec
   8.418642 GB/Sec (with prefault)

zsh: segmentation fault  ./perf bench all


Patrick
--
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] perf bench: Fix NULL pointer dereference in "perf bench all"

[Patrick Palka]

for_each_bench() must check that the "benchmarks" field of a collection
is not NULL before dereferencing it because the "all" collection in
particular has a NULL "benchmarks" field (signifying that it has no
benchmarks to iterate over).

This fixes a NULL pointer dereference when running "perf bench all".

Signed-off-by: Patrick Palka 
---
 tools/perf/builtin-bench.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-bench.c b/tools/perf/builtin-bench.c
index e47f90c..8a987d2 100644
--- a/tools/perf/builtin-bench.c
+++ b/tools/perf/builtin-bench.c
@@ -76,7 +76,7 @@ static struct collection collections[] = {
 
 /* Iterate over all benchmarks within a collection: */
 #define for_each_bench(coll, bench) \
-   for (bench = coll->benchmarks; bench->name; bench++)
+   for (bench = coll->benchmarks; bench && bench->name; bench++)
 
 static void dump_benchmarks(struct collection *coll)
 {
-- 
1.9.0

--
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 v2] drivers: cpufreq: Mark function as static in cpufreq.c

[Patrick Palka]

On Thu, Feb 27, 2014 at 12:25 AM, Viresh Kumar  wrote:
> Hi Rashika,
>
> On 26 February 2014 22:08, Rashika Kheria  wrote:
>> Mark function as static in cpufreq.c because it is not
>> used outside this file.
>>
>> This eliminates the following warning in cpufreq.c:
>> drivers/cpufreq/cpufreq.c:355:9: warning: no previous prototype for 
>> 'show_boost' [-Wmissing-prototypes]
>
> Can you please elaborate how this warning is related to
> the non-static definition of this function?

-Wmissing-prototypes warns when a non-static function is defined
before a corresponding prototype (usually inside an included header
file) is declared.  In such a case, it is impossible to reference the
non-static function from another file, and therefore the function
should be marked static (usually).  Hope that makes sense!
--
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] ext4: address a benign compiler warning

[Patrick Palka]

When !defined(CONFIG_EXT4_DEBUG), mb_debug() should be defined as a
no_printk() statement instead of an empty statement in order to suppress
the following compiler warning:

fs/ext4/mballoc.c: In function ‘ext4_mb_cleanup_pa’:
fs/ext4/mballoc.c:2659:47: warning: suggest braces around empty body in an ‘if’ 
statement [-Wempty-body]
   mb_debug(1, "mballoc: %u PAs left\n", count);

Signed-off-by: Patrick Palka 
---
 fs/ext4/mballoc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/mballoc.h b/fs/ext4/mballoc.h
index 08481ee..9347328 100644
--- a/fs/ext4/mballoc.h
+++ b/fs/ext4/mballoc.h
@@ -48,7 +48,7 @@ extern ushort ext4_mballoc_debug;
}   \
} while (0)
 #else
-#define mb_debug(n, fmt, a...)
+#define mb_debug(n, fmt, a...) no_printk(fmt, ## a)
 #endif
 
 #define EXT4_MB_HISTORY_ALLOC  1   /* allocation */
-- 
1.9.0.rc3

--
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] ext4: address a benign compiler warning

[Patrick Palka]

On Wed, Feb 12, 2014 at 9:59 PM, Joe Perches  wrote:
> On Wed, 2014-02-12 at 21:13 -0500, Patrick Palka wrote:
>> When !defined(CONFIG_EXT4_DEBUG), mb_debug() should be defined as an
>> empty do-while statement so as to suppress the following compiler
>> warning:
>
> Hello Patrick.
>
>> fs/ext4/mballoc.c: In function 'ext4_mb_cleanup_pa':
>> fs/ext4/mballoc.c:2659:47: warning: suggest braces around empty body in an 
>> 'if' statement [-Wempty-body]
>>mb_debug(1, "mballoc: %u PAs left\n", count);
>> ---
>> diff --git a/fs/ext4/mballoc.h b/fs/ext4/mballoc.h
> []
>> @@ -48,7 +48,7 @@ extern ushort ext4_mballoc_debug;
>>   }   \
>>   } while (0)
>>  #else
>> -#define mb_debug(n, fmt, a...)
>> +#define mb_debug(n, fmt, a...)   do { } while (0)
>
> Ideally, this section should be something like below
> so the !CONFIG_EXT4_DEBUG case still verifies that
> the format and argument types match.
>
> This can help avoid people adding debug statements but
> not compiling with the proper CONFIG_ variable set.
>
> ---
>
> #ifdef CONFIG_EXT4_DEBUG
> extern ushort ext4_mballoc_debug;
>
> #define mb_debug(n, fmt, ...)   \
> do {\
> if ((n) <= ext4_mballoc_debug)  \
> pr_debug(fmt, ##__VA_ARGS__);   \
> }   \
> } while (0)
> #else
> #define mb_debug(n, fmt, ...)   \
> no_printk(KERN_DEBUG fmt, ##__VA_ARGS__)
> #endif
>
>

Hi Joe,

Thanks!  I was not aware of that idiom.  It makes a good amount of
sense.  I'm going to send a revised version of the patch shortly.

Patrick
--
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] ext4: address a benign compiler warning

[Patrick Palka]

When !defined(CONFIG_EXT4_DEBUG), mb_debug() should be defined as an
empty do-while statement so as to suppress the following compiler
warning:

fs/ext4/mballoc.c: In function ‘ext4_mb_cleanup_pa’:
fs/ext4/mballoc.c:2659:47: warning: suggest braces around empty body in an ‘if’ 
statement [-Wempty-body]
   mb_debug(1, "mballoc: %u PAs left\n", count);

Signed-off-by: Patrick Palka 
---
 fs/ext4/mballoc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/mballoc.h b/fs/ext4/mballoc.h
index 08481ee..92eeb98 100644
--- a/fs/ext4/mballoc.h
+++ b/fs/ext4/mballoc.h
@@ -48,7 +48,7 @@ extern ushort ext4_mballoc_debug;
}   \
} while (0)
 #else
-#define mb_debug(n, fmt, a...)
+#define mb_debug(n, fmt, a...) do { } while (0)
 #endif
 
 #define EXT4_MB_HISTORY_ALLOC  1   /* allocation */
-- 
1.9.0.rc3

--
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: #pragma once?

[Patrick Palka]

On Mon, Jan 6, 2014 at 3:47 PM, Josh Triplett  wrote:
> Does anyone have any objection to the use of "#pragma once" instead of
> the usual #ifndef-#define-...-#endif include guard?  GCC, LLVM/clang,
> and the latest Sparse all support either method just fine.  (I added
> support to Sparse myself.)  Both have equivalent performance.  "#pragma
> once" is simpler, and avoids the possibility of a typo in the defined
> guard symbol.

Unfortunately in GCC #pragma once is slower and slightly buggier than
regular include guards:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52566
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58770
--
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/

[tip:perf/urgent] perf ui tui progress: Don' t force a refresh during progress update

[tip-bot for Patrick Palka]

Commit-ID:  d53e57d039c323fe3a43630e9f729df48134e2c9
Gitweb: http://git.kernel.org/tip/d53e57d039c323fe3a43630e9f729df48134e2c9
Author: Patrick Palka 
AuthorDate: Fri, 25 Oct 2013 20:25:49 -0400
Committer:  Arnaldo Carvalho de Melo 
CommitDate: Mon, 11 Nov 2013 15:56:39 -0300

perf ui tui progress: Don't force a refresh during progress update

Each call to tui_progress__update() would forcibly refresh the entire
screen.  This is somewhat inefficient and causes noticable flickering
during the startup of perf-report, especially on large/slow terminals.

It looks like the force-refresh in tui_progress__update() serves no
purpose other than to clear the screen so that the progress bar of a
previous operation does not subsume that of a subsequent operation.  But
we can do just that in a much more efficient manner by clearing only the
region that a previous progress bar may have occupied before repainting
the new progress bar.  Then the force-refresh could be removed with no
change in visuals.

This patch disables the slow force-refresh in tui_progress__update() and
instead calls SLsmg_fill_region() on the entire area that the progress
bar may occupy before repainting it.  This change makes the startup of
perf-report much faster and appear much "smoother".

It turns out that this was a big bottleneck in the startup speed of
perf-report -- with this patch, perf-report starts up ~2x faster (1.1s
vs 0.55s) on my machines.  (These numbers were measured by running "time
perf report" on an 8MB perf.data and pressing 'q' immediately.)

Signed-off-by: Patrick Palka 
Acked-by: Ingo Molnar 
Cc: Ingo Molnar 
Cc: Namhyung Kim 
Cc: Paul Mackerras 
Cc: Peter Zijlstra 
Link: 
http://lkml.kernel.org/r/1382747149-9716-1-git-send-email-patr...@parcs.ath.cx
Signed-off-by: Arnaldo Carvalho de Melo 
---
 tools/perf/ui/tui/progress.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/ui/tui/progress.c b/tools/perf/ui/tui/progress.c
index 3e2d936..c61d14b 100644
--- a/tools/perf/ui/tui/progress.c
+++ b/tools/perf/ui/tui/progress.c
@@ -18,13 +18,14 @@ static void tui_progress__update(struct ui_progress *p)
if (p->total == 0)
return;
 
-   ui__refresh_dimensions(true);
+   ui__refresh_dimensions(false);
pthread_mutex_lock(&ui__lock);
y = SLtt_Screen_Rows / 2 - 2;
SLsmg_set_color(0);
SLsmg_draw_box(y, 0, 3, SLtt_Screen_Cols);
SLsmg_gotorc(y++, 1);
SLsmg_write_string((char *)p->title);
+   SLsmg_fill_region(y, 1, 1, SLtt_Screen_Cols - 2, ' ');
SLsmg_set_color(HE_COLORSET_SELECTED);
bar = ((SLtt_Screen_Cols - 2) * p->curr) / p->total;
SLsmg_fill_region(y, 1, 1, bar, ' ');
--
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 rebase] perf/ui/tui: don't force a refresh during progress update

[Patrick Palka]

Each call to tui_progress__update() would forcibly refresh the entire
screen.  This is somewhat inefficient and causes noticable flickering
during the startup of perf-report, especially on large/slow terminals.

It looks like the force-refresh in tui_progress__update() serves no
purpose other than to clear the screen so that the progress bar of a
previous operation does not subsume that of a subsequent operation.
But we can do just that in a much more efficient manner by clearing only
the region that a previous progress bar may have occupied before
repainting the new progress bar.  Then the force-refresh could be
removed with no change in visuals.

This patch disables the slow force-refresh in tui_progress__update() and
instead calls SLsmg_fill_region() on the entire area that the progress
bar may occupy before repainting it.  This change makes the startup of
perf-report much faster and appear much "smoother".

It turns out that this was a big bottleneck in the startup speed of
perf-report -- with this patch, perf-report starts up ~2x faster (1.1s
vs 0.55s) on my machines.  (These numbers were measured by running
"time perf report" on an 8MB perf.data and pressing 'q' immediately.)

Cc: Peter Zijlstra 
Cc: Paul Mackerras 
Cc: Arnaldo Carvalho de Melo 
Cc: Namhyung Kim 
Cc: Ingo Molnar 
Acked-by: Ingo Molnar 
Signed-off-by: Patrick Palka 
---
Rebased against
  git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git perf/core

And also CC'd two more relevant persons.

 tools/perf/ui/tui/progress.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/ui/tui/progress.c b/tools/perf/ui/tui/progress.c
index 3e2d936..c61d14b 100644
--- a/tools/perf/ui/tui/progress.c
+++ b/tools/perf/ui/tui/progress.c
@@ -18,13 +18,14 @@ static void tui_progress__update(struct ui_progress *p)
if (p->total == 0)
return;
 
-   ui__refresh_dimensions(true);
+   ui__refresh_dimensions(false);
pthread_mutex_lock(&ui__lock);
y = SLtt_Screen_Rows / 2 - 2;
SLsmg_set_color(0);
SLsmg_draw_box(y, 0, 3, SLtt_Screen_Cols);
SLsmg_gotorc(y++, 1);
SLsmg_write_string((char *)p->title);
+   SLsmg_fill_region(y, 1, 1, SLtt_Screen_Cols - 2, ' ');
SLsmg_set_color(HE_COLORSET_SELECTED);
bar = ((SLtt_Screen_Cols - 2) * p->curr) / p->total;
SLsmg_fill_region(y, 1, 1, bar, ' ');
-- 
1.8.4.rc3

--
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] perf: Fix library detection when building without libelf

[Patrick Palka]

On Thu, Oct 24, 2013 at 4:13 PM, Arnaldo Carvalho de Melo
 wrote:
> Em Wed, Oct 23, 2013 at 10:51:09PM -0400, Patrick Palka escreveu:
>> When I attempt to build perf on a system with slang but without libelf,
>> 'make' would wrongly complain that the slang library could not be found.
>>
>> It turns out that this was happening because we are not filtering -lelf
>> from EXTLIBS early enough.  As a result, the library test for slang
>> (ditto for gtk, libaudit, etc) erroneously passes -lelf to try-cc, which
>> of course fails on a system without libelf.
>>
>> This patch makes the filtering of -lelf from EXTLIBS occur right after
>> testing for libelf support, so that the subsequent library tests will
>> not erroneously pass -lelf to try-cc when building without libelf
>> support.
>
> Can you please check with the perf/core branch in my tree or Ingo's?
>
> git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git perf/core
>
> or Ingo's:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git perf/core
>
> The feature detection was reworked and perhaps it is fixed there.
>
> - Arnaldo

I can't reproduce the detection failure in your branch, and a quick inspection
shows that the new feature-detection code is not subject to the kind of bug
that my patch fixed. So consider my patch obsolete.

Thanks,
Patrick
--
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] perf: Fix library detection when building without libelf

[Patrick Palka]

When I attempt to build perf on a system with slang but without libelf,
'make' would wrongly complain that the slang library could not be found.

It turns out that this was happening because we are not filtering -lelf
from EXTLIBS early enough.  As a result, the library test for slang
(ditto for gtk, libaudit, etc) erroneously passes -lelf to try-cc, which
of course fails on a system without libelf.

This patch makes the filtering of -lelf from EXTLIBS occur right after
testing for libelf support, so that the subsequent library tests will
not erroneously pass -lelf to try-cc when building without libelf
support.

Cc: Peter Zijlstra 
Cc: Paul Mackerras 
Cc: Ingo Molnar 
Cc: Arnaldo Carvalho de Melo 
Cc: Jiri Olsa 
Signed-off-by: Patrick Palka 
---
 tools/perf/Makefile| 2 --
 tools/perf/config/Makefile | 4 
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index 64c043b..94635c8 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -443,8 +443,6 @@ ifneq ($(OUTPUT),)
 endif
 
 ifdef NO_LIBELF
-EXTLIBS := $(filter-out -lelf,$(EXTLIBS))
-
 # Remove ELF/DWARF dependent codes
 LIB_OBJS := $(filter-out $(OUTPUT)util/symbol-elf.o,$(LIB_OBJS))
 LIB_OBJS := $(filter-out $(OUTPUT)util/dwarf-aux.o,$(LIB_OBJS))
diff --git a/tools/perf/config/Makefile b/tools/perf/config/Makefile
index 5f6f9b3..1748767 100644
--- a/tools/perf/config/Makefile
+++ b/tools/perf/config/Makefile
@@ -174,6 +174,10 @@ else
 endif # SOURCE_LIBELF
 endif # NO_LIBELF
 
+ifdef NO_LIBELF
+EXTLIBS := $(filter-out -lelf,$(EXTLIBS))
+endif
+
 ifndef NO_LIBELF
 CFLAGS += -DLIBELF_SUPPORT
 FLAGS_LIBELF=$(CFLAGS) $(LDFLAGS) $(EXTLIBS)
-- 
1.8.4.rc3

--
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] perf/ui/tui: don't force a refresh during progress update

[Patrick Palka]

Each call to tui_progress__update() would forcibly refresh the entire
screen.  This is somewhat inefficient and causes noticable flickering
during the startup of perf-report, especially on large/slow terminals.

It looks like the force-refresh in tui_progress__update() serves no
purpose other than to clear the screen so that the progress bar of a
previous operation does not subsume with that of a subsequent operation.
But we can do just that in a much more efficient manner by clearing only
the region that a previous progress bar may have occupied before
repainting the new progress bar.  Then the force-refresh could be
removed with no change in visuals.

This patch disables the slow force-refresh in tui_progress__update() and
instead calls SLsmg_fill_region() on the entire area that the progress
bar may occupy before repainting it.  This change makes the startup of
perf-report much faster and appear much "smoother".

It turns out that this was a big bottleneck in the startup speed of
perf-report -- with this patch, perf-report starts up ~1.6x faster (0.8s
vs 0.5s) on my machines.  (These numbers were measured by running "time
perf report" on an 8MB perf.data and pressing 'q' immediately.)

Cc: Peter Zijlstra 
Cc: Paul Mackerras 
Cc: Ingo Molnar 
Signed-off-by: Patrick Palka 
---
 tools/perf/ui/tui/progress.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/ui/tui/progress.c b/tools/perf/ui/tui/progress.c
index 6c2184d..641049a 100644
--- a/tools/perf/ui/tui/progress.c
+++ b/tools/perf/ui/tui/progress.c
@@ -17,13 +17,14 @@ static void tui_progress__update(u64 curr, u64 total, const 
char *title)
if (total == 0)
return;
 
-   ui__refresh_dimensions(true);
+   ui__refresh_dimensions(false);
pthread_mutex_lock(&ui__lock);
y = SLtt_Screen_Rows / 2 - 2;
SLsmg_set_color(0);
SLsmg_draw_box(y, 0, 3, SLtt_Screen_Cols);
SLsmg_gotorc(y++, 1);
SLsmg_write_string((char *)title);
+   SLsmg_fill_region(y, 1, 1, SLtt_Screen_Cols - 2, ' ');
SLsmg_set_color(HE_COLORSET_SELECTED);
bar = ((SLtt_Screen_Cols - 2) * curr) / total;
SLsmg_fill_region(y, 1, 1, bar, ' ');
-- 
1.8.4.rc3

--
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] time: Fix signedness bug in sysfs_get_uname() and its callers

[Patrick Palka]

On Fri, Oct 18, 2013 at 8:16 PM, John Stultz  wrote:
> On 10/11/2013 10:11 AM, Patrick Palka wrote:
>> sysfs_get_uname() is erroneously declared as returning size_t even
>> though it may return a negative value, specifically -EINVAL.  Its
>> callers then check whether its return value is less than zero and indeed
>> that is never the case for size_t.
>>
>> This patch changes sysfs_get_uname() to return ssize_t and makes sure
>> its callers use ssize_t accordingly.
>
> So a similar fix has already been queued in tip/timers/core, but this
> seems more complete, so I've resolved the collisions with the earlier
> fix and queued it for 3.13.
>
> Would you please take a look at the resulting commit and double check I
> didn't flub the conflict resolution?
>
> https://git.linaro.org/gitweb?p=people/jstultz/linux.git;a=commitdiff;h=891292a767c2453af0e5be9465e95b06b4b29ebe;hp=b7bc50e45111e59419474154736f419a555158d9

Looks 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] time: Fix signedness bug in sysfs_get_uname() and its callers

[Patrick Palka]

sysfs_get_uname() is erroneously declared as returning size_t even
though it may return a negative value, specifically -EINVAL.  Its
callers then check whether its return value is less than zero and indeed
that is never the case for size_t.

This patch changes sysfs_get_uname() to return ssize_t and makes sure
its callers use ssize_t accordingly.

Signed-off-by: Patrick Palka 
---
 kernel/time/clockevents.c   | 2 +-
 kernel/time/clocksource.c   | 6 +++---
 kernel/time/tick-internal.h | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index 38959c8..30554b9 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -584,7 +584,7 @@ static ssize_t sysfs_unbind_tick_dev(struct device *dev,
 const char *buf, size_t count)
 {
char name[CS_NAME_LEN];
-   size_t ret = sysfs_get_uname(buf, name, count);
+   ssize_t ret = sysfs_get_uname(buf, name, count);
struct clock_event_device *ce;
 
if (ret < 0)
diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
index 50a8736..b286f42 100644
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -893,7 +893,7 @@ sysfs_show_current_clocksources(struct device *dev,
return count;
 }
 
-size_t sysfs_get_uname(const char *buf, char *dst, size_t cnt)
+ssize_t sysfs_get_uname(const char *buf, char *dst, size_t cnt)
 {
size_t ret = cnt;
 
@@ -924,7 +924,7 @@ static ssize_t sysfs_override_clocksource(struct device 
*dev,
  struct device_attribute *attr,
  const char *buf, size_t count)
 {
-   size_t ret;
+   ssize_t ret;
 
mutex_lock(&clocksource_mutex);
 
@@ -952,7 +952,7 @@ static ssize_t sysfs_unbind_clocksource(struct device *dev,
 {
struct clocksource *cs;
char name[CS_NAME_LEN];
-   size_t ret;
+   ssize_t ret;
 
ret = sysfs_get_uname(buf, name, count);
if (ret < 0)
diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h
index bc906ca..18e71f7 100644
--- a/kernel/time/tick-internal.h
+++ b/kernel/time/tick-internal.h
@@ -31,7 +31,7 @@ extern void tick_install_replacement(struct 
clock_event_device *dev);
 
 extern void clockevents_shutdown(struct clock_event_device *dev);
 
-extern size_t sysfs_get_uname(const char *buf, char *dst, size_t cnt);
+extern ssize_t sysfs_get_uname(const char *buf, char *dst, size_t cnt);
 
 /*
  * NO_HZ / high resolution timer shared code
-- 
1.8.4.rc3

--
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/