[Suggestion] kernel/rcutorture.c: about using scnprintf() instead of sprintf().

[Suggestion] kernel/rcutorture.c: about using scnprintf() instead of sprintf().

[Chen Gang]

Hello Maintainers:

In srcu_torture_stats(), if cpus are more than 1K, the PAGE_SIZE will
not be enough.

In rcu_torture_printk(), the 'page' maximized size is 4096, it has a
function pointer for printing, which not tell its maximized length.


Welcome any additional suggestions or completions.


Thanks.
-- 
Chen Gang

Re: [Suggestion] kernel/rcutorture.c: about using scnprintf() instead of sprintf().

[Paul E. McKenney]

On Tue, Oct 08, 2013 at 04:32:53PM +0800, Chen Gang wrote:
> Hello Maintainers:
> 
> In srcu_torture_stats(), if cpus are more than 1K, the PAGE_SIZE will
> not be enough.
> 
> In rcu_torture_printk(), the 'page' maximized size is 4096, it has a
> function pointer for printing, which not tell its maximized length.
> 
> Welcome any additional suggestions or completions.

I never have run rcutorture on a system with that many CPUs.  ;-)

Given that rcutorture is not used in production, my approach would be to
fix this when I encountered it.  But what change would you suggest, and,
more importantly, how would you go about testing it before submitting
a patch?

Or if you are simply reporting this as a bug, please let me know that.

							Thanx, Paul

Re: [Suggestion] kernel/rcutorture.c: about using scnprintf() instead of sprintf().

[Chen Gang]

On 10/13/2013 07:05 PM, Paul E. McKenney wrote:
> On Tue, Oct 08, 2013 at 04:32:53PM +0800, Chen Gang wrote:
>> Hello Maintainers:
>>
>> In srcu_torture_stats(), if cpus are more than 1K, the PAGE_SIZE will
>> not be enough.
>>
>> In rcu_torture_printk(), the 'page' maximized size is 4096, it has a
>> function pointer for printing, which not tell its maximized length.
>>
>> Welcome any additional suggestions or completions.
> 
> I never have run rcutorture on a system with that many CPUs.  ;-)
> 

I guess most of members (include me), never have run under that case.


> Given that rcutorture is not used in production, my approach would be to
> fix this when I encountered it.  But what change would you suggest, and,
> more importantly, how would you go about testing it before submitting
> a patch?
> 

OK, thanks, I will/should give a fix and test.

Hmm, In my opinion, we need:

 - let it pass LTP common simple test (so I can know how to test it).

 - intend to shrink maximized buffer (PAGE_SIZE -> 64, 256 ..) for test.

 - read your original mail again (about testing contents) as reference.

Excuse me, I have to do some other things of company, so I will/should
try to finish it within this week (2013-10-20), if this time point is
not quite suitable, please let me know, thanks.


> Or if you are simply reporting this as a bug, please let me know that.
> 

I will/should do: in q4 of 2013, I will/should spend part of my time
resources on testing.



Welcome any additional suggestions or completions.

Thanks.
-- 
Chen Gang

Re: [Suggestion] kernel/rcutorture.c: about using scnprintf() instead of sprintf().

[Chen Gang]

On 10/14/2013 09:41 AM, Chen Gang wrote:
> On 10/13/2013 07:05 PM, Paul E. McKenney wrote:
>> On Tue, Oct 08, 2013 at 04:32:53PM +0800, Chen Gang wrote:
>>> Hello Maintainers:
>>>
>>> In srcu_torture_stats(), if cpus are more than 1K, the PAGE_SIZE will
>>> not be enough.
>>>
>>> In rcu_torture_printk(), the 'page' maximized size is 4096, it has a
>>> function pointer for printing, which not tell its maximized length.
>>>
>>> Welcome any additional suggestions or completions.
>>
>> I never have run rcutorture on a system with that many CPUs.  ;-)
>>
> 
> I guess most of members (include me), never have run under that case.
> 
> 
>> Given that rcutorture is not used in production, my approach would be to
>> fix this when I encountered it.  But what change would you suggest, and,
>> more importantly, how would you go about testing it before submitting
>> a patch?
>>
> 
> OK, thanks, I will/should give a fix and test.
> 
> Hmm, In my opinion, we need:
> 
>  - let it pass LTP common simple test (so I can know how to test it).
> 

Oh, after read "Documentation/RCU/torture.txt", it is a test module, so
except related special test (e.g. shrink maximized buffer), it seems not
need additional common test for it (e.g. LTP).

>  - intend to shrink maximized buffer (PAGE_SIZE -> 64, 256 ..) for test.
> 
>  - read your original mail again (about testing contents) as reference.
> 
> Excuse me, I have to do some other things of company, so I will/should
> try to finish it within this week (2013-10-20), if this time point is
> not quite suitable, please let me know, thanks.
> 
> 
>> Or if you are simply reporting this as a bug, please let me know that.
>>
> 
> I will/should do: in q4 of 2013, I will/should spend part of my time
> resources on testing.
> 
> 
> 
> Welcome any additional suggestions or completions.
> 
> Thanks.
> 


-- 
Chen Gang

Re: [Suggestion] kernel/rcutorture.c: about using scnprintf() instead of sprintf().

[Paul E. McKenney]

On Mon, Oct 14, 2013 at 10:22:20AM +0800, Chen Gang wrote:
> On 10/14/2013 09:41 AM, Chen Gang wrote:
> > On 10/13/2013 07:05 PM, Paul E. McKenney wrote:
> >> On Tue, Oct 08, 2013 at 04:32:53PM +0800, Chen Gang wrote:
> >>> Hello Maintainers:
> >>>
> >>> In srcu_torture_stats(), if cpus are more than 1K, the PAGE_SIZE will
> >>> not be enough.
> >>>
> >>> In rcu_torture_printk(), the 'page' maximized size is 4096, it has a
> >>> function pointer for printing, which not tell its maximized length.
> >>>
> >>> Welcome any additional suggestions or completions.
> >>
> >> I never have run rcutorture on a system with that many CPUs.  ;-)
> > 
> > I guess most of members (include me), never have run under that case.

Indeed, there are not yet very many people with systems that large.

> >> Given that rcutorture is not used in production, my approach would be to
> >> fix this when I encountered it.  But what change would you suggest, and,
> >> more importantly, how would you go about testing it before submitting
> >> a patch?
> >>
> > 
> > OK, thanks, I will/should give a fix and test.
> > 
> > Hmm, In my opinion, we need:
> > 
> >  - let it pass LTP common simple test (so I can know how to test it).
> > 
> 
> Oh, after read "Documentation/RCU/torture.txt", it is a test module, so
> except related special test (e.g. shrink maximized buffer), it seems not
> need additional common test for it (e.g. LTP).

Yep!

> >  - intend to shrink maximized buffer (PAGE_SIZE -> 64, 256 ..) for test.

This is a good start!  However, you should also test the original kernel
to be sure that it really fails.  You should of course start by asking
yourself how you would expect it to fail.

What other change should you make to test this in order to be sure that
it will really work when someone tries it on 1024 CPUs?  (I am asking
rather than telling because you really need to have this stuff worked
out on your initial submission.)

Another way of thinking about this is to ask yourself the question "If
someone ran rcutorture with torture_type=srcu on a system with 1024 CPUs
(to say nothing of 4096 CPUs), what would they want to happen?"  Then:
"How would I test for that on a smaller system?"

> >  - read your original mail again (about testing contents) as reference.
> > 
> > Excuse me, I have to do some other things of company, so I will/should
> > try to finish it within this week (2013-10-20), if this time point is
> > not quite suitable, please let me know, thanks.
> > 
> >> Or if you are simply reporting this as a bug, please let me know that.
> > 
> > I will/should do: in q4 of 2013, I will/should spend part of my time
> > resources on testing.

OK, sounds good.

> > Welcome any additional suggestions or completions.

Please see above.

							Thanx, Paul

> > Thanks.
> > 
> 
> 
> -- 
> Chen Gang
> 

Re: [Suggestion] kernel/rcutorture.c: about using scnprintf() instead of sprintf().

[Chen Gang]

On 10/14/2013 07:24 PM, Paul E. McKenney wrote:
> On Mon, Oct 14, 2013 at 10:22:20AM +0800, Chen Gang wrote:
>>>  - intend to shrink maximized buffer (PAGE_SIZE -> 64, 256 ..) for test.
> 
> This is a good start!  However, you should also test the original kernel
> to be sure that it really fails.  You should of course start by asking
> yourself how you would expect it to fail.
> 

When shrink size, for scnprintf() we already got truncation, that means
if we still use sprintf(), it will cause memory overflow.

For memory overflow, it does not means OS will stop quickly, one of the
worst condition is "it may still seems going 'well' but sometimes
may/will show some amazing things which is not easy to find root cause".

So for this kind of memory overflow, "shrinking size and letting
scnprintf() instead of sprintf() to see whether truncation" is well enough.


> What other change should you make to test this in order to be sure that
> it will really work when someone tries it on 1024 CPUs?  (I am asking
> rather than telling because you really need to have this stuff worked
> out on your initial submission.)
> 

Hmm... Can it really work on 1024 CPUs? sorry I don't know. But in fact,
that is not about this patch (it is just one of case which may cause
issues).

This patch is only about "use truncation instead of memory overflow, and
make sure the modification without negative effect". So in my opinion,
current test case is enough for this requirement.

Maybe you want to let this module get additional test to find additional
issues? (If so, I will try when my time resource is possible).


> Another way of thinking about this is to ask yourself the question "If
> someone ran rcutorture with torture_type=srcu on a system with 1024 CPUs
> (to say nothing of 4096 CPUs), what would they want to happen?"  Then:
> "How would I test for that on a smaller system?"
> 

We have to face the efficiency: it is only a test module, if the tester
(assume he/she is a programmer, too) notices about the truncation, they
can simply extend the maximize length to avoid truncation.

At least for me, "rcutorture.c" is really easy enough for a programmer
to test rcu (and also, it is really a useful tool for test rcu).

And for "1024 CPUs",  I think one of efficient way is: "when some
members use it under 1024 CPUs, if they find something can be
improvement, they can report/send related patch".


Hmm... maybe the comment "it is ... additional test or consideration"
need be removed: 'additional' and 'consideration' are not suitable words
to be appeared in comments (they are not precise word).


Thanks.
-- 
Chen Gang

Re: [Suggestion] kernel/rcutorture.c: about using scnprintf() instead of sprintf().

[Paul E. McKenney]

On Tue, Oct 15, 2013 at 09:40:40AM +0800, Chen Gang wrote:
> On 10/14/2013 07:24 PM, Paul E. McKenney wrote:
> > On Mon, Oct 14, 2013 at 10:22:20AM +0800, Chen Gang wrote:
> >>>  - intend to shrink maximized buffer (PAGE_SIZE -> 64, 256 ..) for test.
> > 
> > This is a good start!  However, you should also test the original kernel
> > to be sure that it really fails.  You should of course start by asking
> > yourself how you would expect it to fail.
> 
> When shrink size, for scnprintf() we already got truncation, that means
> if we still use sprintf(), it will cause memory overflow.
> 
> For memory overflow, it does not means OS will stop quickly, one of the
> worst condition is "it may still seems going 'well' but sometimes
> may/will show some amazing things which is not easy to find root cause".
> 
> So for this kind of memory overflow, "shrinking size and letting
> scnprintf() instead of sprintf() to see whether truncation" is well enough.
> 
> > What other change should you make to test this in order to be sure that
> > it will really work when someone tries it on 1024 CPUs?  (I am asking
> > rather than telling because you really need to have this stuff worked
> > out on your initial submission.)
> 
> Hmm... Can it really work on 1024 CPUs? sorry I don't know. But in fact,
> that is not about this patch (it is just one of case which may cause
> issues).
> 
> This patch is only about "use truncation instead of memory overflow, and
> make sure the modification without negative effect". So in my opinion,
> current test case is enough for this requirement.

Yes, the patch is about "use truncation instead of memory overflow",
but the truncation would also be a problem on large systems.  Is it
possible to prevent memory from overflow in the first place?

> Maybe you want to let this module get additional test to find additional
> issues? (If so, I will try when my time resource is possible).
> 
> > Another way of thinking about this is to ask yourself the question "If
> > someone ran rcutorture with torture_type=srcu on a system with 1024 CPUs
> > (to say nothing of 4096 CPUs), what would they want to happen?"  Then:
> > "How would I test for that on a smaller system?"
> 
> We have to face the efficiency: it is only a test module, if the tester
> (assume he/she is a programmer, too) notices about the truncation, they
> can simply extend the maximize length to avoid truncation.

True.  But you can make a change that is just as simple that allows you
to test what would happen on a 1024-CPU system even though your own
system has only 2 CPUs.  Can you see what that change is?

							Thanx, Paul

> At least for me, "rcutorture.c" is really easy enough for a programmer
> to test rcu (and also, it is really a useful tool for test rcu).
> 
> And for "1024 CPUs",  I think one of efficient way is: "when some
> members use it under 1024 CPUs, if they find something can be
> improvement, they can report/send related patch".
> 
> 
> Hmm... maybe the comment "it is ... additional test or consideration"
> need be removed: 'additional' and 'consideration' are not suitable words
> to be appeared in comments (they are not precise word).
> 
> 
> Thanks.
> -- 
> Chen Gang
> 

Re: [Suggestion] kernel/rcutorture.c: about using scnprintf() instead of sprintf().

[Chen Gang]

On 10/15/2013 04:31 PM, Paul E. McKenney wrote:
> On Tue, Oct 15, 2013 at 09:40:40AM +0800, Chen Gang wrote:
>> > Hmm... Can it really work on 1024 CPUs? sorry I don't know. But in fact,
>> > that is not about this patch (it is just one of case which may cause
>> > issues).
>> > 
>> > This patch is only about "use truncation instead of memory overflow, and
>> > make sure the modification without negative effect". So in my opinion,
>> > current test case is enough for this requirement.
> Yes, the patch is about "use truncation instead of memory overflow",
> but the truncation would also be a problem on large systems.  Is it
> possible to prevent memory from overflow in the first place?
> 

Yes of cause it can, but in my opinion, for a test module, truncating
information is acceptable, but memory overflow is not acceptable.

In fact, every information truncation case, can be "prevented memory
from overflow in the first place".  It depends on whether we have enough
interest to do that.


>> > We have to face the efficiency: it is only a test module, if the tester
>> > (assume he/she is a programmer, too) notices about the truncation, they
>> > can simply extend the maximize length to avoid truncation.
> True.  But you can make a change that is just as simple that allows you
> to test what would happen on a 1024-CPU system even though your own
> system has only 2 CPUs.  Can you see what that change is?

In fact, from modifying code, we can virtual most of cases, if current
test case is enough, we need not do that (leave it for guys who have
real 1024-CPUs).


Thanks.
-- 
Chen Gang

[PATCH] kernel/rcutorture.c: use scnprintf() instead of sprintf()

[Chen Gang]

If the contents is more than 4096 bytes (e.g. if have 1K cpus), current
sprintf() will cause memory overflow.

They are all test information which can be truncated, so use scnprintf()
instead of sprintf(), also add 'max' parameter for related functions,
also notice 80 columns boundary and parameters alignments.

Test case:

  Fedora16 x86_64, 2 CPUs, 2GB RAM, [in/rm]mod with "torture_type=srcu".

    let maximize buffer to 256 to truncate in rcu_torture_printk().
    let maximize buffer to 410 to may truncate in srcu_torture_stats().
    let maximize buffer to 4096 (original size) to print full.

  it is a rcu test module, so not need additional test or consideration.


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 kernel/rcutorture.c |  110 +++++++++++++++++++++++++++-----------------------
 1 files changed, 59 insertions(+), 51 deletions(-)

diff --git a/kernel/rcutorture.c b/kernel/rcutorture.c
index be63101..107fd76 100644
--- a/kernel/rcutorture.c
+++ b/kernel/rcutorture.c
@@ -370,7 +370,7 @@ struct rcu_torture_ops {
 	void (*call)(struct rcu_head *head, void (*func)(struct rcu_head *rcu));
 	void (*cb_barrier)(void);
 	void (*fqs)(void);
-	int (*stats)(char *page);
+	int (*stats)(char *page, int max);
 	int irq_capable;
 	int can_boost;
 	const char *name;
@@ -572,20 +572,20 @@ static void srcu_torture_barrier(void)
 	srcu_barrier(&srcu_ctl);
 }
 
-static int srcu_torture_stats(char *page)
+static int srcu_torture_stats(char *page, int max)
 {
 	int cnt = 0;
 	int cpu;
 	int idx = srcu_ctl.completed & 0x1;
 
-	cnt += sprintf(&page[cnt], "%s%s per-CPU(idx=%d):",
-		       torture_type, TORTURE_FLAG, idx);
+	cnt += scnprintf(&page[cnt], max - cnt, "%s%s per-CPU(idx=%d):",
+			torture_type, TORTURE_FLAG, idx);
 	for_each_possible_cpu(cpu) {
-		cnt += sprintf(&page[cnt], " %d(%lu,%lu)", cpu,
-			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[!idx],
-			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[idx]);
+		cnt += scnprintf(&page[cnt], max - cnt, " %d(%lu,%lu)", cpu,
+				per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[!idx],
+				per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[idx]);
 	}
-	cnt += sprintf(&page[cnt], "\n");
+	cnt += scnprintf(&page[cnt], max - cnt, "\n");
 	return cnt;
 }
 
@@ -1047,7 +1047,7 @@ rcu_torture_reader(void *arg)
  * Create an RCU-torture statistics message in the specified buffer.
  */
 static int
-rcu_torture_printk(char *page)
+rcu_torture_printk(char *page, int max)
 {
 	int cnt = 0;
 	int cpu;
@@ -1065,61 +1065,69 @@ rcu_torture_printk(char *page)
 		if (pipesummary[i] != 0)
 			break;
 	}
