* [PATCH] refscale: Optimize process_durations()
@ 2023-10-28 17:04 Christophe JAILLET
2023-10-28 21:30 ` Steven Rostedt
2023-10-30 16:55 ` Davidlohr Bueso
0 siblings, 2 replies; 7+ messages in thread
From: Christophe JAILLET @ 2023-10-28 17:04 UTC (permalink / raw
To: Davidlohr Bueso, Paul E. McKenney, Josh Triplett,
Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes, Boqun Feng,
Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, Zqiang
Cc: linux-kernel, kernel-janitors, Christophe JAILLET, rcu
process_durations() is not a hot path, but there is no good reason to
iterate over and over the data already in 'buf'.
Using a seq_buf saves some useless strcat() and the need of a temp buffer.
Data is written directly at the correct place.
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
kernel/rcu/refscale.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/kernel/rcu/refscale.c b/kernel/rcu/refscale.c
index 2c2648a3ad30..861485d865ec 100644
--- a/kernel/rcu/refscale.c
+++ b/kernel/rcu/refscale.c
@@ -28,6 +28,7 @@
#include <linux/rcupdate_trace.h>
#include <linux/reboot.h>
#include <linux/sched.h>
+#include <linux/seq_buf.h>
#include <linux/spinlock.h>
#include <linux/smp.h>
#include <linux/stat.h>
@@ -890,31 +891,36 @@ static u64 process_durations(int n)
{
int i;
struct reader_task *rt;
- char buf1[64];
+ struct seq_buf s;
char *buf;
u64 sum = 0;
buf = kmalloc(800 + 64, GFP_KERNEL);
if (!buf)
return 0;
- buf[0] = 0;
+
+ seq_buf_init(&s, buf, 800 + 64);
+
sprintf(buf, "Experiment #%d (Format: <THREAD-NUM>:<Total loop time in ns>)",
exp_idx);
for (i = 0; i < n && !torture_must_stop(); i++) {
rt = &(reader_tasks[i]);
- sprintf(buf1, "%d: %llu\t", i, rt->last_duration_ns);
if (i % 5 == 0)
- strcat(buf, "\n");
- if (strlen(buf) >= 800) {
+ seq_buf_putc(&s, '\n');
+
+ if (seq_buf_used(&s) >= 800) {
+ seq_buf_terminate(&s);
pr_alert("%s", buf);
- buf[0] = 0;
+ seq_buf_clear(&s);
}
- strcat(buf, buf1);
+
+ seq_buf_printf(&s, "%d: %llu\t", i, rt->last_duration_ns);
sum += rt->last_duration_ns;
}
+ seq_buf_terminate(&s);
pr_alert("%s\n", buf);
kfree(buf);
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] refscale: Optimize process_durations()
2023-10-28 17:04 [PATCH] refscale: Optimize process_durations() Christophe JAILLET
@ 2023-10-28 21:30 ` Steven Rostedt
2023-10-30 16:55 ` Davidlohr Bueso
1 sibling, 0 replies; 7+ messages in thread
From: Steven Rostedt @ 2023-10-28 21:30 UTC (permalink / raw
To: Christophe JAILLET
Cc: Davidlohr Bueso, Paul E. McKenney, Josh Triplett,
Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes, Boqun Feng,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, linux-kernel,
kernel-janitors, rcu
On Sat, 28 Oct 2023 19:04:44 +0200
Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
> process_durations() is not a hot path, but there is no good reason to
> iterate over and over the data already in 'buf'.
>
> Using a seq_buf saves some useless strcat() and the need of a temp buffer.
> Data is written directly at the correct place.
>
Agreed.
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> kernel/rcu/refscale.c | 20 +++++++++++++-------
> 1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/rcu/refscale.c b/kernel/rcu/refscale.c
> index 2c2648a3ad30..861485d865ec 100644
> --- a/kernel/rcu/refscale.c
> +++ b/kernel/rcu/refscale.c
> @@ -28,6 +28,7 @@
> #include <linux/rcupdate_trace.h>
> #include <linux/reboot.h>
> #include <linux/sched.h>
> +#include <linux/seq_buf.h>
> #include <linux/spinlock.h>
> #include <linux/smp.h>
> #include <linux/stat.h>
> @@ -890,31 +891,36 @@ static u64 process_durations(int n)
> {
> int i;
> struct reader_task *rt;
> - char buf1[64];
> + struct seq_buf s;
> char *buf;
> u64 sum = 0;
>
> buf = kmalloc(800 + 64, GFP_KERNEL);
> if (!buf)
> return 0;
> - buf[0] = 0;
> +
> + seq_buf_init(&s, buf, 800 + 64);
> +
> sprintf(buf, "Experiment #%d (Format: <THREAD-NUM>:<Total loop time in ns>)",
> exp_idx);
>
> for (i = 0; i < n && !torture_must_stop(); i++) {
> rt = &(reader_tasks[i]);
> - sprintf(buf1, "%d: %llu\t", i, rt->last_duration_ns);
>
> if (i % 5 == 0)
> - strcat(buf, "\n");
> - if (strlen(buf) >= 800) {
> + seq_buf_putc(&s, '\n');
I was confused here thinking it was originally adding a '\n' to buf1 on
i % 5, but it's adding it to buf!
Yeah, using seq_buf is much less confusing and then less error prone.
Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
-- Steve
> +
> + if (seq_buf_used(&s) >= 800) {
> + seq_buf_terminate(&s);
> pr_alert("%s", buf);
> - buf[0] = 0;
> + seq_buf_clear(&s);
> }
> - strcat(buf, buf1);
> +
> + seq_buf_printf(&s, "%d: %llu\t", i, rt->last_duration_ns);
>
> sum += rt->last_duration_ns;
> }
> + seq_buf_terminate(&s);
> pr_alert("%s\n", buf);
>
> kfree(buf);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] refscale: Optimize process_durations()
2023-10-28 17:04 [PATCH] refscale: Optimize process_durations() Christophe JAILLET
2023-10-28 21:30 ` Steven Rostedt
@ 2023-10-30 16:55 ` Davidlohr Bueso
2023-10-31 18:21 ` Paul E. McKenney
1 sibling, 1 reply; 7+ messages in thread
From: Davidlohr Bueso @ 2023-10-30 16:55 UTC (permalink / raw
To: Christophe JAILLET
Cc: Paul E. McKenney, Josh Triplett, Frederic Weisbecker,
Neeraj Upadhyay, Joel Fernandes, Boqun Feng, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, linux-kernel,
kernel-janitors, rcu
On Sat, 28 Oct 2023, Christophe JAILLET wrote:
>process_durations() is not a hot path, but there is no good reason to
>iterate over and over the data already in 'buf'.
>
>Using a seq_buf saves some useless strcat() and the need of a temp buffer.
>Data is written directly at the correct place.
Makes sense.
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] refscale: Optimize process_durations()
2023-10-30 16:55 ` Davidlohr Bueso
@ 2023-10-31 18:21 ` Paul E. McKenney
2023-10-31 22:47 ` Paul E. McKenney
0 siblings, 1 reply; 7+ messages in thread
From: Paul E. McKenney @ 2023-10-31 18:21 UTC (permalink / raw
To: Davidlohr Bueso
Cc: Christophe JAILLET, Josh Triplett, Frederic Weisbecker,
Neeraj Upadhyay, Joel Fernandes, Boqun Feng, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, linux-kernel,
kernel-janitors, rcu
On Mon, Oct 30, 2023 at 09:55:16AM -0700, Davidlohr Bueso wrote:
> On Sat, 28 Oct 2023, Christophe JAILLET wrote:
>
> > process_durations() is not a hot path, but there is no good reason to
> > iterate over and over the data already in 'buf'.
> >
> > Using a seq_buf saves some useless strcat() and the need of a temp buffer.
> > Data is written directly at the correct place.
>
> Makes sense.
>
> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Queued and pushed, thank you all!
Thanx, Paul
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] refscale: Optimize process_durations()
2023-10-31 18:21 ` Paul E. McKenney
@ 2023-10-31 22:47 ` Paul E. McKenney
2023-11-01 7:41 ` Christophe JAILLET
0 siblings, 1 reply; 7+ messages in thread
From: Paul E. McKenney @ 2023-10-31 22:47 UTC (permalink / raw
To: Christophe JAILLET
Cc: Davidlohr Bueso, Josh Triplett, Frederic Weisbecker,
Neeraj Upadhyay, Joel Fernandes, Boqun Feng, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, linux-kernel,
kernel-janitors, rcu
On Tue, Oct 31, 2023 at 11:21:14AM -0700, Paul E. McKenney wrote:
> On Mon, Oct 30, 2023 at 09:55:16AM -0700, Davidlohr Bueso wrote:
> > On Sat, 28 Oct 2023, Christophe JAILLET wrote:
> >
> > > process_durations() is not a hot path, but there is no good reason to
> > > iterate over and over the data already in 'buf'.
> > >
> > > Using a seq_buf saves some useless strcat() and the need of a temp buffer.
> > > Data is written directly at the correct place.
> >
> > Makes sense.
> >
> > Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
>
> Queued and pushed, thank you all!
But an allmodconfig build complains about seq_buf_putc() being undefined,
that is, not exported. I suspect that other seq_buf_*() functions in
this patch might also be complained about.
I am dropping this for the moment. Please make it pass an allmodconfig
build so that I can pull it in again. Please see below for the commit.
Thanx, Paul
------------------------------------------------------------------------
commit a1ef9b4cff53c509f412c354c715449d7f2e159b
Author: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Date: Sat Oct 28 19:04:44 2023 +0200
refscale: Optimize process_durations()
The process_durations() function is not on a hot path, but there is
still no good reason to iterate over and over the data already in 'buf',
but this is exactly what the current use of strlen() and strcat() do.
Using a seq_buf saves some useless strcat() and the need of a temp buffer.
Data is written directly at the correct place.
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
diff --git a/kernel/rcu/refscale.c b/kernel/rcu/refscale.c
index 2c2648a3ad30..861485d865ec 100644
--- a/kernel/rcu/refscale.c
+++ b/kernel/rcu/refscale.c
@@ -28,6 +28,7 @@
#include <linux/rcupdate_trace.h>
#include <linux/reboot.h>
#include <linux/sched.h>
+#include <linux/seq_buf.h>
#include <linux/spinlock.h>
#include <linux/smp.h>
#include <linux/stat.h>
@@ -890,31 +891,36 @@ static u64 process_durations(int n)
{
int i;
struct reader_task *rt;
- char buf1[64];
+ struct seq_buf s;
char *buf;
u64 sum = 0;
buf = kmalloc(800 + 64, GFP_KERNEL);
if (!buf)
return 0;
- buf[0] = 0;
+
+ seq_buf_init(&s, buf, 800 + 64);
+
sprintf(buf, "Experiment #%d (Format: <THREAD-NUM>:<Total loop time in ns>)",
exp_idx);
for (i = 0; i < n && !torture_must_stop(); i++) {
rt = &(reader_tasks[i]);
- sprintf(buf1, "%d: %llu\t", i, rt->last_duration_ns);
if (i % 5 == 0)
- strcat(buf, "\n");
- if (strlen(buf) >= 800) {
+ seq_buf_putc(&s, '\n');
+
+ if (seq_buf_used(&s) >= 800) {
+ seq_buf_terminate(&s);
pr_alert("%s", buf);
- buf[0] = 0;
+ seq_buf_clear(&s);
}
- strcat(buf, buf1);
+
+ seq_buf_printf(&s, "%d: %llu\t", i, rt->last_duration_ns);
sum += rt->last_duration_ns;
}
+ seq_buf_terminate(&s);
pr_alert("%s\n", buf);
kfree(buf);
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] refscale: Optimize process_durations()
2023-10-31 22:47 ` Paul E. McKenney
@ 2023-11-01 7:41 ` Christophe JAILLET
2023-11-01 10:24 ` Paul E. McKenney
0 siblings, 1 reply; 7+ messages in thread
From: Christophe JAILLET @ 2023-11-01 7:41 UTC (permalink / raw
To: paulmck
Cc: Davidlohr Bueso, Josh Triplett, Frederic Weisbecker,
Neeraj Upadhyay, Joel Fernandes, Boqun Feng, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, linux-kernel,
kernel-janitors, rcu, Kees Cook
Le 31/10/2023 à 23:47, Paul E. McKenney a écrit :
> On Tue, Oct 31, 2023 at 11:21:14AM -0700, Paul E. McKenney wrote:
>> On Mon, Oct 30, 2023 at 09:55:16AM -0700, Davidlohr Bueso wrote:
>>> On Sat, 28 Oct 2023, Christophe JAILLET wrote:
>>>
>>>> process_durations() is not a hot path, but there is no good reason to
>>>> iterate over and over the data already in 'buf'.
>>>>
>>>> Using a seq_buf saves some useless strcat() and the need of a temp buffer.
>>>> Data is written directly at the correct place.
>>>
>>> Makes sense.
>>>
>>> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
>>
>> Queued and pushed, thank you all!
>
> But an allmodconfig build complains about seq_buf_putc() being undefined,
> that is, not exported. I suspect that other seq_buf_*() functions in
> this patch might also be complained about.
>
> I am dropping this for the moment. Please make it pass an allmodconfig
> build so that I can pull it in again. Please see below for the commit.
>
> Thanx, Paul
>
Ouch!
seq_buf_init(), seq_buf_terminate(), seq_buf_clear() are inlined
functions in a .h file, so shouldn't be a problem.
seq_buf_printf() is exported, but seq_buf_putc() is not!
Really odd to me.
Kees Cook (added in cc) suggests to use this API (see [1]) to avoid some
potential issues and ease the management of NULL terminated strings in
buffers. (#LinuxHardening).
I'll propose to add the missing EXPORT_SYMBOL_GPL.
CJ
[1]: https://lore.kernel.org/all/202310241629.0A4206316F@keescook/
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] refscale: Optimize process_durations()
2023-11-01 7:41 ` Christophe JAILLET
@ 2023-11-01 10:24 ` Paul E. McKenney
0 siblings, 0 replies; 7+ messages in thread
From: Paul E. McKenney @ 2023-11-01 10:24 UTC (permalink / raw
To: Christophe JAILLET
Cc: Davidlohr Bueso, Josh Triplett, Frederic Weisbecker,
Neeraj Upadhyay, Joel Fernandes, Boqun Feng, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, linux-kernel,
kernel-janitors, rcu, Kees Cook
On Wed, Nov 01, 2023 at 08:41:39AM +0100, Christophe JAILLET wrote:
> Le 31/10/2023 à 23:47, Paul E. McKenney a écrit :
> > On Tue, Oct 31, 2023 at 11:21:14AM -0700, Paul E. McKenney wrote:
> > > On Mon, Oct 30, 2023 at 09:55:16AM -0700, Davidlohr Bueso wrote:
> > > > On Sat, 28 Oct 2023, Christophe JAILLET wrote:
> > > >
> > > > > process_durations() is not a hot path, but there is no good reason to
> > > > > iterate over and over the data already in 'buf'.
> > > > >
> > > > > Using a seq_buf saves some useless strcat() and the need of a temp buffer.
> > > > > Data is written directly at the correct place.
> > > >
> > > > Makes sense.
> > > >
> > > > Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
> > >
> > > Queued and pushed, thank you all!
> >
> > But an allmodconfig build complains about seq_buf_putc() being undefined,
> > that is, not exported. I suspect that other seq_buf_*() functions in
> > this patch might also be complained about.
> >
> > I am dropping this for the moment. Please make it pass an allmodconfig
> > build so that I can pull it in again. Please see below for the commit.
>
> Ouch!
Believe me, I know that feeling! ;-)
> seq_buf_init(), seq_buf_terminate(), seq_buf_clear() are inlined functions
> in a .h file, so shouldn't be a problem.
>
> seq_buf_printf() is exported, but seq_buf_putc() is not!
> Really odd to me.
>
> Kees Cook (added in cc) suggests to use this API (see [1]) to avoid some
> potential issues and ease the management of NULL terminated strings in
> buffers. (#LinuxHardening).
>
> I'll propose to add the missing EXPORT_SYMBOL_GPL.
Very good, looking forward to seeing the result.
Thanx, Paul
> CJ
>
> [1]: https://lore.kernel.org/all/202310241629.0A4206316F@keescook/
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-11-01 10:24 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-28 17:04 [PATCH] refscale: Optimize process_durations() Christophe JAILLET
2023-10-28 21:30 ` Steven Rostedt
2023-10-30 16:55 ` Davidlohr Bueso
2023-10-31 18:21 ` Paul E. McKenney
2023-10-31 22:47 ` Paul E. McKenney
2023-11-01 7:41 ` Christophe JAILLET
2023-11-01 10:24 ` Paul E. McKenney
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).