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

[Chen Gang <gang.chen@asianux.com>]

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