-	cnt += sprintf(&page[cnt], "%s%s ", torture_type, TORTURE_FLAG);
-	cnt += sprintf(&page[cnt],
-		       "rtc: %p ver: %lu tfle: %d rta: %d rtaf: %d rtf: %d ",
-		       rcu_torture_current,
-		       rcu_torture_current_version,
-		       list_empty(&rcu_torture_freelist),
-		       atomic_read(&n_rcu_torture_alloc),
-		       atomic_read(&n_rcu_torture_alloc_fail),
-		       atomic_read(&n_rcu_torture_free));
-	cnt += sprintf(&page[cnt], "rtmbe: %d rtbke: %ld rtbre: %ld ",
-		       atomic_read(&n_rcu_torture_mberror),
-		       n_rcu_torture_boost_ktrerror,
-		       n_rcu_torture_boost_rterror);
-	cnt += sprintf(&page[cnt], "rtbf: %ld rtb: %ld nt: %ld ",
-		       n_rcu_torture_boost_failure,
-		       n_rcu_torture_boosts,
-		       n_rcu_torture_timers);
-	cnt += sprintf(&page[cnt],
-		       "onoff: %ld/%ld:%ld/%ld %d,%d:%d,%d %lu:%lu (HZ=%d) ",
-		       n_online_successes, n_online_attempts,
-		       n_offline_successes, n_offline_attempts,
-		       min_online, max_online,
-		       min_offline, max_offline,
-		       sum_online, sum_offline, HZ);
-	cnt += sprintf(&page[cnt], "barrier: %ld/%ld:%ld",
-		       n_barrier_successes,
-		       n_barrier_attempts,
-		       n_rcu_torture_barrier_error);
-	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
+	cnt += scnprintf(&page[cnt], max - cnt, "%s%s ",
+			torture_type, TORTURE_FLAG);
+	cnt += scnprintf(&page[cnt], max - cnt,
+			"rtc: %p ver: %lu tfle: %d rta: %d rtaf: %d rtf: %d ",
+			rcu_torture_current,
+			rcu_torture_current_version,
+			list_empty(&rcu_torture_freelist),
+			atomic_read(&n_rcu_torture_alloc),
+			atomic_read(&n_rcu_torture_alloc_fail),
+			atomic_read(&n_rcu_torture_free));
+	cnt += scnprintf(&page[cnt], max - cnt,
+			"rtmbe: %d rtbke: %ld rtbre: %ld ",
+			atomic_read(&n_rcu_torture_mberror),
+			n_rcu_torture_boost_ktrerror,
+			n_rcu_torture_boost_rterror);
+	cnt += scnprintf(&page[cnt], max - cnt,
+			"rtbf: %ld rtb: %ld nt: %ld ",
+			n_rcu_torture_boost_failure,
+			n_rcu_torture_boosts,
+			n_rcu_torture_timers);
+	cnt += scnprintf(&page[cnt], max - cnt,
+			"onoff: %ld/%ld:%ld/%ld %d,%d:%d,%d %lu:%lu (HZ=%d) ",
+			n_online_successes, n_online_attempts,
+			n_offline_successes, n_offline_attempts,
+			min_online, max_online,
+			min_offline, max_offline,
+			sum_online, sum_offline, HZ);
+	cnt += scnprintf(&page[cnt], max - cnt,
+			"barrier: %ld/%ld:%ld",
+			n_barrier_successes,
+			n_barrier_attempts,
+			n_rcu_torture_barrier_error);
+	cnt += scnprintf(&page[cnt], max - cnt, "\n%s%s ",
+			torture_type, TORTURE_FLAG);
 	if (atomic_read(&n_rcu_torture_mberror) != 0 ||
 	    n_rcu_torture_barrier_error != 0 ||
 	    n_rcu_torture_boost_ktrerror != 0 ||
 	    n_rcu_torture_boost_rterror != 0 ||
 	    n_rcu_torture_boost_failure != 0 ||
 	    i > 1) {
-		cnt += sprintf(&page[cnt], "!!! ");
+		cnt += scnprintf(&page[cnt], max - cnt, "!!! ");
 		atomic_inc(&n_rcu_torture_error);
 		WARN_ON_ONCE(1);
 	}
-	cnt += sprintf(&page[cnt], "Reader Pipe: ");
+	cnt += scnprintf(&page[cnt], max - cnt, "Reader Pipe: ");
 	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
-		cnt += sprintf(&page[cnt], " %ld", pipesummary[i]);
-	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
-	cnt += sprintf(&page[cnt], "Reader Batch: ");
+		cnt += scnprintf(&page[cnt], max - cnt, " %ld", pipesummary[i]);
+	cnt += scnprintf(&page[cnt], max - cnt, "\n%s%s ",
+			torture_type, TORTURE_FLAG);
+	cnt += scnprintf(&page[cnt], max - cnt, "Reader Batch: ");
 	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
-		cnt += sprintf(&page[cnt], " %ld", batchsummary[i]);
-	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
-	cnt += sprintf(&page[cnt], "Free-Block Circulation: ");
+		cnt += scnprintf(&page[cnt], max - cnt, " %ld",
+				batchsummary[i]);
+	cnt += scnprintf(&page[cnt], max - cnt, "\n%s%s ",
+			torture_type, TORTURE_FLAG);
+	cnt += scnprintf(&page[cnt], max - cnt, "Free-Block Circulation: ");
 	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++) {
-		cnt += sprintf(&page[cnt], " %d",
-			       atomic_read(&rcu_torture_wcount[i]));
+		cnt += scnprintf(&page[cnt], max - cnt, " %d",
+				atomic_read(&rcu_torture_wcount[i]));
 	}
-	cnt += sprintf(&page[cnt], "\n");
+	cnt += scnprintf(&page[cnt], max - cnt, "\n");
 	if (cur_ops->stats)
-		cnt += cur_ops->stats(&page[cnt]);
+		cnt += cur_ops->stats(&page[cnt], max - cnt);
 	return cnt;
 }
 
@@ -1136,7 +1144,7 @@ rcu_torture_stats_print(void)
 {
 	int cnt;
 
-	cnt = rcu_torture_printk(printk_buf);
+	cnt = rcu_torture_printk(printk_buf, sizeof(printk_buf));
 	pr_alert("%s", printk_buf);
 }
 
-- 
1.7.7.6

Re: [PATCH] kernel/rcutorture.c: use scnprintf() instead of sprintf()

[Paul E. McKenney]

On Mon, Oct 14, 2013 at 04:38:55PM +0800, Chen Gang wrote:
> If the contents is more than 4096 bytes (e.g. if have 1K cpus), current
> sprintf() will cause memory overflow.
> 
> They are all test information which can be truncated, so use scnprintf()
> instead of sprintf(), also add 'max' parameter for related functions,
> also notice 80 columns boundary and parameters alignments.
> 
> Test case:
> 
>   Fedora16 x86_64, 2 CPUs, 2GB RAM, [in/rm]mod with "torture_type=srcu".
> 
>     let maximize buffer to 256 to truncate in rcu_torture_printk().
>     let maximize buffer to 410 to may truncate in srcu_torture_stats().
>     let maximize buffer to 4096 (original size) to print full.
> 
>   it is a rcu test module, so not need additional test or consideration.
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>

At first glance, not a bad start.

Suppose that your goal was to make it avoid truncation.  What would you
do differently?

							Thanx, Paul

> ---
>  kernel/rcutorture.c |  110 +++++++++++++++++++++++++++-----------------------
>  1 files changed, 59 insertions(+), 51 deletions(-)
> 
> diff --git a/kernel/rcutorture.c b/kernel/rcutorture.c
> index be63101..107fd76 100644
> --- a/kernel/rcutorture.c
> +++ b/kernel/rcutorture.c
> @@ -370,7 +370,7 @@ struct rcu_torture_ops {
>  	void (*call)(struct rcu_head *head, void (*func)(struct rcu_head *rcu));
>  	void (*cb_barrier)(void);
>  	void (*fqs)(void);
> -	int (*stats)(char *page);
> +	int (*stats)(char *page, int max);
>  	int irq_capable;
>  	int can_boost;
>  	const char *name;
> @@ -572,20 +572,20 @@ static void srcu_torture_barrier(void)
>  	srcu_barrier(&srcu_ctl);
>  }
> 
> -static int srcu_torture_stats(char *page)
> +static int srcu_torture_stats(char *page, int max)
>  {
>  	int cnt = 0;
>  	int cpu;
>  	int idx = srcu_ctl.completed & 0x1;
> 
> -	cnt += sprintf(&page[cnt], "%s%s per-CPU(idx=%d):",
> -		       torture_type, TORTURE_FLAG, idx);
> +	cnt += scnprintf(&page[cnt], max - cnt, "%s%s per-CPU(idx=%d):",
> +			torture_type, TORTURE_FLAG, idx);
>  	for_each_possible_cpu(cpu) {
> -		cnt += sprintf(&page[cnt], " %d(%lu,%lu)", cpu,
> -			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[!idx],
> -			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[idx]);
> +		cnt += scnprintf(&page[cnt], max - cnt, " %d(%lu,%lu)", cpu,
> +				per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[!idx],
> +				per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[idx]);
>  	}
> -	cnt += sprintf(&page[cnt], "\n");
> +	cnt += scnprintf(&page[cnt], max - cnt, "\n");
>  	return cnt;
>  }
> 
> @@ -1047,7 +1047,7 @@ rcu_torture_reader(void *arg)
>   * Create an RCU-torture statistics message in the specified buffer.
>   */
>  static int
> -rcu_torture_printk(char *page)
> +rcu_torture_printk(char *page, int max)
>  {
>  	int cnt = 0;
>  	int cpu;
> @@ -1065,61 +1065,69 @@ rcu_torture_printk(char *page)
>  		if (pipesummary[i] != 0)
>  			break;
>  	}
> -	cnt += sprintf(&page[cnt], "%s%s ", torture_type, TORTURE_FLAG);
> -	cnt += sprintf(&page[cnt],
> -		       "rtc: %p ver: %lu tfle: %d rta: %d rtaf: %d rtf: %d ",
> -		       rcu_torture_current,
> -		       rcu_torture_current_version,
> -		       list_empty(&rcu_torture_freelist),
> -		       atomic_read(&n_rcu_torture_alloc),
> -		       atomic_read(&n_rcu_torture_alloc_fail),
> -		       atomic_read(&n_rcu_torture_free));
> -	cnt += sprintf(&page[cnt], "rtmbe: %d rtbke: %ld rtbre: %ld ",
> -		       atomic_read(&n_rcu_torture_mberror),
> -		       n_rcu_torture_boost_ktrerror,
> -		       n_rcu_torture_boost_rterror);
> -	cnt += sprintf(&page[cnt], "rtbf: %ld rtb: %ld nt: %ld ",
> -		       n_rcu_torture_boost_failure,
> -		       n_rcu_torture_boosts,
> -		       n_rcu_torture_timers);
> -	cnt += sprintf(&page[cnt],
> -		       "onoff: %ld/%ld:%ld/%ld %d,%d:%d,%d %lu:%lu (HZ=%d) ",
> -		       n_online_successes, n_online_attempts,
> -		       n_offline_successes, n_offline_attempts,
> -		       min_online, max_online,
> -		       min_offline, max_offline,
> -		       sum_online, sum_offline, HZ);
> -	cnt += sprintf(&page[cnt], "barrier: %ld/%ld:%ld",
> -		       n_barrier_successes,
> -		       n_barrier_attempts,
> -		       n_rcu_torture_barrier_error);
> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
> +	cnt += scnprintf(&page[cnt], max - cnt, "%s%s ",
> +			torture_type, TORTURE_FLAG);
> +	cnt += scnprintf(&page[cnt], max - cnt,
> +			"rtc: %p ver: %lu tfle: %d rta: %d rtaf: %d rtf: %d ",
> +			rcu_torture_current,
> +			rcu_torture_current_version,
> +			list_empty(&rcu_torture_freelist),
> +			atomic_read(&n_rcu_torture_alloc),
> +			atomic_read(&n_rcu_torture_alloc_fail),
> +			atomic_read(&n_rcu_torture_free));
> +	cnt += scnprintf(&page[cnt], max - cnt,
> +			"rtmbe: %d rtbke: %ld rtbre: %ld ",
> +			atomic_read(&n_rcu_torture_mberror),
> +			n_rcu_torture_boost_ktrerror,
> +			n_rcu_torture_boost_rterror);
> +	cnt += scnprintf(&page[cnt], max - cnt,
> +			"rtbf: %ld rtb: %ld nt: %ld ",
> +			n_rcu_torture_boost_failure,
> +			n_rcu_torture_boosts,
> +			n_rcu_torture_timers);
> +	cnt += scnprintf(&page[cnt], max - cnt,
> +			"onoff: %ld/%ld:%ld/%ld %d,%d:%d,%d %lu:%lu (HZ=%d) ",
> +			n_online_successes, n_online_attempts,
> +			n_offline_successes, n_offline_attempts,
> +			min_online, max_online,
> +			min_offline, max_offline,
> +			sum_online, sum_offline, HZ);
> +	cnt += scnprintf(&page[cnt], max - cnt,
> +			"barrier: %ld/%ld:%ld",
> +			n_barrier_successes,
> +			n_barrier_attempts,
> +			n_rcu_torture_barrier_error);
> +	cnt += scnprintf(&page[cnt], max - cnt, "\n%s%s ",
> +			torture_type, TORTURE_FLAG);
>  	if (atomic_read(&n_rcu_torture_mberror) != 0 ||
>  	    n_rcu_torture_barrier_error != 0 ||
>  	    n_rcu_torture_boost_ktrerror != 0 ||
>  	    n_rcu_torture_boost_rterror != 0 ||
>  	    n_rcu_torture_boost_failure != 0 ||
>  	    i > 1) {
> -		cnt += sprintf(&page[cnt], "!!! ");
> +		cnt += scnprintf(&page[cnt], max - cnt, "!!! ");
>  		atomic_inc(&n_rcu_torture_error);
>  		WARN_ON_ONCE(1);
>  	}
> -	cnt += sprintf(&page[cnt], "Reader Pipe: ");
> +	cnt += scnprintf(&page[cnt], max - cnt, "Reader Pipe: ");
>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
> -		cnt += sprintf(&page[cnt], " %ld", pipesummary[i]);
> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
> -	cnt += sprintf(&page[cnt], "Reader Batch: ");
> +		cnt += scnprintf(&page[cnt], max - cnt, " %ld", pipesummary[i]);
> +	cnt += scnprintf(&page[cnt], max - cnt, "\n%s%s ",
> +			torture_type, TORTURE_FLAG);
> +	cnt += scnprintf(&page[cnt], max - cnt, "Reader Batch: ");
>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
> -		cnt += sprintf(&page[cnt], " %ld", batchsummary[i]);
> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
> -	cnt += sprintf(&page[cnt], "Free-Block Circulation: ");
> +		cnt += scnprintf(&page[cnt], max - cnt, " %ld",
> +				batchsummary[i]);
> +	cnt += scnprintf(&page[cnt], max - cnt, "\n%s%s ",
> +			torture_type, TORTURE_FLAG);
> +	cnt += scnprintf(&page[cnt], max - cnt, "Free-Block Circulation: ");
>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++) {
> -		cnt += sprintf(&page[cnt], " %d",
> -			       atomic_read(&rcu_torture_wcount[i]));
> +		cnt += scnprintf(&page[cnt], max - cnt, " %d",
> +				atomic_read(&rcu_torture_wcount[i]));
>  	}
> -	cnt += sprintf(&page[cnt], "\n");
> +	cnt += scnprintf(&page[cnt], max - cnt, "\n");
>  	if (cur_ops->stats)
> -		cnt += cur_ops->stats(&page[cnt]);
> +		cnt += cur_ops->stats(&page[cnt], max - cnt);
>  	return cnt;
>  }
> 
> @@ -1136,7 +1144,7 @@ rcu_torture_stats_print(void)
>  {
>  	int cnt;
> 
> -	cnt = rcu_torture_printk(printk_buf);
> +	cnt = rcu_torture_printk(printk_buf, sizeof(printk_buf));
>  	pr_alert("%s", printk_buf);
>  }
> 
> -- 
> 1.7.7.6
> 

Re: [PATCH] kernel/rcutorture.c: use scnprintf() instead of sprintf()

[Chen Gang]

On 10/14/2013 07:28 PM, Paul E. McKenney wrote:
> On Mon, Oct 14, 2013 at 04:38:55PM +0800, Chen Gang wrote:
>> If the contents is more than 4096 bytes (e.g. if have 1K cpus), current
>> sprintf() will cause memory overflow.
>>
>> They are all test information which can be truncated, so use scnprintf()
>> instead of sprintf(), also add 'max' parameter for related functions,
>> also notice 80 columns boundary and parameters alignments.
>>
>> Test case:
>>
>>   Fedora16 x86_64, 2 CPUs, 2GB RAM, [in/rm]mod with "torture_type=srcu".
>>
>>     let maximize buffer to 256 to truncate in rcu_torture_printk().
>>     let maximize buffer to 410 to may truncate in srcu_torture_stats().
>>     let maximize buffer to 4096 (original size) to print full.
>>
>>   it is a rcu test module, so not need additional test or consideration.
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> 
> At first glance, not a bad start.
> 
> Suppose that your goal was to make it avoid truncation.  What would you
> do differently?
> 

One simple way: using snprintf() instead of scnprintf() in the related
printing functions. Then call them with "buffer == NULL" to get buffer
size, next allocate it and call it again ...

Hmm... it is only a test module, is it worth enough to try to make it
avoid truncation? If some members (quite few members) find truncation,
they can simply extend maximize buffer to avoid it when testing.

But if we do not fix this bug, when memory overflow, the OS may not stop
immediately, then it will/may lead the testers to face various amazing
things (which is not quite easy to find root cause).


All together, making contribution with efficiency and without negative
effect is our goal, so ...


Thanks.

