[PATCH 0/7] kernel-trace: Fine-tuning for seven function implementations

[PATCH 0/7] kernel-trace: Fine-tuning for seven function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 5 May 2017 22:50:05 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (7):
  Combine two function calls into one in hist_trigger_entry_print()
  Replace five seq_puts() calls by seq_putc()
  Adjust two checks for null pointers in compatible_field()
  Improve a size determination in create_hist_field()
  Replace two seq_printf() calls by seq_puts() in probes_seq_show()
  Adjust two checks for null pointers in uprobe_buffer_init()
  Delete an error message for a failed memory allocation in create_trace_uprobe()

 kernel/trace/trace_events_hist.c | 18 ++++++++----------
 kernel/trace/trace_uprobe.c      | 10 ++++------
 2 files changed, 12 insertions(+), 16 deletions(-)

-- 
2.12.2

[PATCH 1/7] kernel-trace: Combine two function calls into one in hist_trigger_entry_print()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 5 May 2017 19:40:39 +0200

A bit of data was put into a sequence by two separate function calls.
Print the same data by a single function call instead.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 kernel/trace/trace_events_hist.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 1c21d0e2a145..94999934ffd5 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1017,7 +1017,5 @@ hist_trigger_entry_print(struct seq_file *m,
 
-	seq_puts(m, "}");
-
-	seq_printf(m, " hitcount: %10llu",
+	seq_printf(m, "} hitcount: %10llu",
 		   tracing_map_read_sum(elt, HITCOUNT_IDX));
 
 	for (i = 1; i < hist_data->n_vals; i++) {
-- 
2.12.2

[PATCH 2/7] kernel-trace: Replace five seq_puts() calls by seq_putc()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 5 May 2017 19:45:12 +0200

Five single characters should be put into a sequence.
Thus use the corresponding function "seq_putc".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 kernel/trace/trace_events_hist.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 94999934ffd5..df566e21344d 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1013,7 +1013,7 @@ hist_trigger_entry_print(struct seq_file *m,
 	}
 
 	if (!multiline)
-		seq_puts(m, " ");
+		seq_putc(m, ' ');
 
 	seq_printf(m, "} hitcount: %10llu",
 		   tracing_map_read_sum(elt, HITCOUNT_IDX));
@@ -1030,7 +1030,7 @@ hist_trigger_entry_print(struct seq_file *m,
 		}
 	}
 
-	seq_puts(m, "\n");
+	seq_putc(m, '\n');
 }
 
 static int print_entries(struct seq_file *m,
@@ -1168,7 +1168,7 @@ static int event_hist_trigger_print(struct seq_file *m,
 		key_field = hist_data->fields[i];
 
 		if (i > hist_data->n_vals)
-			seq_puts(m, ",");
+			seq_putc(m, ',');
 
 		if (key_field->flags & HIST_FIELD_FL_STACKTRACE)
 			seq_puts(m, "stacktrace");
@@ -1182,7 +1182,7 @@ static int event_hist_trigger_print(struct seq_file *m,
 		if (i == HITCOUNT_IDX)
 			seq_puts(m, "hitcount");
 		else {
-			seq_puts(m, ",");
+			seq_putc(m, ',');
 			hist_field_print(m, hist_data->fields[i]);
 		}
 	}
@@ -1195,7 +1195,7 @@ static int event_hist_trigger_print(struct seq_file *m,
 		sort_key = &hist_data->sort_keys[i];
 
 		if (i > 0)
-			seq_puts(m, ",");
+			seq_putc(m, ',');
 
 		if (sort_key->field_idx == HITCOUNT_IDX)
 			seq_puts(m, "hitcount");
-- 
2.12.2

[PATCH 3/7] kernel-trace: Adjust two checks for null pointers in compatible_field()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 5 May 2017 20:00:11 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The script “checkpatch.pl” pointed information out like the following.

Comparison to NULL could be written !…

Thus fix the affected source code place.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 kernel/trace/trace_events_hist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index df566e21344d..36412deac24c 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1324,7 +1324,7 @@ static bool compatible_field(struct ftrace_event_field *field,
 {
 	if (field == test_field)
 		return true;
-	if (field == NULL || test_field == NULL)
+	if (!field || !test_field)
 		return false;
 	if (strcmp(field->name, test_field->name) != 0)
 		return false;
-- 
2.12.2

Re: [PATCH 3/7] kernel-trace: Adjust two checks for null pointers in compatible_field()

[Steven Rostedt]

On Fri, 5 May 2017 23:03:23 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 5 May 2017 20:00:11 +0200
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> The script “checkpatch.pl” pointed information out like the following.
> 
> Comparison to NULL could be written !…
> 
> Thus fix the affected source code place.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  kernel/trace/trace_events_hist.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/trace_events_hist.c
> b/kernel/trace/trace_events_hist.c index df566e21344d..36412deac24c
> 100644 --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -1324,7 +1324,7 @@ static bool compatible_field(struct
> ftrace_event_field *field, {
>  	if (field == test_field)
>  		return true;
> -	if (field == NULL || test_field == NULL)
> +	if (!field || !test_field)

The first two are fine, this one isn't needed.

-- Steve

>  		return false;
>  	if (strcmp(field->name, test_field->name) != 0)
>  		return false;

[PATCH 4/7] kernel-trace: Improve a size determination in create_hist_field()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 5 May 2017 20:15:46 +0200

Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 kernel/trace/trace_events_hist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 36412deac24c..a75223572374 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -353,7 +353,7 @@ static struct hist_field *create_hist_field(struct ftrace_event_field *field,
 	if (field && is_function_field(field))
 		return NULL;
 
-	hist_field = kzalloc(sizeof(struct hist_field), GFP_KERNEL);
+	hist_field = kzalloc(sizeof(*hist_field), GFP_KERNEL);
 	if (!hist_field)
 		return NULL;
 
-- 
2.12.2

[PATCH 5/7] kernel-trace: Replace two seq_printf() calls by seq_puts() in probes_seq_show()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 5 May 2017 20:30:05 +0200

Two strings which did not contain data format specifications should be put
into a sequence. Thus use the corresponding function "seq_puts".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 kernel/trace/trace_uprobe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index a7581fec9681..3e662d7ea6a4 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -612,11 +612,11 @@ static int probes_seq_show(struct seq_file *m, void *v)
 	} else {
 		switch (sizeof(void *)) {
 		case 4:
-			seq_printf(m, "0x00000000");
+			seq_puts(m, "0x00000000");
 			break;
 		case 8:
 		default:
-			seq_printf(m, "0x0000000000000000");
+			seq_puts(m, "0x0000000000000000");
 			break;
 		}
 	}
-- 
2.12.2

Re: [PATCH 0/7] kernel-trace: Fine-tuning for seven function implementations

[Steven Rostedt]

On Fri, 5 May 2017 23:00:41 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 5 May 2017 22:50:05 +0200
> 
> A few update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (7):

Hi Markus,

Just to let you know, it's never a good idea to send new patches out
during the merge window. They are most likely to be missed and
forgotten during this time. Unless they are critical bug fixes, it's
best to wait till after a merge window.

I wont be touching or even looking at these until after 4.12-rc1 is
released. Feel free to reply to this email with a ping in a week or two.

Thanks,

-- Steve

>   Combine two function calls into one in hist_trigger_entry_print()
>   Replace five seq_puts() calls by seq_putc()
>   Adjust two checks for null pointers in compatible_field()
>   Improve a size determination in create_hist_field()
>   Replace two seq_printf() calls by seq_puts() in probes_seq_show()
>   Adjust two checks for null pointers in uprobe_buffer_init()
>   Delete an error message for a failed memory allocation in create_trace_uprobe()
> 
>  kernel/trace/trace_events_hist.c | 18 ++++++++----------
>  kernel/trace/trace_uprobe.c      | 10 ++++------
>  2 files changed, 12 insertions(+), 16 deletions(-)
> 

Re: kernel-trace: Fine-tuning for seven function implementations

[SF Markus Elfring]

> Just to let you know, it's never a good idea to send new patches out
> during the merge window.

Thanks for such information.


> They are most likely to be missed and forgotten during this time.

This can happen then occasionally.


> Unless they are critical bug fixes, it's best to wait till after a merge window.

I am curious if the involved software developers will be interested at all
to consider update suggestions from this small patch series.


> I wont be touching or even looking at these until after 4.12-rc1 is released.

This is also fine.

How are the chances to pick related software development challenges up
when you find that the time will be better?


> Feel free to reply to this email with a ping in a week or two.

Would any source code reviewers like to send also a reminder then
besides looking into any waiting queues in other information systems?

Regards,
Markus

Re: kernel-trace: Fine-tuning for seven function implementations

[SF Markus Elfring]

> I wont be touching or even looking at these until after 4.12-rc1 is released.
> Feel free to reply to this email with a ping in a week or two.

*ping*

How do you think about to give these update suggestions another look?

Regards,
Markus

Re: kernel-trace: Fine-tuning for seven function implementations

[Steven Rostedt]

On Mon, 22 May 2017 09:45:27 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> > I wont be touching or even looking at these until after 4.12-rc1 is
> > released. Feel free to reply to this email with a ping in a week or
> > two.  
> 
> *ping*
> 
> How do you think about to give these update suggestions another look?
>

The conversions of the seq_printf()s and friends are fine, but the rest
are not worth the churn. Yes checkpatch may complain about the NULL
compares and the kallocs(), but that's for new code, not for code that
has already been accepted.

-- Steve

[PATCH 6/7] kernel-trace: Adjust two checks for null pointers in uprobe_buffer_init()

[SF Markus Elfring]

Am 05.05.2017 um 23:00 schrieb SF Markus Elfring:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 5 May 2017 22:50:05 +0200
> 
> A few update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (7):
>   Combine two function calls into one in hist_trigger_entry_print()
>   Replace five seq_puts() calls by seq_putc()
>   Adjust two checks for null pointers in compatible_field()
>   Improve a size determination in create_hist_field()
>   Replace two seq_printf() calls by seq_puts() in probes_seq_show()
>   Adjust two checks for null pointers in uprobe_buffer_init()
>   Delete an error message for a failed memory allocation in create_trace_uprobe()
> 
>  kernel/trace/trace_events_hist.c | 18 ++++++++----------
>  kernel/trace/trace_uprobe.c      | 10 ++++------
>  2 files changed, 12 insertions(+), 16 deletions(-)
> 

[PATCH 6/7] kernel-trace: Adjust two checks for null pointers in uprobe_buffer_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 5 May 2017 22:30:16 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The script “checkpatch.pl” pointed information out like the following.

Comparison to NULL could be written !…

Thus fix the affected source code place.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 kernel/trace/trace_uprobe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 3e662d7ea6a4..e9c1865f7397 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -705,13 +705,13 @@ static int uprobe_buffer_init(void)
 	int cpu, err_cpu;
 
 	uprobe_cpu_buffer = alloc_percpu(struct uprobe_cpu_buffer);
-	if (uprobe_cpu_buffer == NULL)
+	if (!uprobe_cpu_buffer)
 		return -ENOMEM;
 
 	for_each_possible_cpu(cpu) {
 		struct page *p = alloc_pages_node(cpu_to_node(cpu),
 						  GFP_KERNEL, 0);
-		if (p == NULL) {
+		if (!p) {
 			err_cpu = cpu;
 			goto err;
 		}
-- 
2.12.2

[PATCH 7/7] kernel-trace: Delete an error message for a failed memory allocation in create_trace_uprobe()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 5 May 2017 22:38:51 +0200

The script "checkpatch.pl" pointed information out like the following.

WARNING: Possible unnecessary 'out of memory' message

Thus remove such a statement here.

Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 kernel/trace/trace_uprobe.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index e9c1865f7397..d19fa2a6cc7b 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -490,9 +490,7 @@ static int create_trace_uprobe(int argc, char **argv)
 	tu->offset = offset;
 	tu->inode = inode;
 	tu->filename = kstrdup(filename, GFP_KERNEL);
-
 	if (!tu->filename) {
-		pr_info("Failed to allocate filename.\n");
 		ret = -ENOMEM;
 		goto error;
 	}
-- 
2.12.2