> 							Thanx, Paul
> 
>> ---
>>  kernel/rcutorture.c |  110 +++++++++++++++++++++++++++-----------------------
>>  1 files changed, 59 insertions(+), 51 deletions(-)
>>
>> diff --git a/kernel/rcutorture.c b/kernel/rcutorture.c
>> index be63101..107fd76 100644
>> --- a/kernel/rcutorture.c
>> +++ b/kernel/rcutorture.c
>> @@ -370,7 +370,7 @@ struct rcu_torture_ops {
>>  	void (*call)(struct rcu_head *head, void (*func)(struct rcu_head *rcu));
>>  	void (*cb_barrier)(void);
>>  	void (*fqs)(void);
>> -	int (*stats)(char *page);
>> +	int (*stats)(char *page, int max);
>>  	int irq_capable;
>>  	int can_boost;
>>  	const char *name;
>> @@ -572,20 +572,20 @@ static void srcu_torture_barrier(void)
>>  	srcu_barrier(&srcu_ctl);
>>  }
>>
>> -static int srcu_torture_stats(char *page)
>> +static int srcu_torture_stats(char *page, int max)
>>  {
>>  	int cnt = 0;
>>  	int cpu;
>>  	int idx = srcu_ctl.completed & 0x1;
>>
>> -	cnt += sprintf(&page[cnt], "%s%s per-CPU(idx=%d):",
>> -		       torture_type, TORTURE_FLAG, idx);
>> +	cnt += scnprintf(&page[cnt], max - cnt, "%s%s per-CPU(idx=%d):",
>> +			torture_type, TORTURE_FLAG, idx);
>>  	for_each_possible_cpu(cpu) {
>> -		cnt += sprintf(&page[cnt], " %d(%lu,%lu)", cpu,
>> -			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[!idx],
>> -			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[idx]);
>> +		cnt += scnprintf(&page[cnt], max - cnt, " %d(%lu,%lu)", cpu,
>> +				per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[!idx],
>> +				per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[idx]);
>>  	}
>> -	cnt += sprintf(&page[cnt], "\n");
>> +	cnt += scnprintf(&page[cnt], max - cnt, "\n");
>>  	return cnt;
>>  }
>>
>> @@ -1047,7 +1047,7 @@ rcu_torture_reader(void *arg)
>>   * Create an RCU-torture statistics message in the specified buffer.
>>   */
>>  static int
>> -rcu_torture_printk(char *page)
>> +rcu_torture_printk(char *page, int max)
>>  {
>>  	int cnt = 0;
>>  	int cpu;
>> @@ -1065,61 +1065,69 @@ rcu_torture_printk(char *page)
>>  		if (pipesummary[i] != 0)
>>  			break;
>>  	}
>> -	cnt += sprintf(&page[cnt], "%s%s ", torture_type, TORTURE_FLAG);
>> -	cnt += sprintf(&page[cnt],
>> -		       "rtc: %p ver: %lu tfle: %d rta: %d rtaf: %d rtf: %d ",
>> -		       rcu_torture_current,
>> -		       rcu_torture_current_version,
>> -		       list_empty(&rcu_torture_freelist),
>> -		       atomic_read(&n_rcu_torture_alloc),
>> -		       atomic_read(&n_rcu_torture_alloc_fail),
>> -		       atomic_read(&n_rcu_torture_free));
>> -	cnt += sprintf(&page[cnt], "rtmbe: %d rtbke: %ld rtbre: %ld ",
>> -		       atomic_read(&n_rcu_torture_mberror),
>> -		       n_rcu_torture_boost_ktrerror,
>> -		       n_rcu_torture_boost_rterror);
>> -	cnt += sprintf(&page[cnt], "rtbf: %ld rtb: %ld nt: %ld ",
>> -		       n_rcu_torture_boost_failure,
>> -		       n_rcu_torture_boosts,
>> -		       n_rcu_torture_timers);
>> -	cnt += sprintf(&page[cnt],
>> -		       "onoff: %ld/%ld:%ld/%ld %d,%d:%d,%d %lu:%lu (HZ=%d) ",
>> -		       n_online_successes, n_online_attempts,
>> -		       n_offline_successes, n_offline_attempts,
>> -		       min_online, max_online,
>> -		       min_offline, max_offline,
>> -		       sum_online, sum_offline, HZ);
>> -	cnt += sprintf(&page[cnt], "barrier: %ld/%ld:%ld",
>> -		       n_barrier_successes,
>> -		       n_barrier_attempts,
>> -		       n_rcu_torture_barrier_error);
>> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
>> +	cnt += scnprintf(&page[cnt], max - cnt, "%s%s ",
>> +			torture_type, TORTURE_FLAG);
>> +	cnt += scnprintf(&page[cnt], max - cnt,
>> +			"rtc: %p ver: %lu tfle: %d rta: %d rtaf: %d rtf: %d ",
>> +			rcu_torture_current,
>> +			rcu_torture_current_version,
>> +			list_empty(&rcu_torture_freelist),
>> +			atomic_read(&n_rcu_torture_alloc),
>> +			atomic_read(&n_rcu_torture_alloc_fail),
>> +			atomic_read(&n_rcu_torture_free));
>> +	cnt += scnprintf(&page[cnt], max - cnt,
>> +			"rtmbe: %d rtbke: %ld rtbre: %ld ",
>> +			atomic_read(&n_rcu_torture_mberror),
>> +			n_rcu_torture_boost_ktrerror,
>> +			n_rcu_torture_boost_rterror);
>> +	cnt += scnprintf(&page[cnt], max - cnt,
>> +			"rtbf: %ld rtb: %ld nt: %ld ",
>> +			n_rcu_torture_boost_failure,
>> +			n_rcu_torture_boosts,
>> +			n_rcu_torture_timers);
>> +	cnt += scnprintf(&page[cnt], max - cnt,
>> +			"onoff: %ld/%ld:%ld/%ld %d,%d:%d,%d %lu:%lu (HZ=%d) ",
>> +			n_online_successes, n_online_attempts,
>> +			n_offline_successes, n_offline_attempts,
>> +			min_online, max_online,
>> +			min_offline, max_offline,
>> +			sum_online, sum_offline, HZ);
>> +	cnt += scnprintf(&page[cnt], max - cnt,
>> +			"barrier: %ld/%ld:%ld",
>> +			n_barrier_successes,
>> +			n_barrier_attempts,
>> +			n_rcu_torture_barrier_error);
>> +	cnt += scnprintf(&page[cnt], max - cnt, "\n%s%s ",
>> +			torture_type, TORTURE_FLAG);
>>  	if (atomic_read(&n_rcu_torture_mberror) != 0 ||
>>  	    n_rcu_torture_barrier_error != 0 ||
>>  	    n_rcu_torture_boost_ktrerror != 0 ||
>>  	    n_rcu_torture_boost_rterror != 0 ||
>>  	    n_rcu_torture_boost_failure != 0 ||
>>  	    i > 1) {
>> -		cnt += sprintf(&page[cnt], "!!! ");
>> +		cnt += scnprintf(&page[cnt], max - cnt, "!!! ");
>>  		atomic_inc(&n_rcu_torture_error);
>>  		WARN_ON_ONCE(1);
>>  	}
>> -	cnt += sprintf(&page[cnt], "Reader Pipe: ");
>> +	cnt += scnprintf(&page[cnt], max - cnt, "Reader Pipe: ");
>>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
>> -		cnt += sprintf(&page[cnt], " %ld", pipesummary[i]);
>> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
>> -	cnt += sprintf(&page[cnt], "Reader Batch: ");
>> +		cnt += scnprintf(&page[cnt], max - cnt, " %ld", pipesummary[i]);
>> +	cnt += scnprintf(&page[cnt], max - cnt, "\n%s%s ",
>> +			torture_type, TORTURE_FLAG);
>> +	cnt += scnprintf(&page[cnt], max - cnt, "Reader Batch: ");
>>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
>> -		cnt += sprintf(&page[cnt], " %ld", batchsummary[i]);
>> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
>> -	cnt += sprintf(&page[cnt], "Free-Block Circulation: ");
>> +		cnt += scnprintf(&page[cnt], max - cnt, " %ld",
>> +				batchsummary[i]);
>> +	cnt += scnprintf(&page[cnt], max - cnt, "\n%s%s ",
>> +			torture_type, TORTURE_FLAG);
>> +	cnt += scnprintf(&page[cnt], max - cnt, "Free-Block Circulation: ");
>>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++) {
>> -		cnt += sprintf(&page[cnt], " %d",
>> -			       atomic_read(&rcu_torture_wcount[i]));
>> +		cnt += scnprintf(&page[cnt], max - cnt, " %d",
>> +				atomic_read(&rcu_torture_wcount[i]));
>>  	}
>> -	cnt += sprintf(&page[cnt], "\n");
>> +	cnt += scnprintf(&page[cnt], max - cnt, "\n");
>>  	if (cur_ops->stats)
>> -		cnt += cur_ops->stats(&page[cnt]);
>> +		cnt += cur_ops->stats(&page[cnt], max - cnt);
>>  	return cnt;
>>  }
>>
>> @@ -1136,7 +1144,7 @@ rcu_torture_stats_print(void)
>>  {
>>  	int cnt;
>>
>> -	cnt = rcu_torture_printk(printk_buf);
>> +	cnt = rcu_torture_printk(printk_buf, sizeof(printk_buf));
>>  	pr_alert("%s", printk_buf);
>>  }
>>
>> -- 
>> 1.7.7.6
>>
> 
> 
> 


-- 
Chen Gang

Re: [PATCH] kernel/rcutorture.c: use scnprintf() instead of sprintf()

[Chen Gang]

On 10/15/2013 08:54 AM, Chen Gang wrote:
> On 10/14/2013 07:28 PM, Paul E. McKenney wrote:
>> On Mon, Oct 14, 2013 at 04:38:55PM +0800, Chen Gang wrote:
>>> If the contents is more than 4096 bytes (e.g. if have 1K cpus), current
>>> sprintf() will cause memory overflow.
>>>
>>> They are all test information which can be truncated, so use scnprintf()
>>> instead of sprintf(), also add 'max' parameter for related functions,
>>> also notice 80 columns boundary and parameters alignments.
>>>
>>> Test case:
>>>
>>>   Fedora16 x86_64, 2 CPUs, 2GB RAM, [in/rm]mod with "torture_type=srcu".
>>>
>>>     let maximize buffer to 256 to truncate in rcu_torture_printk().
>>>     let maximize buffer to 410 to may truncate in srcu_torture_stats().
>>>     let maximize buffer to 4096 (original size) to print full.
>>>
>>>   it is a rcu test module, so not need additional test or consideration.
>>>
>>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>>
>> At first glance, not a bad start.
>>
>> Suppose that your goal was to make it avoid truncation.  What would you
>> do differently?
>>
> 
> One simple way: using snprintf() instead of scnprintf() in the related
> printing functions. Then call them with "buffer == NULL" to get buffer
> size, next allocate it and call it again ...
> 

Oh, this simple way assumes the printing contents will not be changed
during the 2 calls.

> Hmm... it is only a test module, is it worth enough to try to make it
> avoid truncation? If some members (quite few members) find truncation,
> they can simply extend maximize buffer to avoid it when testing.
> 
> But if we do not fix this bug, when memory overflow, the OS may not stop
> immediately, then it will/may lead the testers to face various amazing
> things (which is not quite easy to find root cause).
> 
> 
> All together, making contribution with efficiency and without negative
> effect is our goal, so ...
> 
> 
> Thanks.
> 
>> 							Thanx, Paul
>>
>>> ---
>>>  kernel/rcutorture.c |  110 +++++++++++++++++++++++++++-----------------------
>>>  1 files changed, 59 insertions(+), 51 deletions(-)
>>>
>>> diff --git a/kernel/rcutorture.c b/kernel/rcutorture.c
>>> index be63101..107fd76 100644
>>> --- a/kernel/rcutorture.c
>>> +++ b/kernel/rcutorture.c
>>> @@ -370,7 +370,7 @@ struct rcu_torture_ops {
>>>  	void (*call)(struct rcu_head *head, void (*func)(struct rcu_head *rcu));
>>>  	void (*cb_barrier)(void);
>>>  	void (*fqs)(void);
>>> -	int (*stats)(char *page);
>>> +	int (*stats)(char *page, int max);
>>>  	int irq_capable;
>>>  	int can_boost;
>>>  	const char *name;
>>> @@ -572,20 +572,20 @@ static void srcu_torture_barrier(void)
>>>  	srcu_barrier(&srcu_ctl);
>>>  }
>>>
>>> -static int srcu_torture_stats(char *page)
>>> +static int srcu_torture_stats(char *page, int max)
>>>  {
>>>  	int cnt = 0;
>>>  	int cpu;
>>>  	int idx = srcu_ctl.completed & 0x1;
>>>
>>> -	cnt += sprintf(&page[cnt], "%s%s per-CPU(idx=%d):",
>>> -		       torture_type, TORTURE_FLAG, idx);
>>> +	cnt += scnprintf(&page[cnt], max - cnt, "%s%s per-CPU(idx=%d):",
>>> +			torture_type, TORTURE_FLAG, idx);
>>>  	for_each_possible_cpu(cpu) {
>>> -		cnt += sprintf(&page[cnt], " %d(%lu,%lu)", cpu,
>>> -			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[!idx],
>>> -			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[idx]);
>>> +		cnt += scnprintf(&page[cnt], max - cnt, " %d(%lu,%lu)", cpu,
>>> +				per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[!idx],
>>> +				per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[idx]);
>>>  	}
>>> -	cnt += sprintf(&page[cnt], "\n");
>>> +	cnt += scnprintf(&page[cnt], max - cnt, "\n");
>>>  	return cnt;
>>>  }
>>>
>>> @@ -1047,7 +1047,7 @@ rcu_torture_reader(void *arg)
>>>   * Create an RCU-torture statistics message in the specified buffer.
>>>   */
>>>  static int
>>> -rcu_torture_printk(char *page)
>>> +rcu_torture_printk(char *page, int max)
>>>  {
>>>  	int cnt = 0;
>>>  	int cpu;
>>> @@ -1065,61 +1065,69 @@ rcu_torture_printk(char *page)
>>>  		if (pipesummary[i] != 0)
>>>  			break;
>>>  	}
>>> -	cnt += sprintf(&page[cnt], "%s%s ", torture_type, TORTURE_FLAG);
>>> -	cnt += sprintf(&page[cnt],
>>> -		       "rtc: %p ver: %lu tfle: %d rta: %d rtaf: %d rtf: %d ",
>>> -		       rcu_torture_current,
>>> -		       rcu_torture_current_version,
>>> -		       list_empty(&rcu_torture_freelist),
>>> -		       atomic_read(&n_rcu_torture_alloc),
>>> -		       atomic_read(&n_rcu_torture_alloc_fail),
>>> -		       atomic_read(&n_rcu_torture_free));
>>> -	cnt += sprintf(&page[cnt], "rtmbe: %d rtbke: %ld rtbre: %ld ",
>>> -		       atomic_read(&n_rcu_torture_mberror),
>>> -		       n_rcu_torture_boost_ktrerror,
>>> -		       n_rcu_torture_boost_rterror);
>>> -	cnt += sprintf(&page[cnt], "rtbf: %ld rtb: %ld nt: %ld ",
>>> -		       n_rcu_torture_boost_failure,
>>> -		       n_rcu_torture_boosts,
>>> -		       n_rcu_torture_timers);
>>> -	cnt += sprintf(&page[cnt],
>>> -		       "onoff: %ld/%ld:%ld/%ld %d,%d:%d,%d %lu:%lu (HZ=%d) ",
>>> -		       n_online_successes, n_online_attempts,
>>> -		       n_offline_successes, n_offline_attempts,
>>> -		       min_online, max_online,
>>> -		       min_offline, max_offline,
>>> -		       sum_online, sum_offline, HZ);
>>> -	cnt += sprintf(&page[cnt], "barrier: %ld/%ld:%ld",
>>> -		       n_barrier_successes,
>>> -		       n_barrier_attempts,
>>> -		       n_rcu_torture_barrier_error);
>>> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
>>> +	cnt += scnprintf(&page[cnt], max - cnt, "%s%s ",
>>> +			torture_type, TORTURE_FLAG);
>>> +	cnt += scnprintf(&page[cnt], max - cnt,
>>> +			"rtc: %p ver: %lu tfle: %d rta: %d rtaf: %d rtf: %d ",
>>> +			rcu_torture_current,
>>> +			rcu_torture_current_version,
>>> +			list_empty(&rcu_torture_freelist),
>>> +			atomic_read(&n_rcu_torture_alloc),
>>> +			atomic_read(&n_rcu_torture_alloc_fail),
>>> +			atomic_read(&n_rcu_torture_free));
>>> +	cnt += scnprintf(&page[cnt], max - cnt,
>>> +			"rtmbe: %d rtbke: %ld rtbre: %ld ",
>>> +			atomic_read(&n_rcu_torture_mberror),
>>> +			n_rcu_torture_boost_ktrerror,
>>> +			n_rcu_torture_boost_rterror);
>>> +	cnt += scnprintf(&page[cnt], max - cnt,
>>> +			"rtbf: %ld rtb: %ld nt: %ld ",
>>> +			n_rcu_torture_boost_failure,
>>> +			n_rcu_torture_boosts,
>>> +			n_rcu_torture_timers);
>>> +	cnt += scnprintf(&page[cnt], max - cnt,
>>> +			"onoff: %ld/%ld:%ld/%ld %d,%d:%d,%d %lu:%lu (HZ=%d) ",
>>> +			n_online_successes, n_online_attempts,
>>> +			n_offline_successes, n_offline_attempts,
>>> +			min_online, max_online,
>>> +			min_offline, max_offline,
>>> +			sum_online, sum_offline, HZ);
>>> +	cnt += scnprintf(&page[cnt], max - cnt,
>>> +			"barrier: %ld/%ld:%ld",
>>> +			n_barrier_successes,
>>> +			n_barrier_attempts,
>>> +			n_rcu_torture_barrier_error);
>>> +	cnt += scnprintf(&page[cnt], max - cnt, "\n%s%s ",
>>> +			torture_type, TORTURE_FLAG);
>>>  	if (atomic_read(&n_rcu_torture_mberror) != 0 ||
>>>  	    n_rcu_torture_barrier_error != 0 ||
>>>  	    n_rcu_torture_boost_ktrerror != 0 ||
>>>  	    n_rcu_torture_boost_rterror != 0 ||
>>>  	    n_rcu_torture_boost_failure != 0 ||
>>>  	    i > 1) {
>>> -		cnt += sprintf(&page[cnt], "!!! ");
>>> +		cnt += scnprintf(&page[cnt], max - cnt, "!!! ");
>>>  		atomic_inc(&n_rcu_torture_error);
>>>  		WARN_ON_ONCE(1);
>>>  	}
>>> -	cnt += sprintf(&page[cnt], "Reader Pipe: ");
>>> +	cnt += scnprintf(&page[cnt], max - cnt, "Reader Pipe: ");
>>>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
>>> -		cnt += sprintf(&page[cnt], " %ld", pipesummary[i]);
>>> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
>>> -	cnt += sprintf(&page[cnt], "Reader Batch: ");
>>> +		cnt += scnprintf(&page[cnt], max - cnt, " %ld", pipesummary[i]);
>>> +	cnt += scnprintf(&page[cnt], max - cnt, "\n%s%s ",
>>> +			torture_type, TORTURE_FLAG);
>>> +	cnt += scnprintf(&page[cnt], max - cnt, "Reader Batch: ");
>>>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
>>> -		cnt += sprintf(&page[cnt], " %ld", batchsummary[i]);
>>> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
>>> -	cnt += sprintf(&page[cnt], "Free-Block Circulation: ");
>>> +		cnt += scnprintf(&page[cnt], max - cnt, " %ld",
>>> +				batchsummary[i]);
>>> +	cnt += scnprintf(&page[cnt], max - cnt, "\n%s%s ",
>>> +			torture_type, TORTURE_FLAG);
>>> +	cnt += scnprintf(&page[cnt], max - cnt, "Free-Block Circulation: ");
>>>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++) {
>>> -		cnt += sprintf(&page[cnt], " %d",
>>> -			       atomic_read(&rcu_torture_wcount[i]));
>>> +		cnt += scnprintf(&page[cnt], max - cnt, " %d",
>>> +				atomic_read(&rcu_torture_wcount[i]));
>>>  	}
>>> -	cnt += sprintf(&page[cnt], "\n");
>>> +	cnt += scnprintf(&page[cnt], max - cnt, "\n");
>>>  	if (cur_ops->stats)
>>> -		cnt += cur_ops->stats(&page[cnt]);
>>> +		cnt += cur_ops->stats(&page[cnt], max - cnt);
>>>  	return cnt;
>>>  }
>>>
>>> @@ -1136,7 +1144,7 @@ rcu_torture_stats_print(void)
>>>  {
>>>  	int cnt;
>>>
>>> -	cnt = rcu_torture_printk(printk_buf);
>>> +	cnt = rcu_torture_printk(printk_buf, sizeof(printk_buf));
>>>  	pr_alert("%s", printk_buf);
>>>  }
>>>
>>> -- 
>>> 1.7.7.6
>>>
>>
>>
>>
> 
> 


-- 
Chen Gang

Re: [PATCH] kernel/rcutorture.c: use scnprintf() instead of sprintf()

[Paul E. McKenney]

On Tue, Oct 15, 2013 at 09:51:42AM +0800, Chen Gang wrote:
> On 10/15/2013 08:54 AM, Chen Gang wrote:
> > On 10/14/2013 07:28 PM, Paul E. McKenney wrote:
> >> On Mon, Oct 14, 2013 at 04:38:55PM +0800, Chen Gang wrote:
> >>> If the contents is more than 4096 bytes (e.g. if have 1K cpus), current
> >>> sprintf() will cause memory overflow.
> >>>
> >>> They are all test information which can be truncated, so use scnprintf()
> >>> instead of sprintf(), also add 'max' parameter for related functions,
> >>> also notice 80 columns boundary and parameters alignments.
> >>>
> >>> Test case:
> >>>
> >>>   Fedora16 x86_64, 2 CPUs, 2GB RAM, [in/rm]mod with "torture_type=srcu".
> >>>
> >>>     let maximize buffer to 256 to truncate in rcu_torture_printk().
> >>>     let maximize buffer to 410 to may truncate in srcu_torture_stats().
> >>>     let maximize buffer to 4096 (original size) to print full.
> >>>
> >>>   it is a rcu test module, so not need additional test or consideration.
> >>>
> >>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> >>
> >> At first glance, not a bad start.
> >>
> >> Suppose that your goal was to make it avoid truncation.  What would you
> >> do differently?
> >>
> > 
> > One simple way: using snprintf() instead of scnprintf() in the related
> > printing functions. Then call them with "buffer == NULL" to get buffer
> > size, next allocate it and call it again ...
> 
> Oh, this simple way assumes the printing contents will not be changed
> during the 2 calls.

Indeed.  But can you make use of nr_cpu_ids, which is set at boot time
to the the maximum number of CPUs that the particular booting system
will ever be able to contain?  Keep in mind that you know the maximum
number of digits that an unsigned long will print in 32-bit and 64-bit
systems.

> > Hmm... it is only a test module, is it worth enough to try to make it
> > avoid truncation? If some members (quite few members) find truncation,
> > they can simply extend maximize buffer to avoid it when testing.
> > 
> > But if we do not fix this bug, when memory overflow, the OS may not stop
> > immediately, then it will/may lead the testers to face various amazing
> > things (which is not quite easy to find root cause).

It might cause strange symptoms, but it is not bad practice to try
it anyway, especially when the code is unfamiliar.  After all, if the
strange systems appear on memory overflow, but do not appear if there
is no memory overflow, you have a pretty good idea what the cause .
Besides, there might be some other mechanism to prevent the problem.
Of course, there is no such mechanism in this particular case, but in
general it is more efficient to find that out quickly then to spend time
designing a solution that is not needed.

							Thanx, Paul

> > All together, making contribution with efficiency and without negative
> > effect is our goal, so ...
> > 
> > 
> > Thanks.
> > 
> >> 							Thanx, Paul
> >>
> >>> ---
> >>>  kernel/rcutorture.c |  110 +++++++++++++++++++++++++++-----------------------
> >>>  1 files changed, 59 insertions(+), 51 deletions(-)
> >>>
> >>> diff --git a/kernel/rcutorture.c b/kernel/rcutorture.c
> >>> index be63101..107fd76 100644
> >>> --- a/kernel/rcutorture.c
> >>> +++ b/kernel/rcutorture.c
> >>> @@ -370,7 +370,7 @@ struct rcu_torture_ops {
> >>>  	void (*call)(struct rcu_head *head, void (*func)(struct rcu_head *rcu));
> >>>  	void (*cb_barrier)(void);
> >>>  	void (*fqs)(void);
> >>> -	int (*stats)(char *page);
> >>> +	int (*stats)(char *page, int max);
> >>>  	int irq_capable;
> >>>  	int can_boost;
> >>>  	const char *name;
> >>> @@ -572,20 +572,20 @@ static void srcu_torture_barrier(void)
> >>>  	srcu_barrier(&srcu_ctl);
> >>>  }
> >>>
> >>> -static int srcu_torture_stats(char *page)
> >>> +static int srcu_torture_stats(char *page, int max)
> >>>  {
> >>>  	int cnt = 0;
> >>>  	int cpu;
> >>>  	int idx = srcu_ctl.completed & 0x1;
> >>>
> >>> -	cnt += sprintf(&page[cnt], "%s%s per-CPU(idx=%d):",
> >>> -		       torture_type, TORTURE_FLAG, idx);
> >>> +	cnt += scnprintf(&page[cnt], max - cnt, "%s%s per-CPU(idx=%d):",
> >>> +			torture_type, TORTURE_FLAG, idx);
> >>>  	for_each_possible_cpu(cpu) {
> >>> -		cnt += sprintf(&page[cnt], " %d(%lu,%lu)", cpu,
> >>> -			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[!idx],
> >>> -			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[idx]);
> >>> +		cnt += scnprintf(&page[cnt], max - cnt, " %d(%lu,%lu)", cpu,
> >>> +				per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[!idx],
> >>> +				per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[idx]);
> >>>  	}
> >>> -	cnt += sprintf(&page[cnt], "\n");
> >>> +	cnt += scnprintf(&page[cnt], max - cnt, "\n");
> >>>  	return cnt;
> >>>  }
> >>>
> >>> @@ -1047,7 +1047,7 @@ rcu_torture_reader(void *arg)
> >>>   * Create an RCU-torture statistics message in the specified buffer.
> >>>   */
> >>>  static int
> >>> -rcu_torture_printk(char *page)
> >>> +rcu_torture_printk(char *page, int max)
> >>>  {
> >>>  	int cnt = 0;
> >>>  	int cpu;
> >>> @@ -1065,61 +1065,69 @@ rcu_torture_printk(char *page)
> >>>  		if (pipesummary[i] != 0)
> >>>  			break;
> >>>  	}
> >>> -	cnt += sprintf(&page[cnt], "%s%s ", torture_type, TORTURE_FLAG);
> >>> -	cnt += sprintf(&page[cnt],
> >>> -		       "rtc: %p ver: %lu tfle: %d rta: %d rtaf: %d rtf: %d ",
> >>> -		       rcu_torture_current,
> >>> -		       rcu_torture_current_version,
> >>> -		       list_empty(&rcu_torture_freelist),
> >>> -		       atomic_read(&n_rcu_torture_alloc),
> >>> -		       atomic_read(&n_rcu_torture_alloc_fail),
> >>> -		       atomic_read(&n_rcu_torture_free));
> >>> -	cnt += sprintf(&page[cnt], "rtmbe: %d rtbke: %ld rtbre: %ld ",
> >>> -		       atomic_read(&n_rcu_torture_mberror),
> >>> -		       n_rcu_torture_boost_ktrerror,
> >>> -		       n_rcu_torture_boost_rterror);
> >>> -	cnt += sprintf(&page[cnt], "rtbf: %ld rtb: %ld nt: %ld ",
> >>> -		       n_rcu_torture_boost_failure,
> >>> -		       n_rcu_torture_boosts,
> >>> -		       n_rcu_torture_timers);
> >>> -	cnt += sprintf(&page[cnt],
> >>> -		       "onoff: %ld/%ld:%ld/%ld %d,%d:%d,%d %lu:%lu (HZ=%d) ",
> >>> -		       n_online_successes, n_online_attempts,
> >>> -		       n_offline_successes, n_offline_attempts,
> >>> -		       min_online, max_online,
> >>> -		       min_offline, max_offline,
> >>> -		       sum_online, sum_offline, HZ);
> >>> -	cnt += sprintf(&page[cnt], "barrier: %ld/%ld:%ld",
> >>> -		       n_barrier_successes,
> >>> -		       n_barrier_attempts,
> >>> -		       n_rcu_torture_barrier_error);
> >>> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
> >>> +	cnt += scnprintf(&page[cnt], max - cnt, "%s%s ",
> >>> +			torture_type, TORTURE_FLAG);
> >>> +	cnt += scnprintf(&page[cnt], max - cnt,
> >>> +			"rtc: %p ver: %lu tfle: %d rta: %d rtaf: %d rtf: %d ",
> >>> +			rcu_torture_current,
> >>> +			rcu_torture_current_version,
> >>> +			list_empty(&rcu_torture_freelist),
> >>> +			atomic_read(&n_rcu_torture_alloc),
> >>> +			atomic_read(&n_rcu_torture_alloc_fail),
> >>> +			atomic_read(&n_rcu_torture_free));
> >>> +	cnt += scnprintf(&page[cnt], max - cnt,
> >>> +			"rtmbe: %d rtbke: %ld rtbre: %ld ",
> >>> +			atomic_read(&n_rcu_torture_mberror),
> >>> +			n_rcu_torture_boost_ktrerror,
> >>> +			n_rcu_torture_boost_rterror);
> >>> +	cnt += scnprintf(&page[cnt], max - cnt,
> >>> +			"rtbf: %ld rtb: %ld nt: %ld ",
> >>> +			n_rcu_torture_boost_failure,
> >>> +			n_rcu_torture_boosts,
> >>> +			n_rcu_torture_timers);
> >>> +	cnt += scnprintf(&page[cnt], max - cnt,
> >>> +			"onoff: %ld/%ld:%ld/%ld %d,%d:%d,%d %lu:%lu (HZ=%d) ",
> >>> +			n_online_successes, n_online_attempts,
> >>> +			n_offline_successes, n_offline_attempts,
> >>> +			min_online, max_online,
> >>> +			min_offline, max_offline,
> >>> +			sum_online, sum_offline, HZ);
> >>> +	cnt += scnprintf(&page[cnt], max - cnt,
> >>> +			"barrier: %ld/%ld:%ld",
> >>> +			n_barrier_successes,
> >>> +			n_barrier_attempts,
> >>> +			n_rcu_torture_barrier_error);
> >>> +	cnt += scnprintf(&page[cnt], max - cnt, "\n%s%s ",
> >>> +			torture_type, TORTURE_FLAG);
> >>>  	if (atomic_read(&n_rcu_torture_mberror) != 0 ||
> >>>  	    n_rcu_torture_barrier_error != 0 ||
> >>>  	    n_rcu_torture_boost_ktrerror != 0 ||
> >>>  	    n_rcu_torture_boost_rterror != 0 ||
> >>>  	    n_rcu_torture_boost_failure != 0 ||
> >>>  	    i > 1) {
> >>> -		cnt += sprintf(&page[cnt], "!!! ");
> >>> +		cnt += scnprintf(&page[cnt], max - cnt, "!!! ");
> >>>  		atomic_inc(&n_rcu_torture_error);
> >>>  		WARN_ON_ONCE(1);
> >>>  	}
> >>> -	cnt += sprintf(&page[cnt], "Reader Pipe: ");
> >>> +	cnt += scnprintf(&page[cnt], max - cnt, "Reader Pipe: ");
> >>>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
> >>> -		cnt += sprintf(&page[cnt], " %ld", pipesummary[i]);
> >>> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
> >>> -	cnt += sprintf(&page[cnt], "Reader Batch: ");
> >>> +		cnt += scnprintf(&page[cnt], max - cnt, " %ld", pipesummary[i]);
> >>> +	cnt += scnprintf(&page[cnt], max - cnt, "\n%s%s ",
> >>> +			torture_type, TORTURE_FLAG);
> >>> +	cnt += scnprintf(&page[cnt], max - cnt, "Reader Batch: ");
> >>>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
> >>> -		cnt += sprintf(&page[cnt], " %ld", batchsummary[i]);
> >>> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
> >>> -	cnt += sprintf(&page[cnt], "Free-Block Circulation: ");
> >>> +		cnt += scnprintf(&page[cnt], max - cnt, " %ld",
> >>> +				batchsummary[i]);
> >>> +	cnt += scnprintf(&page[cnt], max - cnt, "\n%s%s ",
> >>> +			torture_type, TORTURE_FLAG);
> >>> +	cnt += scnprintf(&page[cnt], max - cnt, "Free-Block Circulation: ");
> >>>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++) {
> >>> -		cnt += sprintf(&page[cnt], " %d",
> >>> -			       atomic_read(&rcu_torture_wcount[i]));
> >>> +		cnt += scnprintf(&page[cnt], max - cnt, " %d",
> >>> +				atomic_read(&rcu_torture_wcount[i]));
> >>>  	}
> >>> -	cnt += sprintf(&page[cnt], "\n");
> >>> +	cnt += scnprintf(&page[cnt], max - cnt, "\n");
> >>>  	if (cur_ops->stats)
> >>> -		cnt += cur_ops->stats(&page[cnt]);
> >>> +		cnt += cur_ops->stats(&page[cnt], max - cnt);
> >>>  	return cnt;
> >>>  }
> >>>
> >>> @@ -1136,7 +1144,7 @@ rcu_torture_stats_print(void)
> >>>  {
> >>>  	int cnt;
> >>>
> >>> -	cnt = rcu_torture_printk(printk_buf);
> >>> +	cnt = rcu_torture_printk(printk_buf, sizeof(printk_buf));
> >>>  	pr_alert("%s", printk_buf);
> >>>  }
> >>>
> >>> -- 
> >>> 1.7.7.6
> >>>
> >>
> >>
> >>
> > 
> > 
> 
> 
> -- 
> Chen Gang
> 

Re: [PATCH] kernel/rcutorture.c: use scnprintf() instead of sprintf()

[Chen Gang]

On 10/15/2013 04:26 PM, Paul E. McKenney wrote:
> On Tue, Oct 15, 2013 at 09:51:42AM +0800, Chen Gang wrote:
>>> One simple way: using snprintf() instead of scnprintf() in the related
>>> printing functions. Then call them with "buffer == NULL" to get buffer
>>> size, next allocate it and call it again ...
>>
>> Oh, this simple way assumes the printing contents will not be changed
>> during the 2 calls.
> 
> Indeed.  But can you make use of nr_cpu_ids, which is set at boot time
> to the the maximum number of CPUs that the particular booting system
> will ever be able to contain?  Keep in mind that you know the maximum
> number of digits that an unsigned long will print in 32-bit and 64-bit
> systems.
> 

Yeah, that is a way for it. It seems you (related maintainer) like
additional fix for it.

Hmm... I will try within this week (although I don't think it is quite
necessary to me).

:-)

>>> Hmm... it is only a test module, is it worth enough to try to make it
>>> avoid truncation? If some members (quite few members) find truncation,
>>> they can simply extend maximize buffer to avoid it when testing.
>>>
>>> But if we do not fix this bug, when memory overflow, the OS may not stop
>>> immediately, then it will/may lead the testers to face various amazing
>>> things (which is not quite easy to find root cause).
> 
> It might cause strange symptoms, but it is not bad practice to try
> it anyway, especially when the code is unfamiliar.  After all, if the
> strange systems appear on memory overflow, but do not appear if there
> is no memory overflow, you have a pretty good idea what the cause .
> Besides, there might be some other mechanism to prevent the problem.
> Of course, there is no such mechanism in this particular case, but in
> general it is more efficient to find that out quickly then to spend time
> designing a solution that is not needed.

Excuse me, my English is not quite well, I am not quite understand your
meaning.

I guess your meaning is: "after find a simple/acceptable solution, we
can think of more, it may be more efficient".

If what I guess is correct, It is OK to me -- since at least, it is not
an 'urgent' thing (for 'important' thing, your idea is more efficient,
although for 'urgent' thing, it is not).


Thanks.
-- 
Chen Gang

Re: [PATCH] kernel/rcutorture.c: use scnprintf() instead of sprintf()

[Paul E. McKenney]

On Tue, Oct 15, 2013 at 08:32:41PM +0800, Chen Gang wrote:
> On 10/15/2013 04:26 PM, Paul E. McKenney wrote:
> > On Tue, Oct 15, 2013 at 09:51:42AM +0800, Chen Gang wrote:
> >>> One simple way: using snprintf() instead of scnprintf() in the related
> >>> printing functions. Then call them with "buffer == NULL" to get buffer
> >>> size, next allocate it and call it again ...
> >>
> >> Oh, this simple way assumes the printing contents will not be changed
> >> during the 2 calls.
> > 
> > Indeed.  But can you make use of nr_cpu_ids, which is set at boot time
> > to the the maximum number of CPUs that the particular booting system
> > will ever be able to contain?  Keep in mind that you know the maximum
> > number of digits that an unsigned long will print in 32-bit and 64-bit
> > systems.
> 
> Yeah, that is a way for it. It seems you (related maintainer) like
> additional fix for it.
> 
> Hmm... I will try within this week (although I don't think it is quite
> necessary to me).
> 
> :-)

If you always ensure that the buffer is big enough, do you really need
the checking?

> >>> Hmm... it is only a test module, is it worth enough to try to make it
> >>> avoid truncation? If some members (quite few members) find truncation,
> >>> they can simply extend maximize buffer to avoid it when testing.
> >>>
> >>> But if we do not fix this bug, when memory overflow, the OS may not stop
> >>> immediately, then it will/may lead the testers to face various amazing
> >>> things (which is not quite easy to find root cause).
> > 
> > It might cause strange symptoms, but it is not bad practice to try
> > it anyway, especially when the code is unfamiliar.  After all, if the
> > strange systems appear on memory overflow, but do not appear if there
> > is no memory overflow, you have a pretty good idea what the cause .
> > Besides, there might be some other mechanism to prevent the problem.
> > Of course, there is no such mechanism in this particular case, but in
> > general it is more efficient to find that out quickly then to spend time
> > designing a solution that is not needed.
> 
> Excuse me, my English is not quite well, I am not quite understand your
> meaning.
> 
> I guess your meaning is: "after find a simple/acceptable solution, we
> can think of more, it may be more efficient".
> 
> If what I guess is correct, It is OK to me -- since at least, it is not
> an 'urgent' thing (for 'important' thing, your idea is more efficient,
> although for 'urgent' thing, it is not).

That is important as well -- the first solution you think of might not
be the right one.

My point is related.  If you believe you found a bug by inspection,
it is often worth testing to be sure.  Especially if the code in
question is at all complex.

							Thanx, Paul

Re: [PATCH] kernel/rcutorture.c: use scnprintf() instead of sprintf()

[Chen Gang]

On 10/15/2013 10:47 PM, Paul E. McKenney wrote:
> On Tue, Oct 15, 2013 at 08:32:41PM +0800, Chen Gang wrote:
>> Yeah, that is a way for it. It seems you (related maintainer) like
>> additional fix for it.
>>
>> Hmm... I will try within this week (although I don't think it is quite
>> necessary to me).
>>
>> :-)
> 
> If you always ensure that the buffer is big enough, do you really need
> the checking?
> 

Since they are all normal static functions: Of cause not need length
checking, either don't need return value, either don't need local
variable 'cnt'.

>>
>> Excuse me, my English is not quite well, I am not quite understand your
>> meaning.
>>
>> I guess your meaning is: "after find a simple/acceptable solution, we
>> can think of more, it may be more efficient".
>>
>> If what I guess is correct, It is OK to me -- since at least, it is not
>> an 'urgent' thing (for 'important' thing, your idea is more efficient,
>> although for 'urgent' thing, it is not).
> 
> That is important as well -- the first solution you think of might not
> be the right one.
> 

In my opinion, my first solution is correct, simple, and acceptable
enough for a test module, although it may be not the simplest, or not
most acceptable one.

> My point is related.  If you believe you found a bug by inspection,
> it is often worth testing to be sure.  Especially if the code in
> question is at all complex.
> 

Yeah, "it is often worth testing to be sure": it is related with test
case which based on the demands (so demands/requirement is the first),
in fact, most of maintainers will not give much focus on a test module.

The reason why I still will spend more time resource on test module is:
"since the related maintainer wants to focus on it, if it isn't urgent,
I will spend more time resource on it".

For 'important' but not 'urgent' thing (I assume your demands belong to
'important' thing), often need a trigger, if no triggers, better not
touch it now (it is not quite efficient). Now you are the 'trigger'. ;-)


> 							Thanx, Paul
> 
> 
> 


Thanks.
-- 
Chen Gang

Re: [PATCH] kernel/rcutorture.c: use scnprintf() instead of sprintf()

[Chen Gang]

On 10/16/2013 10:07 AM, Chen Gang wrote:
> On 10/15/2013 10:47 PM, Paul E. McKenney wrote:
>> > On Tue, Oct 15, 2013 at 08:32:41PM +0800, Chen Gang wrote:
>>> >> Yeah, that is a way for it. It seems you (related maintainer) like
>>> >> additional fix for it.
>>> >>
>>> >> Hmm... I will try within this week (although I don't think it is quite
>>> >> necessary to me).
>>> >>
>>> >> :-)
>> > 
>> > If you always ensure that the buffer is big enough, do you really need
>> > the checking?
>> > 
> Since they are all normal static functions: Of cause not need length
> checking, either don't need return value, either don't need local
> variable 'cnt'.
> 

2 information:

 - this way (base on nr_cpu_ids, not snprintf) is not extensible.
   when add new printing contents, need modify maximized length.
   if acceptable to you, I will go (or do you have any new ideas?).

 - sorry, I have some internal urgent things to do, so may not finish
   within this week, and I will finish it in this month (2013-10-31).


Thanks.
-- 
Chen Gang

[PATCH] kernel/rcutorture.c: be sure of enough memory for result printing.

[Chen Gang]

If the contents is more than 4096 bytes (e.g. if have 1K cpus), current
sprintf() will cause memory overflow. And this fix patch is to be sure
of memory large enough.

Benefit:

 - do not truncate printing contents.
 - extensible, it is large enough for printing various related contents.
 - simple and clear enough for both source code readers and writers.

Shortcoming:

 - It will waste some memory:
    1 cpu may waste 24KB,
    10 cpus may waste 96KB,
    100 cpus may waste 816KB,
    1K cpus may waste 8MB
    ...
   after finish printing, it will free the related memory, quickly.
   it is a test module, so wast a little memory for extensible is OK.

Related  test (Fedora16 2 CPUs, 2GB RAM x86_64)

 - as module, with/without "torture_type=srcu".
 - build-in not boot runnable, with/without "torture_type=srcu".
 - build-in let boot runnable, with/without "torture_type=srcu".


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 kernel/rcutorture.c |   67 ++++++++++++++++++++++++++-------------------------
 1 files changed, 34 insertions(+), 33 deletions(-)

diff --git a/kernel/rcutorture.c b/kernel/rcutorture.c
index be63101..3413bc1 100644
--- a/kernel/rcutorture.c
+++ b/kernel/rcutorture.c
@@ -133,8 +133,6 @@ MODULE_PARM_DESC(verbose, "Enable verbose debugging printk()s");
 #define VERBOSE_PRINTK_ERRSTRING(s) \
 	do { if (verbose) pr_alert("%s" TORTURE_FLAG "!!! " s "\n", torture_type); } while (0)
 
-static char printk_buf[4096];
-
 static int nrealreaders;
 static struct task_struct *writer_task;
 static struct task_struct **fakewriter_tasks;
@@ -370,7 +368,7 @@ struct rcu_torture_ops {
 	void (*call)(struct rcu_head *head, void (*func)(struct rcu_head *rcu));
 	void (*cb_barrier)(void);
 	void (*fqs)(void);
-	int (*stats)(char *page);
+	void (*stats)(char *page);
 	int irq_capable;
 	int can_boost;
 	const char *name;
@@ -572,21 +570,19 @@ static void srcu_torture_barrier(void)
 	srcu_barrier(&srcu_ctl);
 }
 
-static int srcu_torture_stats(char *page)
+static void srcu_torture_stats(char *page)
 {
-	int cnt = 0;
 	int cpu;
 	int idx = srcu_ctl.completed & 0x1;
 
-	cnt += sprintf(&page[cnt], "%s%s per-CPU(idx=%d):",
+	page += sprintf(page, "%s%s per-CPU(idx=%d):",
 		       torture_type, TORTURE_FLAG, idx);
 	for_each_possible_cpu(cpu) {
-		cnt += sprintf(&page[cnt], " %d(%lu,%lu)", cpu,
+		page += sprintf(page, " %d(%lu,%lu)", cpu,
 			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[!idx],
 			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[idx]);
 	}
-	cnt += sprintf(&page[cnt], "\n");
-	return cnt;
+	sprintf(page, "\n");
 }
 
 static void srcu_torture_synchronize_expedited(void)
@@ -1046,10 +1042,9 @@ rcu_torture_reader(void *arg)
 /*
  * Create an RCU-torture statistics message in the specified buffer.
  */
-static int
+static void
 rcu_torture_printk(char *page)
 {
-	int cnt = 0;
 	int cpu;
 	int i;
 	long pipesummary[RCU_TORTURE_PIPE_LEN + 1] = { 0 };
@@ -1065,8 +1060,8 @@ rcu_torture_printk(char *page)
 		if (pipesummary[i] != 0)
 			break;
 	}
-	cnt += sprintf(&page[cnt], "%s%s ", torture_type, TORTURE_FLAG);
-	cnt += sprintf(&page[cnt],
+	page += sprintf(page, "%s%s ", torture_type, TORTURE_FLAG);
+	page += sprintf(page,
 		       "rtc: %p ver: %lu tfle: %d rta: %d rtaf: %d rtf: %d ",
 		       rcu_torture_current,
 		       rcu_torture_current_version,
@@ -1074,53 +1069,52 @@ rcu_torture_printk(char *page)
 		       atomic_read(&n_rcu_torture_alloc),
 		       atomic_read(&n_rcu_torture_alloc_fail),
 		       atomic_read(&n_rcu_torture_free));
-	cnt += sprintf(&page[cnt], "rtmbe: %d rtbke: %ld rtbre: %ld ",
+	page += sprintf(page, "rtmbe: %d rtbke: %ld rtbre: %ld ",
 		       atomic_read(&n_rcu_torture_mberror),
 		       n_rcu_torture_boost_ktrerror,
 		       n_rcu_torture_boost_rterror);
-	cnt += sprintf(&page[cnt], "rtbf: %ld rtb: %ld nt: %ld ",
+	page += sprintf(page, "rtbf: %ld rtb: %ld nt: %ld ",
 		       n_rcu_torture_boost_failure,
 		       n_rcu_torture_boosts,
 		       n_rcu_torture_timers);
-	cnt += sprintf(&page[cnt],
+	page += sprintf(page,
 		       "onoff: %ld/%ld:%ld/%ld %d,%d:%d,%d %lu:%lu (HZ=%d) ",
 		       n_online_successes, n_online_attempts,
 		       n_offline_successes, n_offline_attempts,
 		       min_online, max_online,
 		       min_offline, max_offline,
 		       sum_online, sum_offline, HZ);
-	cnt += sprintf(&page[cnt], "barrier: %ld/%ld:%ld",
+	page += sprintf(page, "barrier: %ld/%ld:%ld",
 		       n_barrier_successes,
 		       n_barrier_attempts,
 		       n_rcu_torture_barrier_error);
-	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
+	page += sprintf(page, "\n%s%s ", torture_type, TORTURE_FLAG);
 	if (atomic_read(&n_rcu_torture_mberror) != 0 ||
 	    n_rcu_torture_barrier_error != 0 ||
 	    n_rcu_torture_boost_ktrerror != 0 ||
 	    n_rcu_torture_boost_rterror != 0 ||
 	    n_rcu_torture_boost_failure != 0 ||
 	    i > 1) {
-		cnt += sprintf(&page[cnt], "!!! ");
+		page += sprintf(page, "!!! ");
 		atomic_inc(&n_rcu_torture_error);
 		WARN_ON_ONCE(1);
 	}
-	cnt += sprintf(&page[cnt], "Reader Pipe: ");
+	page += sprintf(page, "Reader Pipe: ");
 	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
-		cnt += sprintf(&page[cnt], " %ld", pipesummary[i]);
-	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
-	cnt += sprintf(&page[cnt], "Reader Batch: ");
+		page += sprintf(page, " %ld", pipesummary[i]);
+	page += sprintf(page, "\n%s%s ", torture_type, TORTURE_FLAG);
+	page += sprintf(page, "Reader Batch: ");
 	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
-		cnt += sprintf(&page[cnt], " %ld", batchsummary[i]);
-	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
-	cnt += sprintf(&page[cnt], "Free-Block Circulation: ");
+		page += sprintf(page, " %ld", batchsummary[i]);
+	page += sprintf(page, "\n%s%s ", torture_type, TORTURE_FLAG);
+	page += sprintf(page, "Free-Block Circulation: ");
 	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++) {
-		cnt += sprintf(&page[cnt], " %d",
+		page += sprintf(page, " %d",
 			       atomic_read(&rcu_torture_wcount[i]));
 	}
-	cnt += sprintf(&page[cnt], "\n");
+	page += sprintf(page, "\n");
 	if (cur_ops->stats)
-		cnt += cur_ops->stats(&page[cnt]);
-	return cnt;
+		 cur_ops->stats(page);
 }
 
 /*
@@ -1134,10 +1128,17 @@ rcu_torture_printk(char *page)
 static void
 rcu_torture_stats_print(void)
 {
-	int cnt;
+	int size = (nr_cpu_ids + 2) * PAGE_SIZE; /* be sure of large enough */
+	char *buf;
 
-	cnt = rcu_torture_printk(printk_buf);
-	pr_alert("%s", printk_buf);
+	buf = kmalloc(size, GFP_KERNEL);
+	if (!buf) {
+		pr_err("no enough memory for printing, requre: %d", size);
+		return;
+	}
+	rcu_torture_printk(buf);
+	pr_alert("%s", buf);
+	kfree(buf);
 }
 
 /*
-- 
1.7.7.6

Re: [PATCH] kernel/rcutorture.c: be sure of enough memory for result printing.

[Chen Gang]

Oh, sorry, I forgot to let it pass "./scripts/checkpatch.pl", after
finish checking, it finds a style issue (which is pointed out below), if
necessary to send patch v2, please let me know.

Thanks.

On 10/21/2013 01:51 PM, Chen Gang wrote:
> If the contents is more than 4096 bytes (e.g. if have 1K cpus), current
> sprintf() will cause memory overflow. And this fix patch is to be sure
> of memory large enough.
> 
> Benefit:
> 
>  - do not truncate printing contents.
>  - extensible, it is large enough for printing various related contents.
>  - simple and clear enough for both source code readers and writers.
> 
> Shortcoming:
> 
>  - It will waste some memory:
>     1 cpu may waste 24KB,
>     10 cpus may waste 96KB,
>     100 cpus may waste 816KB,
>     1K cpus may waste 8MB
>     ...
>    after finish printing, it will free the related memory, quickly.
>    it is a test module, so wast a little memory for extensible is OK.
> 
> Related  test (Fedora16 2 CPUs, 2GB RAM x86_64)
> 
>  - as module, with/without "torture_type=srcu".
>  - build-in not boot runnable, with/without "torture_type=srcu".
>  - build-in let boot runnable, with/without "torture_type=srcu".
> 
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  kernel/rcutorture.c |   67 ++++++++++++++++++++++++++-------------------------
>  1 files changed, 34 insertions(+), 33 deletions(-)
> 
> diff --git a/kernel/rcutorture.c b/kernel/rcutorture.c
> index be63101..3413bc1 100644
> --- a/kernel/rcutorture.c
> +++ b/kernel/rcutorture.c
> @@ -133,8 +133,6 @@ MODULE_PARM_DESC(verbose, "Enable verbose debugging printk()s");
>  #define VERBOSE_PRINTK_ERRSTRING(s) \
>  	do { if (verbose) pr_alert("%s" TORTURE_FLAG "!!! " s "\n", torture_type); } while (0)
>  
> -static char printk_buf[4096];
> -
>  static int nrealreaders;
>  static struct task_struct *writer_task;
>  static struct task_struct **fakewriter_tasks;
> @@ -370,7 +368,7 @@ struct rcu_torture_ops {
>  	void (*call)(struct rcu_head *head, void (*func)(struct rcu_head *rcu));
>  	void (*cb_barrier)(void);
>  	void (*fqs)(void);
> -	int (*stats)(char *page);
> +	void (*stats)(char *page);
>  	int irq_capable;
>  	int can_boost;
>  	const char *name;
> @@ -572,21 +570,19 @@ static void srcu_torture_barrier(void)
>  	srcu_barrier(&srcu_ctl);
>  }
>  
> -static int srcu_torture_stats(char *page)
> +static void srcu_torture_stats(char *page)
>  {
> -	int cnt = 0;
>  	int cpu;
>  	int idx = srcu_ctl.completed & 0x1;
>  
> -	cnt += sprintf(&page[cnt], "%s%s per-CPU(idx=%d):",
> +	page += sprintf(page, "%s%s per-CPU(idx=%d):",
>  		       torture_type, TORTURE_FLAG, idx);
>  	for_each_possible_cpu(cpu) {
> -		cnt += sprintf(&page[cnt], " %d(%lu,%lu)", cpu,
> +		page += sprintf(page, " %d(%lu,%lu)", cpu,
>  			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[!idx],
>  			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[idx]);
>  	}
> -	cnt += sprintf(&page[cnt], "\n");
> -	return cnt;
> +	sprintf(page, "\n");
>  }
>  
>  static void srcu_torture_synchronize_expedited(void)
> @@ -1046,10 +1042,9 @@ rcu_torture_reader(void *arg)
>  /*
>   * Create an RCU-torture statistics message in the specified buffer.
>   */
> -static int
> +static void
>  rcu_torture_printk(char *page)
>  {
> -	int cnt = 0;
>  	int cpu;
>  	int i;
>  	long pipesummary[RCU_TORTURE_PIPE_LEN + 1] = { 0 };
> @@ -1065,8 +1060,8 @@ rcu_torture_printk(char *page)
>  		if (pipesummary[i] != 0)
>  			break;
>  	}
> -	cnt += sprintf(&page[cnt], "%s%s ", torture_type, TORTURE_FLAG);
> -	cnt += sprintf(&page[cnt],
> +	page += sprintf(page, "%s%s ", torture_type, TORTURE_FLAG);
> +	page += sprintf(page,
>  		       "rtc: %p ver: %lu tfle: %d rta: %d rtaf: %d rtf: %d ",
>  		       rcu_torture_current,
>  		       rcu_torture_current_version,
> @@ -1074,53 +1069,52 @@ rcu_torture_printk(char *page)
>  		       atomic_read(&n_rcu_torture_alloc),
>  		       atomic_read(&n_rcu_torture_alloc_fail),
>  		       atomic_read(&n_rcu_torture_free));
> -	cnt += sprintf(&page[cnt], "rtmbe: %d rtbke: %ld rtbre: %ld ",
> +	page += sprintf(page, "rtmbe: %d rtbke: %ld rtbre: %ld ",
>  		       atomic_read(&n_rcu_torture_mberror),
>  		       n_rcu_torture_boost_ktrerror,
>  		       n_rcu_torture_boost_rterror);
> -	cnt += sprintf(&page[cnt], "rtbf: %ld rtb: %ld nt: %ld ",
> +	page += sprintf(page, "rtbf: %ld rtb: %ld nt: %ld ",
>  		       n_rcu_torture_boost_failure,
>  		       n_rcu_torture_boosts,
>  		       n_rcu_torture_timers);
> -	cnt += sprintf(&page[cnt],
> +	page += sprintf(page,
>  		       "onoff: %ld/%ld:%ld/%ld %d,%d:%d,%d %lu:%lu (HZ=%d) ",
>  		       n_online_successes, n_online_attempts,
>  		       n_offline_successes, n_offline_attempts,
>  		       min_online, max_online,
>  		       min_offline, max_offline,
>  		       sum_online, sum_offline, HZ);
> -	cnt += sprintf(&page[cnt], "barrier: %ld/%ld:%ld",
> +	page += sprintf(page, "barrier: %ld/%ld:%ld",
>  		       n_barrier_successes,
>  		       n_barrier_attempts,
>  		       n_rcu_torture_barrier_error);
> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
> +	page += sprintf(page, "\n%s%s ", torture_type, TORTURE_FLAG);
>  	if (atomic_read(&n_rcu_torture_mberror) != 0 ||
>  	    n_rcu_torture_barrier_error != 0 ||
>  	    n_rcu_torture_boost_ktrerror != 0 ||
>  	    n_rcu_torture_boost_rterror != 0 ||
>  	    n_rcu_torture_boost_failure != 0 ||
>  	    i > 1) {
> -		cnt += sprintf(&page[cnt], "!!! ");
> +		page += sprintf(page, "!!! ");
>  		atomic_inc(&n_rcu_torture_error);
>  		WARN_ON_ONCE(1);
>  	}
> -	cnt += sprintf(&page[cnt], "Reader Pipe: ");
> +	page += sprintf(page, "Reader Pipe: ");
>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
> -		cnt += sprintf(&page[cnt], " %ld", pipesummary[i]);
> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
> -	cnt += sprintf(&page[cnt], "Reader Batch: ");
> +		page += sprintf(page, " %ld", pipesummary[i]);
> +	page += sprintf(page, "\n%s%s ", torture_type, TORTURE_FLAG);
> +	page += sprintf(page, "Reader Batch: ");
>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
> -		cnt += sprintf(&page[cnt], " %ld", batchsummary[i]);
> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
> -	cnt += sprintf(&page[cnt], "Free-Block Circulation: ");
> +		page += sprintf(page, " %ld", batchsummary[i]);
> +	page += sprintf(page, "\n%s%s ", torture_type, TORTURE_FLAG);
> +	page += sprintf(page, "Free-Block Circulation: ");
>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++) {
> -		cnt += sprintf(&page[cnt], " %d",
> +		page += sprintf(page, " %d",
>  			       atomic_read(&rcu_torture_wcount[i]));
>  	}
> -	cnt += sprintf(&page[cnt], "\n");
> +	page += sprintf(page, "\n");
>  	if (cur_ops->stats)
> -		cnt += cur_ops->stats(&page[cnt]);
> -	return cnt;
> +		 cur_ops->stats(page);

Oh, sorry, this line has a waste white space.

>  }
>  
>  /*
> @@ -1134,10 +1128,17 @@ rcu_torture_printk(char *page)
>  static void
>  rcu_torture_stats_print(void)
>  {
> -	int cnt;
> +	int size = (nr_cpu_ids + 2) * PAGE_SIZE; /* be sure of large enough */
> +	char *buf;
>  
> -	cnt = rcu_torture_printk(printk_buf);
> -	pr_alert("%s", printk_buf);
> +	buf = kmalloc(size, GFP_KERNEL);
> +	if (!buf) {
> +		pr_err("no enough memory for printing, requre: %d", size);
> +		return;
> +	}
> +	rcu_torture_printk(buf);
> +	pr_alert("%s", buf);
> +	kfree(buf);
>  }
>  
>  /*
> 


-- 
Chen Gang

Re: [PATCH] kernel/rcutorture.c: be sure of enough memory for result printing.

[Chen Gang]

BTW: I use kmalloc(), and still can pass build-in and let boot runnable
under my laptop, but I think it is not quite precise, may still need
more think of, please check (or give a confirmation whether it is OK).

Hmm... maybe the pr_err("...") also need "srcu-torture" prefix just like
another printing to let checkers easy search?

Thanks.

On 10/21/2013 02:18 PM, Chen Gang wrote:
> 
> Oh, sorry, I forgot to let it pass "./scripts/checkpatch.pl", after
> finish checking, it finds a style issue (which is pointed out below), if
> necessary to send patch v2, please let me know.
> 
> Thanks.
> 
> On 10/21/2013 01:51 PM, Chen Gang wrote:
>> If the contents is more than 4096 bytes (e.g. if have 1K cpus), current
>> sprintf() will cause memory overflow. And this fix patch is to be sure
>> of memory large enough.
>>
>> Benefit:
>>
>>  - do not truncate printing contents.
>>  - extensible, it is large enough for printing various related contents.
>>  - simple and clear enough for both source code readers and writers.
>>
>> Shortcoming:
>>
>>  - It will waste some memory:
>>     1 cpu may waste 24KB,
>>     10 cpus may waste 96KB,
>>     100 cpus may waste 816KB,
>>     1K cpus may waste 8MB
>>     ...
>>    after finish printing, it will free the related memory, quickly.
>>    it is a test module, so wast a little memory for extensible is OK.
>>
>> Related  test (Fedora16 2 CPUs, 2GB RAM x86_64)
>>
>>  - as module, with/without "torture_type=srcu".
>>  - build-in not boot runnable, with/without "torture_type=srcu".
>>  - build-in let boot runnable, with/without "torture_type=srcu".
>>
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> ---
>>  kernel/rcutorture.c |   67 ++++++++++++++++++++++++++-------------------------
>>  1 files changed, 34 insertions(+), 33 deletions(-)
>>
>> diff --git a/kernel/rcutorture.c b/kernel/rcutorture.c
>> index be63101..3413bc1 100644
>> --- a/kernel/rcutorture.c
>> +++ b/kernel/rcutorture.c
>> @@ -133,8 +133,6 @@ MODULE_PARM_DESC(verbose, "Enable verbose debugging printk()s");
>>  #define VERBOSE_PRINTK_ERRSTRING(s) \
>>  	do { if (verbose) pr_alert("%s" TORTURE_FLAG "!!! " s "\n", torture_type); } while (0)
>>  
>> -static char printk_buf[4096];
>> -
>>  static int nrealreaders;
>>  static struct task_struct *writer_task;
>>  static struct task_struct **fakewriter_tasks;
>> @@ -370,7 +368,7 @@ struct rcu_torture_ops {
>>  	void (*call)(struct rcu_head *head, void (*func)(struct rcu_head *rcu));
>>  	void (*cb_barrier)(void);
>>  	void (*fqs)(void);
>> -	int (*stats)(char *page);
>> +	void (*stats)(char *page);
>>  	int irq_capable;
>>  	int can_boost;
>>  	const char *name;
>> @@ -572,21 +570,19 @@ static void srcu_torture_barrier(void)
>>  	srcu_barrier(&srcu_ctl);
>>  }
>>  
>> -static int srcu_torture_stats(char *page)
>> +static void srcu_torture_stats(char *page)
>>  {
>> -	int cnt = 0;
>>  	int cpu;
>>  	int idx = srcu_ctl.completed & 0x1;
>>  
>> -	cnt += sprintf(&page[cnt], "%s%s per-CPU(idx=%d):",
>> +	page += sprintf(page, "%s%s per-CPU(idx=%d):",
>>  		       torture_type, TORTURE_FLAG, idx);
>>  	for_each_possible_cpu(cpu) {
>> -		cnt += sprintf(&page[cnt], " %d(%lu,%lu)", cpu,
>> +		page += sprintf(page, " %d(%lu,%lu)", cpu,
>>  			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[!idx],
>>  			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[idx]);
>>  	}
>> -	cnt += sprintf(&page[cnt], "\n");
>> -	return cnt;
>> +	sprintf(page, "\n");
>>  }
>>  
>>  static void srcu_torture_synchronize_expedited(void)
>> @@ -1046,10 +1042,9 @@ rcu_torture_reader(void *arg)
>>  /*
>>   * Create an RCU-torture statistics message in the specified buffer.
>>   */
>> -static int
>> +static void
>>  rcu_torture_printk(char *page)
>>  {
>> -	int cnt = 0;
>>  	int cpu;
>>  	int i;
>>  	long pipesummary[RCU_TORTURE_PIPE_LEN + 1] = { 0 };
>> @@ -1065,8 +1060,8 @@ rcu_torture_printk(char *page)
>>  		if (pipesummary[i] != 0)
>>  			break;
>>  	}
>> -	cnt += sprintf(&page[cnt], "%s%s ", torture_type, TORTURE_FLAG);
>> -	cnt += sprintf(&page[cnt],
>> +	page += sprintf(page, "%s%s ", torture_type, TORTURE_FLAG);
>> +	page += sprintf(page,
>>  		       "rtc: %p ver: %lu tfle: %d rta: %d rtaf: %d rtf: %d ",
>>  		       rcu_torture_current,
>>  		       rcu_torture_current_version,
>> @@ -1074,53 +1069,52 @@ rcu_torture_printk(char *page)
>>  		       atomic_read(&n_rcu_torture_alloc),
>>  		       atomic_read(&n_rcu_torture_alloc_fail),
>>  		       atomic_read(&n_rcu_torture_free));
>> -	cnt += sprintf(&page[cnt], "rtmbe: %d rtbke: %ld rtbre: %ld ",
>> +	page += sprintf(page, "rtmbe: %d rtbke: %ld rtbre: %ld ",
>>  		       atomic_read(&n_rcu_torture_mberror),
>>  		       n_rcu_torture_boost_ktrerror,
>>  		       n_rcu_torture_boost_rterror);
>> -	cnt += sprintf(&page[cnt], "rtbf: %ld rtb: %ld nt: %ld ",
>> +	page += sprintf(page, "rtbf: %ld rtb: %ld nt: %ld ",
>>  		       n_rcu_torture_boost_failure,
>>  		       n_rcu_torture_boosts,
>>  		       n_rcu_torture_timers);
>> -	cnt += sprintf(&page[cnt],
>> +	page += sprintf(page,
>>  		       "onoff: %ld/%ld:%ld/%ld %d,%d:%d,%d %lu:%lu (HZ=%d) ",
>>  		       n_online_successes, n_online_attempts,
>>  		       n_offline_successes, n_offline_attempts,
>>  		       min_online, max_online,
>>  		       min_offline, max_offline,
>>  		       sum_online, sum_offline, HZ);
>> -	cnt += sprintf(&page[cnt], "barrier: %ld/%ld:%ld",
>> +	page += sprintf(page, "barrier: %ld/%ld:%ld",
>>  		       n_barrier_successes,
>>  		       n_barrier_attempts,
>>  		       n_rcu_torture_barrier_error);
>> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
>> +	page += sprintf(page, "\n%s%s ", torture_type, TORTURE_FLAG);
>>  	if (atomic_read(&n_rcu_torture_mberror) != 0 ||
>>  	    n_rcu_torture_barrier_error != 0 ||
>>  	    n_rcu_torture_boost_ktrerror != 0 ||
>>  	    n_rcu_torture_boost_rterror != 0 ||
>>  	    n_rcu_torture_boost_failure != 0 ||
>>  	    i > 1) {
>> -		cnt += sprintf(&page[cnt], "!!! ");
>> +		page += sprintf(page, "!!! ");
>>  		atomic_inc(&n_rcu_torture_error);
>>  		WARN_ON_ONCE(1);
>>  	}
>> -	cnt += sprintf(&page[cnt], "Reader Pipe: ");
>> +	page += sprintf(page, "Reader Pipe: ");
>>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
>> -		cnt += sprintf(&page[cnt], " %ld", pipesummary[i]);
>> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
>> -	cnt += sprintf(&page[cnt], "Reader Batch: ");
>> +		page += sprintf(page, " %ld", pipesummary[i]);
>> +	page += sprintf(page, "\n%s%s ", torture_type, TORTURE_FLAG);
>> +	page += sprintf(page, "Reader Batch: ");
>>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
>> -		cnt += sprintf(&page[cnt], " %ld", batchsummary[i]);
>> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
>> -	cnt += sprintf(&page[cnt], "Free-Block Circulation: ");
>> +		page += sprintf(page, " %ld", batchsummary[i]);
>> +	page += sprintf(page, "\n%s%s ", torture_type, TORTURE_FLAG);
>> +	page += sprintf(page, "Free-Block Circulation: ");
>>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++) {
>> -		cnt += sprintf(&page[cnt], " %d",
>> +		page += sprintf(page, " %d",
>>  			       atomic_read(&rcu_torture_wcount[i]));
>>  	}
>> -	cnt += sprintf(&page[cnt], "\n");
>> +	page += sprintf(page, "\n");
>>  	if (cur_ops->stats)
>> -		cnt += cur_ops->stats(&page[cnt]);
>> -	return cnt;
>> +		 cur_ops->stats(page);
> 
> Oh, sorry, this line has a waste white space.
> 
>>  }
>>  
>>  /*
>> @@ -1134,10 +1128,17 @@ rcu_torture_printk(char *page)
>>  static void
>>  rcu_torture_stats_print(void)
>>  {
>> -	int cnt;
>> +	int size = (nr_cpu_ids + 2) * PAGE_SIZE; /* be sure of large enough */
>> +	char *buf;
>>  
>> -	cnt = rcu_torture_printk(printk_buf);
>> -	pr_alert("%s", printk_buf);
>> +	buf = kmalloc(size, GFP_KERNEL);
>> +	if (!buf) {
>> +		pr_err("no enough memory for printing, requre: %d", size);
>> +		return;
>> +	}
>> +	rcu_torture_printk(buf);
>> +	pr_alert("%s", buf);
>> +	kfree(buf);
>>  }
>>  
>>  /*
>>
> 
> 


-- 
Chen Gang

Re: [PATCH] kernel/rcutorture.c: be sure of enough memory for result printing.

[Chen Gang]

Hello Maintainers:

Is it patch OK or not? when you have time, please help check (if it is
necessary to send patch v2, please let me know).

Thanks.

On 10/21/2013 05:35 PM, Chen Gang wrote:
> 
> BTW: I use kmalloc(), and still can pass build-in and let boot runnable
> under my laptop, but I think it is not quite precise, may still need
> more think of, please check (or give a confirmation whether it is OK).
> 
> Hmm... maybe the pr_err("...") also need "srcu-torture" prefix just like
> another printing to let checkers easy search?
> 
> Thanks.
> 
> On 10/21/2013 02:18 PM, Chen Gang wrote:
>>
>> Oh, sorry, I forgot to let it pass "./scripts/checkpatch.pl", after
>> finish checking, it finds a style issue (which is pointed out below), if
>> necessary to send patch v2, please let me know.
>>
>> Thanks.
>>
>> On 10/21/2013 01:51 PM, Chen Gang wrote:
>>> If the contents is more than 4096 bytes (e.g. if have 1K cpus), current
>>> sprintf() will cause memory overflow. And this fix patch is to be sure
>>> of memory large enough.
>>>
>>> Benefit:
>>>
>>>  - do not truncate printing contents.
>>>  - extensible, it is large enough for printing various related contents.
>>>  - simple and clear enough for both source code readers and writers.
>>>
>>> Shortcoming:
>>>
>>>  - It will waste some memory:
>>>     1 cpu may waste 24KB,
>>>     10 cpus may waste 96KB,
>>>     100 cpus may waste 816KB,
>>>     1K cpus may waste 8MB
>>>     ...
>>>    after finish printing, it will free the related memory, quickly.
>>>    it is a test module, so wast a little memory for extensible is OK.
>>>
>>> Related  test (Fedora16 2 CPUs, 2GB RAM x86_64)
>>>
>>>  - as module, with/without "torture_type=srcu".
>>>  - build-in not boot runnable, with/without "torture_type=srcu".
>>>  - build-in let boot runnable, with/without "torture_type=srcu".
>>>
>>>
>>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>>> ---
>>>  kernel/rcutorture.c |   67 ++++++++++++++++++++++++++-------------------------
>>>  1 files changed, 34 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/kernel/rcutorture.c b/kernel/rcutorture.c
>>> index be63101..3413bc1 100644
>>> --- a/kernel/rcutorture.c
>>> +++ b/kernel/rcutorture.c
>>> @@ -133,8 +133,6 @@ MODULE_PARM_DESC(verbose, "Enable verbose debugging printk()s");
>>>  #define VERBOSE_PRINTK_ERRSTRING(s) \
>>>  	do { if (verbose) pr_alert("%s" TORTURE_FLAG "!!! " s "\n", torture_type); } while (0)
>>>  
>>> -static char printk_buf[4096];
>>> -
>>>  static int nrealreaders;
>>>  static struct task_struct *writer_task;
>>>  static struct task_struct **fakewriter_tasks;
>>> @@ -370,7 +368,7 @@ struct rcu_torture_ops {
>>>  	void (*call)(struct rcu_head *head, void (*func)(struct rcu_head *rcu));
>>>  	void (*cb_barrier)(void);
>>>  	void (*fqs)(void);
>>> -	int (*stats)(char *page);
>>> +	void (*stats)(char *page);
>>>  	int irq_capable;
>>>  	int can_boost;
>>>  	const char *name;
>>> @@ -572,21 +570,19 @@ static void srcu_torture_barrier(void)
>>>  	srcu_barrier(&srcu_ctl);
>>>  }
>>>  
>>> -static int srcu_torture_stats(char *page)
>>> +static void srcu_torture_stats(char *page)
>>>  {
>>> -	int cnt = 0;
>>>  	int cpu;
>>>  	int idx = srcu_ctl.completed & 0x1;
>>>  
>>> -	cnt += sprintf(&page[cnt], "%s%s per-CPU(idx=%d):",
>>> +	page += sprintf(page, "%s%s per-CPU(idx=%d):",
>>>  		       torture_type, TORTURE_FLAG, idx);
>>>  	for_each_possible_cpu(cpu) {
>>> -		cnt += sprintf(&page[cnt], " %d(%lu,%lu)", cpu,
>>> +		page += sprintf(page, " %d(%lu,%lu)", cpu,
>>>  			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[!idx],
>>>  			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[idx]);
>>>  	}
>>> -	cnt += sprintf(&page[cnt], "\n");
>>> -	return cnt;
>>> +	sprintf(page, "\n");
>>>  }
>>>  
>>>  static void srcu_torture_synchronize_expedited(void)
>>> @@ -1046,10 +1042,9 @@ rcu_torture_reader(void *arg)
>>>  /*
>>>   * Create an RCU-torture statistics message in the specified buffer.
>>>   */
>>> -static int
>>> +static void
>>>  rcu_torture_printk(char *page)
>>>  {
>>> -	int cnt = 0;
>>>  	int cpu;
>>>  	int i;
>>>  	long pipesummary[RCU_TORTURE_PIPE_LEN + 1] = { 0 };
>>> @@ -1065,8 +1060,8 @@ rcu_torture_printk(char *page)
>>>  		if (pipesummary[i] != 0)
>>>  			break;
>>>  	}
>>> -	cnt += sprintf(&page[cnt], "%s%s ", torture_type, TORTURE_FLAG);
>>> -	cnt += sprintf(&page[cnt],
>>> +	page += sprintf(page, "%s%s ", torture_type, TORTURE_FLAG);
>>> +	page += sprintf(page,
>>>  		       "rtc: %p ver: %lu tfle: %d rta: %d rtaf: %d rtf: %d ",
>>>  		       rcu_torture_current,
>>>  		       rcu_torture_current_version,
>>> @@ -1074,53 +1069,52 @@ rcu_torture_printk(char *page)
>>>  		       atomic_read(&n_rcu_torture_alloc),
>>>  		       atomic_read(&n_rcu_torture_alloc_fail),
>>>  		       atomic_read(&n_rcu_torture_free));
>>> -	cnt += sprintf(&page[cnt], "rtmbe: %d rtbke: %ld rtbre: %ld ",
>>> +	page += sprintf(page, "rtmbe: %d rtbke: %ld rtbre: %ld ",
>>>  		       atomic_read(&n_rcu_torture_mberror),
>>>  		       n_rcu_torture_boost_ktrerror,
>>>  		       n_rcu_torture_boost_rterror);
>>> -	cnt += sprintf(&page[cnt], "rtbf: %ld rtb: %ld nt: %ld ",
>>> +	page += sprintf(page, "rtbf: %ld rtb: %ld nt: %ld ",
>>>  		       n_rcu_torture_boost_failure,
>>>  		       n_rcu_torture_boosts,
>>>  		       n_rcu_torture_timers);
>>> -	cnt += sprintf(&page[cnt],
>>> +	page += sprintf(page,
>>>  		       "onoff: %ld/%ld:%ld/%ld %d,%d:%d,%d %lu:%lu (HZ=%d) ",
>>>  		       n_online_successes, n_online_attempts,
>>>  		       n_offline_successes, n_offline_attempts,
>>>  		       min_online, max_online,
>>>  		       min_offline, max_offline,
>>>  		       sum_online, sum_offline, HZ);
>>> -	cnt += sprintf(&page[cnt], "barrier: %ld/%ld:%ld",
>>> +	page += sprintf(page, "barrier: %ld/%ld:%ld",
>>>  		       n_barrier_successes,
>>>  		       n_barrier_attempts,
>>>  		       n_rcu_torture_barrier_error);
>>> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
>>> +	page += sprintf(page, "\n%s%s ", torture_type, TORTURE_FLAG);
>>>  	if (atomic_read(&n_rcu_torture_mberror) != 0 ||
>>>  	    n_rcu_torture_barrier_error != 0 ||
>>>  	    n_rcu_torture_boost_ktrerror != 0 ||
>>>  	    n_rcu_torture_boost_rterror != 0 ||
>>>  	    n_rcu_torture_boost_failure != 0 ||
>>>  	    i > 1) {
>>> -		cnt += sprintf(&page[cnt], "!!! ");
>>> +		page += sprintf(page, "!!! ");
>>>  		atomic_inc(&n_rcu_torture_error);
>>>  		WARN_ON_ONCE(1);
>>>  	}
>>> -	cnt += sprintf(&page[cnt], "Reader Pipe: ");
>>> +	page += sprintf(page, "Reader Pipe: ");
>>>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
>>> -		cnt += sprintf(&page[cnt], " %ld", pipesummary[i]);
>>> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
>>> -	cnt += sprintf(&page[cnt], "Reader Batch: ");
>>> +		page += sprintf(page, " %ld", pipesummary[i]);
>>> +	page += sprintf(page, "\n%s%s ", torture_type, TORTURE_FLAG);
>>> +	page += sprintf(page, "Reader Batch: ");
>>>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
>>> -		cnt += sprintf(&page[cnt], " %ld", batchsummary[i]);
>>> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
>>> -	cnt += sprintf(&page[cnt], "Free-Block Circulation: ");
>>> +		page += sprintf(page, " %ld", batchsummary[i]);
>>> +	page += sprintf(page, "\n%s%s ", torture_type, TORTURE_FLAG);
>>> +	page += sprintf(page, "Free-Block Circulation: ");
>>>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++) {
>>> -		cnt += sprintf(&page[cnt], " %d",
>>> +		page += sprintf(page, " %d",
>>>  			       atomic_read(&rcu_torture_wcount[i]));
>>>  	}
>>> -	cnt += sprintf(&page[cnt], "\n");
>>> +	page += sprintf(page, "\n");
>>>  	if (cur_ops->stats)
>>> -		cnt += cur_ops->stats(&page[cnt]);
>>> -	return cnt;
>>> +		 cur_ops->stats(page);
>>
>> Oh, sorry, this line has a waste white space.
>>
>>>  }
>>>  
>>>  /*
>>> @@ -1134,10 +1128,17 @@ rcu_torture_printk(char *page)
>>>  static void
>>>  rcu_torture_stats_print(void)
>>>  {
>>> -	int cnt;
>>> +	int size = (nr_cpu_ids + 2) * PAGE_SIZE; /* be sure of large enough */
>>> +	char *buf;
>>>  
>>> -	cnt = rcu_torture_printk(printk_buf);
>>> -	pr_alert("%s", printk_buf);
>>> +	buf = kmalloc(size, GFP_KERNEL);
>>> +	if (!buf) {
>>> +		pr_err("no enough memory for printing, requre: %d", size);
>>> +		return;
>>> +	}
>>> +	rcu_torture_printk(buf);
>>> +	pr_alert("%s", buf);
>>> +	kfree(buf);
>>>  }
>>>  
>>>  /*
>>>
>>
>>
> 
> 


-- 
Chen Gang

Re: [PATCH] kernel/rcutorture.c: be sure of enough memory for result printing.

[Chen Gang]

Hello Maintainers:

Is the patch OK? If you want to fix it by yourself, please let me know.

Thanks.

On 10/27/2013 10:43 PM, Chen Gang wrote:
> Hello Maintainers:
> 
> Is it patch OK or not? when you have time, please help check (if it is
> necessary to send patch v2, please let me know).
> 
> Thanks.
> 
> On 10/21/2013 05:35 PM, Chen Gang wrote:
>>
>> BTW: I use kmalloc(), and still can pass build-in and let boot runnable
>> under my laptop, but I think it is not quite precise, may still need
>> more think of, please check (or give a confirmation whether it is OK).
>>
>> Hmm... maybe the pr_err("...") also need "srcu-torture" prefix just like
>> another printing to let checkers easy search?
>>
>> Thanks.
>>
>> On 10/21/2013 02:18 PM, Chen Gang wrote:
>>>
>>> Oh, sorry, I forgot to let it pass "./scripts/checkpatch.pl", after
>>> finish checking, it finds a style issue (which is pointed out below), if
>>> necessary to send patch v2, please let me know.
>>>
>>> Thanks.
>>>
>>> On 10/21/2013 01:51 PM, Chen Gang wrote:
>>>> If the contents is more than 4096 bytes (e.g. if have 1K cpus), current
>>>> sprintf() will cause memory overflow. And this fix patch is to be sure
>>>> of memory large enough.
>>>>
>>>> Benefit:
>>>>
>>>>  - do not truncate printing contents.
>>>>  - extensible, it is large enough for printing various related contents.
>>>>  - simple and clear enough for both source code readers and writers.
>>>>
>>>> Shortcoming:
>>>>
>>>>  - It will waste some memory:
>>>>     1 cpu may waste 24KB,
>>>>     10 cpus may waste 96KB,
>>>>     100 cpus may waste 816KB,
>>>>     1K cpus may waste 8MB
>>>>     ...
>>>>    after finish printing, it will free the related memory, quickly.
>>>>    it is a test module, so wast a little memory for extensible is OK.
>>>>
>>>> Related  test (Fedora16 2 CPUs, 2GB RAM x86_64)
>>>>
>>>>  - as module, with/without "torture_type=srcu".
>>>>  - build-in not boot runnable, with/without "torture_type=srcu".
>>>>  - build-in let boot runnable, with/without "torture_type=srcu".
>>>>
>>>>
>>>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>>>> ---
>>>>  kernel/rcutorture.c |   67 ++++++++++++++++++++++++++-------------------------
>>>>  1 files changed, 34 insertions(+), 33 deletions(-)
>>>>
>>>> diff --git a/kernel/rcutorture.c b/kernel/rcutorture.c
>>>> index be63101..3413bc1 100644
>>>> --- a/kernel/rcutorture.c
>>>> +++ b/kernel/rcutorture.c
>>>> @@ -133,8 +133,6 @@ MODULE_PARM_DESC(verbose, "Enable verbose debugging printk()s");
>>>>  #define VERBOSE_PRINTK_ERRSTRING(s) \
>>>>  	do { if (verbose) pr_alert("%s" TORTURE_FLAG "!!! " s "\n", torture_type); } while (0)
>>>>  
>>>> -static char printk_buf[4096];
>>>> -
>>>>  static int nrealreaders;
>>>>  static struct task_struct *writer_task;
>>>>  static struct task_struct **fakewriter_tasks;
>>>> @@ -370,7 +368,7 @@ struct rcu_torture_ops {
>>>>  	void (*call)(struct rcu_head *head, void (*func)(struct rcu_head *rcu));
>>>>  	void (*cb_barrier)(void);
>>>>  	void (*fqs)(void);
>>>> -	int (*stats)(char *page);
>>>> +	void (*stats)(char *page);
>>>>  	int irq_capable;
>>>>  	int can_boost;
>>>>  	const char *name;
>>>> @@ -572,21 +570,19 @@ static void srcu_torture_barrier(void)
>>>>  	srcu_barrier(&srcu_ctl);
>>>>  }
>>>>  
>>>> -static int srcu_torture_stats(char *page)
>>>> +static void srcu_torture_stats(char *page)
>>>>  {
>>>> -	int cnt = 0;
>>>>  	int cpu;
>>>>  	int idx = srcu_ctl.completed & 0x1;
>>>>  
>>>> -	cnt += sprintf(&page[cnt], "%s%s per-CPU(idx=%d):",
>>>> +	page += sprintf(page, "%s%s per-CPU(idx=%d):",
>>>>  		       torture_type, TORTURE_FLAG, idx);
>>>>  	for_each_possible_cpu(cpu) {
>>>> -		cnt += sprintf(&page[cnt], " %d(%lu,%lu)", cpu,
>>>> +		page += sprintf(page, " %d(%lu,%lu)", cpu,
>>>>  			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[!idx],
>>>>  			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[idx]);
>>>>  	}
>>>> -	cnt += sprintf(&page[cnt], "\n");
>>>> -	return cnt;
>>>> +	sprintf(page, "\n");
>>>>  }
>>>>  
>>>>  static void srcu_torture_synchronize_expedited(void)
>>>> @@ -1046,10 +1042,9 @@ rcu_torture_reader(void *arg)
>>>>  /*
>>>>   * Create an RCU-torture statistics message in the specified buffer.
>>>>   */
>>>> -static int
>>>> +static void
>>>>  rcu_torture_printk(char *page)
>>>>  {
>>>> -	int cnt = 0;
>>>>  	int cpu;
>>>>  	int i;
>>>>  	long pipesummary[RCU_TORTURE_PIPE_LEN + 1] = { 0 };
>>>> @@ -1065,8 +1060,8 @@ rcu_torture_printk(char *page)
>>>>  		if (pipesummary[i] != 0)
>>>>  			break;
>>>>  	}
>>>> -	cnt += sprintf(&page[cnt], "%s%s ", torture_type, TORTURE_FLAG);
>>>> -	cnt += sprintf(&page[cnt],
>>>> +	page += sprintf(page, "%s%s ", torture_type, TORTURE_FLAG);
>>>> +	page += sprintf(page,
>>>>  		       "rtc: %p ver: %lu tfle: %d rta: %d rtaf: %d rtf: %d ",
>>>>  		       rcu_torture_current,
>>>>  		       rcu_torture_current_version,
>>>> @@ -1074,53 +1069,52 @@ rcu_torture_printk(char *page)
>>>>  		       atomic_read(&n_rcu_torture_alloc),
>>>>  		       atomic_read(&n_rcu_torture_alloc_fail),
>>>>  		       atomic_read(&n_rcu_torture_free));
>>>> -	cnt += sprintf(&page[cnt], "rtmbe: %d rtbke: %ld rtbre: %ld ",
>>>> +	page += sprintf(page, "rtmbe: %d rtbke: %ld rtbre: %ld ",
>>>>  		       atomic_read(&n_rcu_torture_mberror),
>>>>  		       n_rcu_torture_boost_ktrerror,
>>>>  		       n_rcu_torture_boost_rterror);
>>>> -	cnt += sprintf(&page[cnt], "rtbf: %ld rtb: %ld nt: %ld ",
>>>> +	page += sprintf(page, "rtbf: %ld rtb: %ld nt: %ld ",
>>>>  		       n_rcu_torture_boost_failure,
>>>>  		       n_rcu_torture_boosts,
>>>>  		       n_rcu_torture_timers);
>>>> -	cnt += sprintf(&page[cnt],
>>>> +	page += sprintf(page,
>>>>  		       "onoff: %ld/%ld:%ld/%ld %d,%d:%d,%d %lu:%lu (HZ=%d) ",
>>>>  		       n_online_successes, n_online_attempts,
>>>>  		       n_offline_successes, n_offline_attempts,
>>>>  		       min_online, max_online,
>>>>  		       min_offline, max_offline,
>>>>  		       sum_online, sum_offline, HZ);
>>>> -	cnt += sprintf(&page[cnt], "barrier: %ld/%ld:%ld",
>>>> +	page += sprintf(page, "barrier: %ld/%ld:%ld",
>>>>  		       n_barrier_successes,
>>>>  		       n_barrier_attempts,
>>>>  		       n_rcu_torture_barrier_error);
>>>> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
>>>> +	page += sprintf(page, "\n%s%s ", torture_type, TORTURE_FLAG);
>>>>  	if (atomic_read(&n_rcu_torture_mberror) != 0 ||
>>>>  	    n_rcu_torture_barrier_error != 0 ||
>>>>  	    n_rcu_torture_boost_ktrerror != 0 ||
>>>>  	    n_rcu_torture_boost_rterror != 0 ||
>>>>  	    n_rcu_torture_boost_failure != 0 ||
>>>>  	    i > 1) {
>>>> -		cnt += sprintf(&page[cnt], "!!! ");
>>>> +		page += sprintf(page, "!!! ");
>>>>  		atomic_inc(&n_rcu_torture_error);
>>>>  		WARN_ON_ONCE(1);
>>>>  	}
>>>> -	cnt += sprintf(&page[cnt], "Reader Pipe: ");
>>>> +	page += sprintf(page, "Reader Pipe: ");
>>>>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
>>>> -		cnt += sprintf(&page[cnt], " %ld", pipesummary[i]);
>>>> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
>>>> -	cnt += sprintf(&page[cnt], "Reader Batch: ");
>>>> +		page += sprintf(page, " %ld", pipesummary[i]);
>>>> +	page += sprintf(page, "\n%s%s ", torture_type, TORTURE_FLAG);
>>>> +	page += sprintf(page, "Reader Batch: ");
>>>>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
>>>> -		cnt += sprintf(&page[cnt], " %ld", batchsummary[i]);
>>>> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
>>>> -	cnt += sprintf(&page[cnt], "Free-Block Circulation: ");
>>>> +		page += sprintf(page, " %ld", batchsummary[i]);
>>>> +	page += sprintf(page, "\n%s%s ", torture_type, TORTURE_FLAG);
>>>> +	page += sprintf(page, "Free-Block Circulation: ");
>>>>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++) {
>>>> -		cnt += sprintf(&page[cnt], " %d",
>>>> +		page += sprintf(page, " %d",
>>>>  			       atomic_read(&rcu_torture_wcount[i]));
>>>>  	}
>>>> -	cnt += sprintf(&page[cnt], "\n");
>>>> +	page += sprintf(page, "\n");
>>>>  	if (cur_ops->stats)
>>>> -		cnt += cur_ops->stats(&page[cnt]);
>>>> -	return cnt;
>>>> +		 cur_ops->stats(page);
>>>
>>> Oh, sorry, this line has a waste white space.
>>>
>>>>  }
>>>>  
>>>>  /*
>>>> @@ -1134,10 +1128,17 @@ rcu_torture_printk(char *page)
>>>>  static void
>>>>  rcu_torture_stats_print(void)
>>>>  {
>>>> -	int cnt;
>>>> +	int size = (nr_cpu_ids + 2) * PAGE_SIZE; /* be sure of large enough */
>>>> +	char *buf;
>>>>  
>>>> -	cnt = rcu_torture_printk(printk_buf);
>>>> -	pr_alert("%s", printk_buf);
>>>> +	buf = kmalloc(size, GFP_KERNEL);
>>>> +	if (!buf) {
>>>> +		pr_err("no enough memory for printing, requre: %d", size);
>>>> +		return;
>>>> +	}
>>>> +	rcu_torture_printk(buf);
>>>> +	pr_alert("%s", buf);
>>>> +	kfree(buf);
>>>>  }
>>>>  
>>>>  /*
>>>>
>>>
>>>
>>
>>
> 
> 


-- 
Chen Gang

Re: [PATCH] kernel/rcutorture.c: be sure of enough memory for result printing.

[Paul E. McKenney]

On Mon, Oct 21, 2013 at 01:51:25PM +0800, Chen Gang wrote:
> If the contents is more than 4096 bytes (e.g. if have 1K cpus), current
> sprintf() will cause memory overflow. And this fix patch is to be sure
> of memory large enough.

Getting close, a few issues called out below.  Please resubmit with
these fixed.

> Benefit:
> 
>  - do not truncate printing contents.
>  - extensible, it is large enough for printing various related contents.
>  - simple and clear enough for both source code readers and writers.
> 
> Shortcoming:
> 
>  - It will waste some memory:
>     1 cpu may waste 24KB,
>     10 cpus may waste 96KB,
>     100 cpus may waste 816KB,
>     1K cpus may waste 8MB
>     ...
>    after finish printing, it will free the related memory, quickly.
>    it is a test module, so wast a little memory for extensible is OK.

Hmmm... 1K CPUs should be able to get by with about 200K rather than 8MB.
(Actually more like 60K, but allowing a little slop is not a bad thing.)

> Related  test (Fedora16 2 CPUs, 2GB RAM x86_64)
> 
>  - as module, with/without "torture_type=srcu".
>  - build-in not boot runnable, with/without "torture_type=srcu".
>  - build-in let boot runnable, with/without "torture_type=srcu".
> 
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  kernel/rcutorture.c |   67 ++++++++++++++++++++++++++-------------------------
>  1 files changed, 34 insertions(+), 33 deletions(-)
> 
> diff --git a/kernel/rcutorture.c b/kernel/rcutorture.c
> index be63101..3413bc1 100644
> --- a/kernel/rcutorture.c
> +++ b/kernel/rcutorture.c
> @@ -133,8 +133,6 @@ MODULE_PARM_DESC(verbose, "Enable verbose debugging printk()s");
>  #define VERBOSE_PRINTK_ERRSTRING(s) \
>  	do { if (verbose) pr_alert("%s" TORTURE_FLAG "!!! " s "\n", torture_type); } while (0)
> 
> -static char printk_buf[4096];
> -
>  static int nrealreaders;
>  static struct task_struct *writer_task;
>  static struct task_struct **fakewriter_tasks;
> @@ -370,7 +368,7 @@ struct rcu_torture_ops {
>  	void (*call)(struct rcu_head *head, void (*func)(struct rcu_head *rcu));
>  	void (*cb_barrier)(void);
>  	void (*fqs)(void);
> -	int (*stats)(char *page);
> +	void (*stats)(char *page);
>  	int irq_capable;
>  	int can_boost;
>  	const char *name;
> @@ -572,21 +570,19 @@ static void srcu_torture_barrier(void)
>  	srcu_barrier(&srcu_ctl);
>  }
> 
> -static int srcu_torture_stats(char *page)
> +static void srcu_torture_stats(char *page)
>  {
> -	int cnt = 0;
>  	int cpu;
>  	int idx = srcu_ctl.completed & 0x1;
> 
> -	cnt += sprintf(&page[cnt], "%s%s per-CPU(idx=%d):",
> +	page += sprintf(page, "%s%s per-CPU(idx=%d):",
>  		       torture_type, TORTURE_FLAG, idx);
>  	for_each_possible_cpu(cpu) {
> -		cnt += sprintf(&page[cnt], " %d(%lu,%lu)", cpu,
> +		page += sprintf(page, " %d(%lu,%lu)", cpu,

This format string has at most 54 characters, call it 100.

>  			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[!idx],
>  			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[idx]);
>  	}
> -	cnt += sprintf(&page[cnt], "\n");
> -	return cnt;
> +	sprintf(page, "\n");
>  }
> 
>  static void srcu_torture_synchronize_expedited(void)
> @@ -1046,10 +1042,9 @@ rcu_torture_reader(void *arg)
>  /*
>   * Create an RCU-torture statistics message in the specified buffer.
>   */
> -static int
> +static void
>  rcu_torture_printk(char *page)
>  {
> -	int cnt = 0;
>  	int cpu;
>  	int i;
>  	long pipesummary[RCU_TORTURE_PIPE_LEN + 1] = { 0 };
> @@ -1065,8 +1060,8 @@ rcu_torture_printk(char *page)
>  		if (pipesummary[i] != 0)
>  			break;
>  	}
> -	cnt += sprintf(&page[cnt], "%s%s ", torture_type, TORTURE_FLAG);
> -	cnt += sprintf(&page[cnt],
> +	page += sprintf(page, "%s%s ", torture_type, TORTURE_FLAG);
> +	page += sprintf(page,
>  		       "rtc: %p ver: %lu tfle: %d rta: %d rtaf: %d rtf: %d ",
>  		       rcu_torture_current,
>  		       rcu_torture_current_version,
> @@ -1074,53 +1069,52 @@ rcu_torture_printk(char *page)
>  		       atomic_read(&n_rcu_torture_alloc),
>  		       atomic_read(&n_rcu_torture_alloc_fail),
>  		       atomic_read(&n_rcu_torture_free));
> -	cnt += sprintf(&page[cnt], "rtmbe: %d rtbke: %ld rtbre: %ld ",
> +	page += sprintf(page, "rtmbe: %d rtbke: %ld rtbre: %ld ",
>  		       atomic_read(&n_rcu_torture_mberror),
>  		       n_rcu_torture_boost_ktrerror,
>  		       n_rcu_torture_boost_rterror);
> -	cnt += sprintf(&page[cnt], "rtbf: %ld rtb: %ld nt: %ld ",
> +	page += sprintf(page, "rtbf: %ld rtb: %ld nt: %ld ",
>  		       n_rcu_torture_boost_failure,
>  		       n_rcu_torture_boosts,
>  		       n_rcu_torture_timers);
> -	cnt += sprintf(&page[cnt],
> +	page += sprintf(page,
>  		       "onoff: %ld/%ld:%ld/%ld %d,%d:%d,%d %lu:%lu (HZ=%d) ",
>  		       n_online_successes, n_online_attempts,
>  		       n_offline_successes, n_offline_attempts,
>  		       min_online, max_online,
>  		       min_offline, max_offline,
>  		       sum_online, sum_offline, HZ);
> -	cnt += sprintf(&page[cnt], "barrier: %ld/%ld:%ld",
> +	page += sprintf(page, "barrier: %ld/%ld:%ld",
>  		       n_barrier_successes,
>  		       n_barrier_attempts,
>  		       n_rcu_torture_barrier_error);
> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
> +	page += sprintf(page, "\n%s%s ", torture_type, TORTURE_FLAG);
>  	if (atomic_read(&n_rcu_torture_mberror) != 0 ||
>  	    n_rcu_torture_barrier_error != 0 ||
>  	    n_rcu_torture_boost_ktrerror != 0 ||
>  	    n_rcu_torture_boost_rterror != 0 ||
>  	    n_rcu_torture_boost_failure != 0 ||
>  	    i > 1) {
> -		cnt += sprintf(&page[cnt], "!!! ");
> +		page += sprintf(page, "!!! ");
>  		atomic_inc(&n_rcu_torture_error);
>  		WARN_ON_ONCE(1);
>  	}
> -	cnt += sprintf(&page[cnt], "Reader Pipe: ");
> +	page += sprintf(page, "Reader Pipe: ");
>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
> -		cnt += sprintf(&page[cnt], " %ld", pipesummary[i]);
> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
> -	cnt += sprintf(&page[cnt], "Reader Batch: ");
> +		page += sprintf(page, " %ld", pipesummary[i]);
> +	page += sprintf(page, "\n%s%s ", torture_type, TORTURE_FLAG);
> +	page += sprintf(page, "Reader Batch: ");
>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
> -		cnt += sprintf(&page[cnt], " %ld", batchsummary[i]);
> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
> -	cnt += sprintf(&page[cnt], "Free-Block Circulation: ");
> +		page += sprintf(page, " %ld", batchsummary[i]);
> +	page += sprintf(page, "\n%s%s ", torture_type, TORTURE_FLAG);
> +	page += sprintf(page, "Free-Block Circulation: ");
>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++) {
> -		cnt += sprintf(&page[cnt], " %d",
> +		page += sprintf(page, " %d",
>  			       atomic_read(&rcu_torture_wcount[i]));
>  	}
> -	cnt += sprintf(&page[cnt], "\n");
> +	page += sprintf(page, "\n");
>  	if (cur_ops->stats)
> -		cnt += cur_ops->stats(&page[cnt]);
> -	return cnt;
> +		 cur_ops->stats(page);

Extraneous space here, please fix.  (As your later checkpatch noted.)

And this function can output a few K of characters.

>  }
> 
>  /*
> @@ -1134,10 +1128,17 @@ rcu_torture_printk(char *page)
>  static void
>  rcu_torture_stats_print(void)
>  {
> -	int cnt;
> +	int size = (nr_cpu_ids + 2) * PAGE_SIZE; /* be sure of large enough */

So 8K should cover the preamble, and about 54 per CPU for SRCU (which
we can round to 100 and then double to 200 for safety against changes),
so something like this should suffice:

	int size = nr_cpu_ids * 200 + 8192; /* be sure of large enough */

This allows about a factor of 4 slop, which is OK.  The original factor
of 80 was a bit excessive.

> +	char *buf;
> 
> -	cnt = rcu_torture_printk(printk_buf);
> -	pr_alert("%s", printk_buf);
> +	buf = kmalloc(size, GFP_KERNEL);
> +	if (!buf) {
> +		pr_err("no enough memory for printing, requre: %d", size);
> +		return;
> +	}
> +	rcu_torture_printk(buf);
> +	pr_alert("%s", buf);
> +	kfree(buf);
>  }
> 
>  /*
> -- 
> 1.7.7.6
> 

[PATCH v2] kernel/rcutorture.c: be sure of enough memory for result printing

[Chen Gang]

If the contents is more than 4096 bytes (e.g. if have 1K cpus), current
sprintf() will cause memory overflow. And this fix patch is to be sure
of memory large enough.

Benefit:

 - do not truncate printing contents.
 - extensible, it is large enough for printing various related contents.
 - simple and clear enough for both source code readers and writers.

Shortcoming:

 - It will waste some memory:

    1 cpu now comsumes 50 - 60 bytes, and this patch provides 200 bytes.
    global printing now comsumes a few KB, and this patch provide 8KB,
    so for 1K cpus, it may waste 100 - 200 KB memory.

   after finish printing, it will free the related memory, quickly.
   it is a test module, so wast a little memory for extensible is OK.

Related  test (Fedora16 2 CPUs, 2GB RAM x86_64)

 - as module, with/without "torture_type=srcu".
 - build-in not boot runnable, with/without "torture_type=srcu".
 - build-in let boot runnable, with/without "torture_type=srcu".


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 kernel/rcu/torture.c |   68 +++++++++++++++++++++++++------------------------
 1 files changed, 35 insertions(+), 33 deletions(-)

diff --git a/kernel/rcu/torture.c b/kernel/rcu/torture.c
index 3929cd4..a60874c 100644
--- a/kernel/rcu/torture.c
+++ b/kernel/rcu/torture.c
@@ -139,8 +139,6 @@ MODULE_PARM_DESC(verbose, "Enable verbose debugging printk()s");
 #define VERBOSE_PRINTK_ERRSTRING(s) \
 	do { if (verbose) pr_alert("%s" TORTURE_FLAG "!!! " s "\n", torture_type); } while (0)
 
-static char printk_buf[4096];
-
 static int nrealreaders;
 static struct task_struct *writer_task;
 static struct task_struct **fakewriter_tasks;
@@ -376,7 +374,7 @@ struct rcu_torture_ops {
 	void (*call)(struct rcu_head *head, void (*func)(struct rcu_head *rcu));
 	void (*cb_barrier)(void);
 	void (*fqs)(void);
-	int (*stats)(char *page);
+	void (*stats)(char *page);
 	int irq_capable;
 	int can_boost;
 	const char *name;
@@ -578,21 +576,19 @@ static void srcu_torture_barrier(void)
 	srcu_barrier(&srcu_ctl);
 }
 
-static int srcu_torture_stats(char *page)
+static void srcu_torture_stats(char *page)
 {
-	int cnt = 0;
 	int cpu;
 	int idx = srcu_ctl.completed & 0x1;
 
-	cnt += sprintf(&page[cnt], "%s%s per-CPU(idx=%d):",
+	page += sprintf(page, "%s%s per-CPU(idx=%d):",
 		       torture_type, TORTURE_FLAG, idx);
 	for_each_possible_cpu(cpu) {
-		cnt += sprintf(&page[cnt], " %d(%lu,%lu)", cpu,
+		page += sprintf(page, " %d(%lu,%lu)", cpu,
 			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[!idx],
 			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[idx]);
 	}
-	cnt += sprintf(&page[cnt], "\n");
-	return cnt;
+	sprintf(page, "\n");
 }
 
 static void srcu_torture_synchronize_expedited(void)
@@ -1052,10 +1048,9 @@ rcu_torture_reader(void *arg)
 /*
  * Create an RCU-torture statistics message in the specified buffer.
  */
-static int
+static void
 rcu_torture_printk(char *page)
 {
-	int cnt = 0;
 	int cpu;
 	int i;
 	long pipesummary[RCU_TORTURE_PIPE_LEN + 1] = { 0 };
@@ -1071,8 +1066,8 @@ rcu_torture_printk(char *page)
 		if (pipesummary[i] != 0)
 			break;
 	}
-	cnt += sprintf(&page[cnt], "%s%s ", torture_type, TORTURE_FLAG);
-	cnt += sprintf(&page[cnt],
+	page += sprintf(page, "%s%s ", torture_type, TORTURE_FLAG);
+	page += sprintf(page,
 		       "rtc: %p ver: %lu tfle: %d rta: %d rtaf: %d rtf: %d ",
 		       rcu_torture_current,
 		       rcu_torture_current_version,
@@ -1080,53 +1075,52 @@ rcu_torture_printk(char *page)
 		       atomic_read(&n_rcu_torture_alloc),
 		       atomic_read(&n_rcu_torture_alloc_fail),
 		       atomic_read(&n_rcu_torture_free));
-	cnt += sprintf(&page[cnt], "rtmbe: %d rtbke: %ld rtbre: %ld ",
+	page += sprintf(page, "rtmbe: %d rtbke: %ld rtbre: %ld ",
 		       atomic_read(&n_rcu_torture_mberror),
 		       n_rcu_torture_boost_ktrerror,
 		       n_rcu_torture_boost_rterror);
-	cnt += sprintf(&page[cnt], "rtbf: %ld rtb: %ld nt: %ld ",
+	page += sprintf(page, "rtbf: %ld rtb: %ld nt: %ld ",
 		       n_rcu_torture_boost_failure,
 		       n_rcu_torture_boosts,
 		       n_rcu_torture_timers);
-	cnt += sprintf(&page[cnt],
+	page += sprintf(page,
 		       "onoff: %ld/%ld:%ld/%ld %d,%d:%d,%d %lu:%lu (HZ=%d) ",
 		       n_online_successes, n_online_attempts,
 		       n_offline_successes, n_offline_attempts,
 		       min_online, max_online,
 		       min_offline, max_offline,
 		       sum_online, sum_offline, HZ);
-	cnt += sprintf(&page[cnt], "barrier: %ld/%ld:%ld",
+	page += sprintf(page, "barrier: %ld/%ld:%ld",
 		       n_barrier_successes,
 		       n_barrier_attempts,
 		       n_rcu_torture_barrier_error);
-	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
+	page += sprintf(page, "\n%s%s ", torture_type, TORTURE_FLAG);
 	if (atomic_read(&n_rcu_torture_mberror) != 0 ||
 	    n_rcu_torture_barrier_error != 0 ||
 	    n_rcu_torture_boost_ktrerror != 0 ||
 	    n_rcu_torture_boost_rterror != 0 ||
 	    n_rcu_torture_boost_failure != 0 ||
 	    i > 1) {
-		cnt += sprintf(&page[cnt], "!!! ");
+		page += sprintf(page, "!!! ");
 		atomic_inc(&n_rcu_torture_error);
 		WARN_ON_ONCE(1);
 	}
-	cnt += sprintf(&page[cnt], "Reader Pipe: ");
+	page += sprintf(page, "Reader Pipe: ");
 	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
-		cnt += sprintf(&page[cnt], " %ld", pipesummary[i]);
-	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
-	cnt += sprintf(&page[cnt], "Reader Batch: ");
+		page += sprintf(page, " %ld", pipesummary[i]);
+	page += sprintf(page, "\n%s%s ", torture_type, TORTURE_FLAG);
+	page += sprintf(page, "Reader Batch: ");
 	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
-		cnt += sprintf(&page[cnt], " %ld", batchsummary[i]);
-	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
-	cnt += sprintf(&page[cnt], "Free-Block Circulation: ");
+		page += sprintf(page, " %ld", batchsummary[i]);
+	page += sprintf(page, "\n%s%s ", torture_type, TORTURE_FLAG);
+	page += sprintf(page, "Free-Block Circulation: ");
 	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++) {
-		cnt += sprintf(&page[cnt], " %d",
+		page += sprintf(page, " %d",
 			       atomic_read(&rcu_torture_wcount[i]));
 	}
-	cnt += sprintf(&page[cnt], "\n");
+	page += sprintf(page, "\n");
 	if (cur_ops->stats)
-		cnt += cur_ops->stats(&page[cnt]);
-	return cnt;
+		cur_ops->stats(page);
 }
 
 /*
@@ -1140,10 +1134,18 @@ rcu_torture_printk(char *page)
 static void
 rcu_torture_stats_print(void)
 {
-	int cnt;
+	int size = nr_cpu_ids * 200 + 8192; /* be sure of large enough */
+	char *buf;
 
-	cnt = rcu_torture_printk(printk_buf);
-	pr_alert("%s", printk_buf);
+	buf = kmalloc(size, GFP_KERNEL);
+	if (!buf) {
+		pr_err("rcu-torture: no enough memory for printing, requre: %d",
+			size);
+		return;
+	}
+	rcu_torture_printk(buf);
+	pr_alert("%s", buf);
+	kfree(buf);
 }
 
 /*
-- 
1.7.7.6

Re: [PATCH v2] kernel/rcutorture.c: be sure of enough memory for result printing

[Paul E. McKenney]

On Thu, Nov 07, 2013 at 10:30:25AM +0800, Chen Gang wrote:
> If the contents is more than 4096 bytes (e.g. if have 1K cpus), current
> sprintf() will cause memory overflow. And this fix patch is to be sure
> of memory large enough.
> 
> Benefit:
> 
>  - do not truncate printing contents.
>  - extensible, it is large enough for printing various related contents.
>  - simple and clear enough for both source code readers and writers.
> 
> Shortcoming:
> 
>  - It will waste some memory:
> 
>     1 cpu now comsumes 50 - 60 bytes, and this patch provides 200 bytes.
>     global printing now comsumes a few KB, and this patch provide 8KB,
>     so for 1K cpus, it may waste 100 - 200 KB memory.
> 
>    after finish printing, it will free the related memory, quickly.
>    it is a test module, so wast a little memory for extensible is OK.
> 
> Related  test (Fedora16 2 CPUs, 2GB RAM x86_64)
> 
>  - as module, with/without "torture_type=srcu".
>  - build-in not boot runnable, with/without "torture_type=srcu".
>  - build-in let boot runnable, with/without "torture_type=srcu".
> 
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>

Thank you, queued for 3.14.

							Thanx, Paul

> ---
>  kernel/rcu/torture.c |   68 +++++++++++++++++++++++++------------------------
>  1 files changed, 35 insertions(+), 33 deletions(-)
> 
> diff --git a/kernel/rcu/torture.c b/kernel/rcu/torture.c
> index 3929cd4..a60874c 100644
> --- a/kernel/rcu/torture.c
> +++ b/kernel/rcu/torture.c
> @@ -139,8 +139,6 @@ MODULE_PARM_DESC(verbose, "Enable verbose debugging printk()s");
>  #define VERBOSE_PRINTK_ERRSTRING(s) \
>  	do { if (verbose) pr_alert("%s" TORTURE_FLAG "!!! " s "\n", torture_type); } while (0)
> 
> -static char printk_buf[4096];
> -
>  static int nrealreaders;
>  static struct task_struct *writer_task;
>  static struct task_struct **fakewriter_tasks;
> @@ -376,7 +374,7 @@ struct rcu_torture_ops {
>  	void (*call)(struct rcu_head *head, void (*func)(struct rcu_head *rcu));
>  	void (*cb_barrier)(void);
>  	void (*fqs)(void);
> -	int (*stats)(char *page);
> +	void (*stats)(char *page);
>  	int irq_capable;
>  	int can_boost;
>  	const char *name;
> @@ -578,21 +576,19 @@ static void srcu_torture_barrier(void)
>  	srcu_barrier(&srcu_ctl);
>  }
> 
> -static int srcu_torture_stats(char *page)
> +static void srcu_torture_stats(char *page)
>  {
> -	int cnt = 0;
>  	int cpu;
>  	int idx = srcu_ctl.completed & 0x1;
> 
> -	cnt += sprintf(&page[cnt], "%s%s per-CPU(idx=%d):",
> +	page += sprintf(page, "%s%s per-CPU(idx=%d):",
>  		       torture_type, TORTURE_FLAG, idx);
>  	for_each_possible_cpu(cpu) {
> -		cnt += sprintf(&page[cnt], " %d(%lu,%lu)", cpu,
> +		page += sprintf(page, " %d(%lu,%lu)", cpu,
>  			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[!idx],
>  			       per_cpu_ptr(srcu_ctl.per_cpu_ref, cpu)->c[idx]);
>  	}
> -	cnt += sprintf(&page[cnt], "\n");
> -	return cnt;
> +	sprintf(page, "\n");
>  }
> 
>  static void srcu_torture_synchronize_expedited(void)
> @@ -1052,10 +1048,9 @@ rcu_torture_reader(void *arg)
>  /*
>   * Create an RCU-torture statistics message in the specified buffer.
>   */
> -static int
> +static void
>  rcu_torture_printk(char *page)
>  {
> -	int cnt = 0;
>  	int cpu;
>  	int i;
>  	long pipesummary[RCU_TORTURE_PIPE_LEN + 1] = { 0 };
> @@ -1071,8 +1066,8 @@ rcu_torture_printk(char *page)
>  		if (pipesummary[i] != 0)
>  			break;
>  	}
> -	cnt += sprintf(&page[cnt], "%s%s ", torture_type, TORTURE_FLAG);
> -	cnt += sprintf(&page[cnt],
> +	page += sprintf(page, "%s%s ", torture_type, TORTURE_FLAG);
> +	page += sprintf(page,
>  		       "rtc: %p ver: %lu tfle: %d rta: %d rtaf: %d rtf: %d ",
>  		       rcu_torture_current,
>  		       rcu_torture_current_version,
> @@ -1080,53 +1075,52 @@ rcu_torture_printk(char *page)
>  		       atomic_read(&n_rcu_torture_alloc),
>  		       atomic_read(&n_rcu_torture_alloc_fail),
>  		       atomic_read(&n_rcu_torture_free));
> -	cnt += sprintf(&page[cnt], "rtmbe: %d rtbke: %ld rtbre: %ld ",
> +	page += sprintf(page, "rtmbe: %d rtbke: %ld rtbre: %ld ",
>  		       atomic_read(&n_rcu_torture_mberror),
>  		       n_rcu_torture_boost_ktrerror,
>  		       n_rcu_torture_boost_rterror);
> -	cnt += sprintf(&page[cnt], "rtbf: %ld rtb: %ld nt: %ld ",
> +	page += sprintf(page, "rtbf: %ld rtb: %ld nt: %ld ",
>  		       n_rcu_torture_boost_failure,
>  		       n_rcu_torture_boosts,
>  		       n_rcu_torture_timers);
> -	cnt += sprintf(&page[cnt],
> +	page += sprintf(page,
>  		       "onoff: %ld/%ld:%ld/%ld %d,%d:%d,%d %lu:%lu (HZ=%d) ",
>  		       n_online_successes, n_online_attempts,
>  		       n_offline_successes, n_offline_attempts,
>  		       min_online, max_online,
>  		       min_offline, max_offline,
>  		       sum_online, sum_offline, HZ);
> -	cnt += sprintf(&page[cnt], "barrier: %ld/%ld:%ld",
> +	page += sprintf(page, "barrier: %ld/%ld:%ld",
>  		       n_barrier_successes,
>  		       n_barrier_attempts,
>  		       n_rcu_torture_barrier_error);
> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
> +	page += sprintf(page, "\n%s%s ", torture_type, TORTURE_FLAG);
>  	if (atomic_read(&n_rcu_torture_mberror) != 0 ||
>  	    n_rcu_torture_barrier_error != 0 ||
>  	    n_rcu_torture_boost_ktrerror != 0 ||
>  	    n_rcu_torture_boost_rterror != 0 ||
>  	    n_rcu_torture_boost_failure != 0 ||
>  	    i > 1) {
> -		cnt += sprintf(&page[cnt], "!!! ");
> +		page += sprintf(page, "!!! ");
>  		atomic_inc(&n_rcu_torture_error);
>  		WARN_ON_ONCE(1);
>  	}
> -	cnt += sprintf(&page[cnt], "Reader Pipe: ");
> +	page += sprintf(page, "Reader Pipe: ");
>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
> -		cnt += sprintf(&page[cnt], " %ld", pipesummary[i]);
> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
> -	cnt += sprintf(&page[cnt], "Reader Batch: ");
> +		page += sprintf(page, " %ld", pipesummary[i]);
> +	page += sprintf(page, "\n%s%s ", torture_type, TORTURE_FLAG);
> +	page += sprintf(page, "Reader Batch: ");
>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++)
> -		cnt += sprintf(&page[cnt], " %ld", batchsummary[i]);
> -	cnt += sprintf(&page[cnt], "\n%s%s ", torture_type, TORTURE_FLAG);
> -	cnt += sprintf(&page[cnt], "Free-Block Circulation: ");
> +		page += sprintf(page, " %ld", batchsummary[i]);
> +	page += sprintf(page, "\n%s%s ", torture_type, TORTURE_FLAG);
> +	page += sprintf(page, "Free-Block Circulation: ");
>  	for (i = 0; i < RCU_TORTURE_PIPE_LEN + 1; i++) {
> -		cnt += sprintf(&page[cnt], " %d",
> +		page += sprintf(page, " %d",
>  			       atomic_read(&rcu_torture_wcount[i]));
>  	}
> -	cnt += sprintf(&page[cnt], "\n");
> +	page += sprintf(page, "\n");
>  	if (cur_ops->stats)
> -		cnt += cur_ops->stats(&page[cnt]);
> -	return cnt;
> +		cur_ops->stats(page);
>  }
> 
>  /*
> @@ -1140,10 +1134,18 @@ rcu_torture_printk(char *page)
>  static void
>  rcu_torture_stats_print(void)
>  {
> -	int cnt;
> +	int size = nr_cpu_ids * 200 + 8192; /* be sure of large enough */
> +	char *buf;
> 
> -	cnt = rcu_torture_printk(printk_buf);
> -	pr_alert("%s", printk_buf);
> +	buf = kmalloc(size, GFP_KERNEL);
> +	if (!buf) {
> +		pr_err("rcu-torture: no enough memory for printing, requre: %d",
> +			size);
> +		return;
> +	}
> +	rcu_torture_printk(buf);
> +	pr_alert("%s", buf);
> +	kfree(buf);
>  }
> 
>  /*
> -- 
> 1.7.7.6
> 

Re: [PATCH v2] kernel/rcutorture.c: be sure of enough memory for result printing

[Chen Gang]

On 11/08/2013 04:59 AM, Paul E. McKenney wrote:
> On Thu, Nov 07, 2013 at 10:30:25AM +0800, Chen Gang wrote:
>> > If the contents is more than 4096 bytes (e.g. if have 1K cpus), current
>> > sprintf() will cause memory overflow. And this fix patch is to be sure
>> > of memory large enough.
>> > 
>> > Benefit:
>> > 
>> >  - do not truncate printing contents.
>> >  - extensible, it is large enough for printing various related contents.
>> >  - simple and clear enough for both source code readers and writers.
>> > 
>> > Shortcoming:
>> > 
>> >  - It will waste some memory:
>> > 
>> >     1 cpu now comsumes 50 - 60 bytes, and this patch provides 200 bytes.
>> >     global printing now comsumes a few KB, and this patch provide 8KB,
>> >     so for 1K cpus, it may waste 100 - 200 KB memory.
>> > 
>> >    after finish printing, it will free the related memory, quickly.
>> >    it is a test module, so wast a little memory for extensible is OK.
>> > 
>> > Related  test (Fedora16 2 CPUs, 2GB RAM x86_64)
>> > 
>> >  - as module, with/without "torture_type=srcu".
>> >  - build-in not boot runnable, with/without "torture_type=srcu".
>> >  - build-in let boot runnable, with/without "torture_type=srcu".
>> > 
>> > 
>> > Signed-off-by: Chen Gang <gang.chen@asianux.com>
> Thank you, queued for 3.14.

Thank you too.

-- 
Chen Gang