[PATCH 1/2] refscale: simplify the errexit checkpoint

[PATCH 1/2] refscale: simplify the errexit checkpoint

[Li Zhijian]

Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
---
 kernel/rcu/refscale.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/kernel/rcu/refscale.c b/kernel/rcu/refscale.c
index 66dc14cf5687..2cbe2a2ba387 100644
--- a/kernel/rcu/refscale.c
+++ b/kernel/rcu/refscale.c
@@ -698,26 +698,25 @@ static int main_func(void *arg)
 	// Print the average of all experiments
 	SCALEOUT("END OF TEST. Calculating average duration per loop (nanoseconds)...\n");
 
-	if (!errexit) {
-		buf[0] = 0;
-		strcat(buf, "\n");
-		strcat(buf, "Runs\tTime(ns)\n");
-	}
+	if (errexit)
+		goto err;
+
+	buf[0] = 0;
+	strcat(buf, "\n");
+	strcat(buf, "Runs\tTime(ns)\n");
 
 	for (exp = 0; exp < nruns; exp++) {
 		u64 avg;
 		u32 rem;
 
-		if (errexit)
-			break;
 		avg = div_u64_rem(result_avg[exp], 1000, &rem);
 		sprintf(buf1, "%d\t%llu.%03u\n", exp + 1, avg, rem);
 		strcat(buf, buf1);
 	}
 
-	if (!errexit)
-		SCALEOUT("%s", buf);
+	SCALEOUT("%s", buf);
 
+err:
 	// This will shutdown everything including us.
 	if (shutdown) {
 		shutdown_start = 1;
-- 
2.33.0

[PATCH 2/2] refscale: prevent buffer to pr_alert() being too long

[Li Zhijian]

0Day/LKP observed that the refscale results become incompleted
when a larger nruns(such as 300) is specified.
It seems that printk() can accept < 1024 buffer at once.
Print the buffer if its length exceeds 800 simply.

CC: Philip Li <philip.li@intel.com>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
---
 kernel/rcu/refscale.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/kernel/rcu/refscale.c b/kernel/rcu/refscale.c
index 2cbe2a2ba387..b1b9052010fd 100644
--- a/kernel/rcu/refscale.c
+++ b/kernel/rcu/refscale.c
@@ -604,7 +604,7 @@ static u64 process_durations(int n)
 	char *buf;
 	u64 sum = 0;
 
-	buf = kmalloc(128 + nreaders * 32, GFP_KERNEL);
+	buf = kmalloc(64 * 20, GFP_KERNEL);
 	if (!buf)
 		return 0;
 	buf[0] = 0;
@@ -617,13 +617,15 @@ static u64 process_durations(int n)
 
 		if (i % 5 == 0)
 			strcat(buf, "\n");
+		if (strlen(buf) > 800) {
+			pr_alert("%s", buf);
+			buf[0] = 0;
+		}
 		strcat(buf, buf1);
 
 		sum += rt->last_duration_ns;
 	}
-	strcat(buf, "\n");
-
-	SCALEOUT("%s\n", buf);
+	pr_alert("%s\n", buf);
 
 	kfree(buf);
 	return sum;
@@ -648,7 +650,7 @@ static int main_func(void *arg)
 
 	VERBOSE_SCALEOUT("main_func task started");
 	result_avg = kzalloc(nruns * sizeof(*result_avg), GFP_KERNEL);
-	buf = kzalloc(64 + nruns * 32, GFP_KERNEL);
+	buf = kzalloc(64 * 20, GFP_KERNEL);
 	if (!result_avg || !buf) {
 		VERBOSE_SCALEOUT_ERRSTRING("out of memory");
 		errexit = true;
@@ -701,10 +703,7 @@ static int main_func(void *arg)
 	if (errexit)
 		goto err;
 
-	buf[0] = 0;
-	strcat(buf, "\n");
-	strcat(buf, "Runs\tTime(ns)\n");
-
+	pr_alert("Runs\tTime(ns)\n");
 	for (exp = 0; exp < nruns; exp++) {
 		u64 avg;
 		u32 rem;
@@ -712,9 +711,13 @@ static int main_func(void *arg)
 		avg = div_u64_rem(result_avg[exp], 1000, &rem);
 		sprintf(buf1, "%d\t%llu.%03u\n", exp + 1, avg, rem);
 		strcat(buf, buf1);
+		if (strlen(buf) > 800) {
+			pr_alert("%s", buf);
+			buf[0] = 0;
+		}
 	}
 
-	SCALEOUT("%s", buf);
+	pr_alert("%s", buf);
 
 err:
 	// This will shutdown everything including us.
-- 
2.33.0

Re: [PATCH 1/2] refscale: simplify the errexit checkpoint

[Paul E. McKenney]

On Fri, Oct 22, 2021 at 06:51:10PM +0800, Li Zhijian wrote:
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>

Good catch!

But given that errexit is only set once at the beginning, why not
eliminate this local variable in favor of a goto at the point that it
is currently assigned to?  That would permit further simplification.

							Thanx, Paul

> ---
>  kernel/rcu/refscale.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/rcu/refscale.c b/kernel/rcu/refscale.c
> index 66dc14cf5687..2cbe2a2ba387 100644
> --- a/kernel/rcu/refscale.c
> +++ b/kernel/rcu/refscale.c
> @@ -698,26 +698,25 @@ static int main_func(void *arg)
>  	// Print the average of all experiments
>  	SCALEOUT("END OF TEST. Calculating average duration per loop (nanoseconds)...\n");
>  
> -	if (!errexit) {
> -		buf[0] = 0;
> -		strcat(buf, "\n");
> -		strcat(buf, "Runs\tTime(ns)\n");
> -	}
> +	if (errexit)
> +		goto err;
> +
> +	buf[0] = 0;
> +	strcat(buf, "\n");
> +	strcat(buf, "Runs\tTime(ns)\n");
>  
>  	for (exp = 0; exp < nruns; exp++) {
>  		u64 avg;
>  		u32 rem;
>  
> -		if (errexit)
> -			break;
>  		avg = div_u64_rem(result_avg[exp], 1000, &rem);
>  		sprintf(buf1, "%d\t%llu.%03u\n", exp + 1, avg, rem);
>  		strcat(buf, buf1);
>  	}
>  
> -	if (!errexit)
> -		SCALEOUT("%s", buf);
> +	SCALEOUT("%s", buf);
>  
> +err:
>  	// This will shutdown everything including us.
>  	if (shutdown) {
>  		shutdown_start = 1;
> -- 
> 2.33.0
> 
> 
> 

Re: [PATCH 2/2] refscale: prevent buffer to pr_alert() being too long

[Paul E. McKenney]

On Fri, Oct 22, 2021 at 06:51:11PM +0800, Li Zhijian wrote:
> 0Day/LKP observed that the refscale results become incompleted
> when a larger nruns(such as 300) is specified.
> It seems that printk() can accept < 1024 buffer at once.
> Print the buffer if its length exceeds 800 simply.
> 
> CC: Philip Li <philip.li@intel.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>

Good catch!  A couple of questions below.

						Thanx, Paul

> ---
>  kernel/rcu/refscale.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/rcu/refscale.c b/kernel/rcu/refscale.c
> index 2cbe2a2ba387..b1b9052010fd 100644
> --- a/kernel/rcu/refscale.c
> +++ b/kernel/rcu/refscale.c
> @@ -604,7 +604,7 @@ static u64 process_durations(int n)
>  	char *buf;
>  	u64 sum = 0;
>  
> -	buf = kmalloc(128 + nreaders * 32, GFP_KERNEL);
> +	buf = kmalloc(64 * 20, GFP_KERNEL);

This allocation (and the one below) is 1280 bytes rather than
1024 bytes.  Why the extra couple hundred bytes?

>  	if (!buf)
>  		return 0;
>  	buf[0] = 0;
> @@ -617,13 +617,15 @@ static u64 process_durations(int n)
>  
>  		if (i % 5 == 0)
>  			strcat(buf, "\n");
> +		if (strlen(buf) > 800) {
> +			pr_alert("%s", buf);

Does the tools/testing/selftests/rcutorture/bin/kvm-recheck-refscale.sh
script also require changes to handle the partial lines?

Same for the later comparison against 800.

> +			buf[0] = 0;
> +		}
>  		strcat(buf, buf1);
>  
>  		sum += rt->last_duration_ns;
>  	}
> -	strcat(buf, "\n");
> -
> -	SCALEOUT("%s\n", buf);
> +	pr_alert("%s\n", buf);
>  
>  	kfree(buf);
>  	return sum;
> @@ -648,7 +650,7 @@ static int main_func(void *arg)
>  
>  	VERBOSE_SCALEOUT("main_func task started");
>  	result_avg = kzalloc(nruns * sizeof(*result_avg), GFP_KERNEL);
> -	buf = kzalloc(64 + nruns * 32, GFP_KERNEL);
> +	buf = kzalloc(64 * 20, GFP_KERNEL);
>  	if (!result_avg || !buf) {
>  		VERBOSE_SCALEOUT_ERRSTRING("out of memory");
>  		errexit = true;
> @@ -701,10 +703,7 @@ static int main_func(void *arg)
>  	if (errexit)
>  		goto err;
>  
> -	buf[0] = 0;
> -	strcat(buf, "\n");
> -	strcat(buf, "Runs\tTime(ns)\n");
> -
> +	pr_alert("Runs\tTime(ns)\n");
>  	for (exp = 0; exp < nruns; exp++) {
>  		u64 avg;
>  		u32 rem;
> @@ -712,9 +711,13 @@ static int main_func(void *arg)
>  		avg = div_u64_rem(result_avg[exp], 1000, &rem);
>  		sprintf(buf1, "%d\t%llu.%03u\n", exp + 1, avg, rem);
>  		strcat(buf, buf1);
> +		if (strlen(buf) > 800) {
> +			pr_alert("%s", buf);
> +			buf[0] = 0;
> +		}
>  	}
>  
> -	SCALEOUT("%s", buf);
> +	pr_alert("%s", buf);
>  
>  err:
>  	// This will shutdown everything including us.
> -- 
> 2.33.0
> 
> 
> 

Re: [PATCH 1/2] refscale: simplify the errexit checkpoint

[Li, Zhijian]

on 2021/10/23 7:10, Paul E. McKenney wrote:
> On Fri, Oct 22, 2021 at 06:51:10PM +0800, Li Zhijian wrote:
>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> Good catch!
>
> But given that errexit is only set once at the beginning, why not
> eliminate this local variable in favor of a goto at the point that it
> is currently assigned to?  That would permit further simplification.

It sounds pretty good, i will update it later.

Thanks
Zhijian


> 							Thanx, Paul
>
>> ---
>>   kernel/rcu/refscale.c | 17 ++++++++---------
>>   1 file changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/kernel/rcu/refscale.c b/kernel/rcu/refscale.c
>> index 66dc14cf5687..2cbe2a2ba387 100644
>> --- a/kernel/rcu/refscale.c
>> +++ b/kernel/rcu/refscale.c
>> @@ -698,26 +698,25 @@ static int main_func(void *arg)
>>   	// Print the average of all experiments
>>   	SCALEOUT("END OF TEST. Calculating average duration per loop (nanoseconds)...\n");
>>   
>> -	if (!errexit) {
>> -		buf[0] = 0;
>> -		strcat(buf, "\n");
>> -		strcat(buf, "Runs\tTime(ns)\n");
>> -	}
>> +	if (errexit)
>> +		goto err;
>> +
>> +	buf[0] = 0;
>> +	strcat(buf, "\n");
>> +	strcat(buf, "Runs\tTime(ns)\n");
>>   
>>   	for (exp = 0; exp < nruns; exp++) {
>>   		u64 avg;
>>   		u32 rem;
>>   
>> -		if (errexit)
>> -			break;
>>   		avg = div_u64_rem(result_avg[exp], 1000, &rem);
>>   		sprintf(buf1, "%d\t%llu.%03u\n", exp + 1, avg, rem);
>>   		strcat(buf, buf1);
>>   	}
>>   
>> -	if (!errexit)
>> -		SCALEOUT("%s", buf);
>> +	SCALEOUT("%s", buf);
>>   
>> +err:
>>   	// This will shutdown everything including us.
>>   	if (shutdown) {
>>   		shutdown_start = 1;
>> -- 
>> 2.33.0
>>
>>
>>
>

Re: [PATCH 2/2] refscale: prevent buffer to pr_alert() being too long

[Li, Zhijian]

on 2021/10/23 7:15, Paul E. McKenney wrote:
> On Fri, Oct 22, 2021 at 06:51:11PM +0800, Li Zhijian wrote:
>> 0Day/LKP observed that the refscale results become incompleted
>> when a larger nruns(such as 300) is specified.
>> It seems that printk() can accept < 1024 buffer at once.
>> Print the buffer if its length exceeds 800 simply.
>>
>> CC: Philip Li <philip.li@intel.com>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> Good catch!  A couple of questions below.
>
> 						Thanx, Paul
>
>> ---
>>   kernel/rcu/refscale.c | 23 +++++++++++++----------
>>   1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/kernel/rcu/refscale.c b/kernel/rcu/refscale.c
>> index 2cbe2a2ba387..b1b9052010fd 100644
>> --- a/kernel/rcu/refscale.c
>> +++ b/kernel/rcu/refscale.c
>> @@ -604,7 +604,7 @@ static u64 process_durations(int n)
>>   	char *buf;
>>   	u64 sum = 0;
>>   
>> -	buf = kmalloc(128 + nreaders * 32, GFP_KERNEL);
>> +	buf = kmalloc(64 * 20, GFP_KERNEL);
> This allocation (and the one below) is 1280 bytes rather than
> 1024 bytes.  Why the extra couple hundred bytes?

Nothing special, so let's change 1024 or (800 + 64 ),which is sufficent as well ?


>
>>   	if (!buf)
>>   		return 0;
>>   	buf[0] = 0;
>> @@ -617,13 +617,15 @@ static u64 process_durations(int n)
>>   
>>   		if (i % 5 == 0)
>>   			strcat(buf, "\n");
>> +		if (strlen(buf) > 800) {
>> +			pr_alert("%s", buf);
> Does the tools/testing/selftests/rcutorture/bin/kvm-recheck-refscale.sh
> script also require changes to handle the partial lines?

I missed that, i will take a look at it

Thanks

Zhijian


>
> Same for the later comparison against 800.
>
>> +			buf[0] = 0;
>> +		}
>>   		strcat(buf, buf1);
>>   
>>   		sum += rt->last_duration_ns;
>>   	}
>> -	strcat(buf, "\n");
>> -
>> -	SCALEOUT("%s\n", buf);
>> +	pr_alert("%s\n", buf);
>>   
>>   	kfree(buf);
>>   	return sum;
>> @@ -648,7 +650,7 @@ static int main_func(void *arg)
>>   
>>   	VERBOSE_SCALEOUT("main_func task started");
>>   	result_avg = kzalloc(nruns * sizeof(*result_avg), GFP_KERNEL);
>> -	buf = kzalloc(64 + nruns * 32, GFP_KERNEL);
>> +	buf = kzalloc(64 * 20, GFP_KERNEL);
>>   	if (!result_avg || !buf) {
>>   		VERBOSE_SCALEOUT_ERRSTRING("out of memory");
>>   		errexit = true;
>> @@ -701,10 +703,7 @@ static int main_func(void *arg)
>>   	if (errexit)
>>   		goto err;
>>   
>> -	buf[0] = 0;
>> -	strcat(buf, "\n");
>> -	strcat(buf, "Runs\tTime(ns)\n");
>> -
>> +	pr_alert("Runs\tTime(ns)\n");
>>   	for (exp = 0; exp < nruns; exp++) {
>>   		u64 avg;
>>   		u32 rem;
>> @@ -712,9 +711,13 @@ static int main_func(void *arg)
>>   		avg = div_u64_rem(result_avg[exp], 1000, &rem);
>>   		sprintf(buf1, "%d\t%llu.%03u\n", exp + 1, avg, rem);
>>   		strcat(buf, buf1);
>> +		if (strlen(buf) > 800) {
>> +			pr_alert("%s", buf);
>> +			buf[0] = 0;
>> +		}
>>   	}
>>   
>> -	SCALEOUT("%s", buf);
>> +	pr_alert("%s", buf);
>>   
>>   err:
>>   	// This will shutdown everything including us.
>> -- 
>> 2.33.0
>>
>>
>>
>

Re: [PATCH 2/2] refscale: prevent buffer to pr_alert() being too long

[Paul E. McKenney]

On Sun, Oct 24, 2021 at 10:46:16PM +0800, Li, Zhijian wrote:
> 
> on 2021/10/23 7:15, Paul E. McKenney wrote:
> > On Fri, Oct 22, 2021 at 06:51:11PM +0800, Li Zhijian wrote:
> > > 0Day/LKP observed that the refscale results become incompleted
> > > when a larger nruns(such as 300) is specified.
> > > It seems that printk() can accept < 1024 buffer at once.
> > > Print the buffer if its length exceeds 800 simply.
> > > 
> > > CC: Philip Li <philip.li@intel.com>
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> > Good catch!  A couple of questions below.
> > 
> > 						Thanx, Paul
> > 
> > > ---
> > >   kernel/rcu/refscale.c | 23 +++++++++++++----------
> > >   1 file changed, 13 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/kernel/rcu/refscale.c b/kernel/rcu/refscale.c
> > > index 2cbe2a2ba387..b1b9052010fd 100644
> > > --- a/kernel/rcu/refscale.c
> > > +++ b/kernel/rcu/refscale.c
> > > @@ -604,7 +604,7 @@ static u64 process_durations(int n)
> > >   	char *buf;
> > >   	u64 sum = 0;
> > > -	buf = kmalloc(128 + nreaders * 32, GFP_KERNEL);
> > > +	buf = kmalloc(64 * 20, GFP_KERNEL);
> > This allocation (and the one below) is 1280 bytes rather than
> > 1024 bytes.  Why the extra couple hundred bytes?
> 
> Nothing special, so let's change 1024 or (800 + 64 ),which is sufficent as well ?

Either works.  No problem having a few extra bytes, but 200 seemed a bit
excessive.  ;-)

> > >   	if (!buf)
> > >   		return 0;
> > >   	buf[0] = 0;
> > > @@ -617,13 +617,15 @@ static u64 process_durations(int n)
> > >   		if (i % 5 == 0)
> > >   			strcat(buf, "\n");
> > > +		if (strlen(buf) > 800) {
> > > +			pr_alert("%s", buf);
> > Does the tools/testing/selftests/rcutorture/bin/kvm-recheck-refscale.sh
> > script also require changes to handle the partial lines?
> 
> I missed that, i will take a look at it

Looking forward to seeing what you come up with.

							Thanx, Paul

> Thanks
> 
> Zhijian
> 
> 
> > 
> > Same for the later comparison against 800.
> > 
> > > +			buf[0] = 0;
> > > +		}
> > >   		strcat(buf, buf1);
> > >   		sum += rt->last_duration_ns;
> > >   	}
> > > -	strcat(buf, "\n");
> > > -
> > > -	SCALEOUT("%s\n", buf);
> > > +	pr_alert("%s\n", buf);
> > >   	kfree(buf);
> > >   	return sum;
> > > @@ -648,7 +650,7 @@ static int main_func(void *arg)
> > >   	VERBOSE_SCALEOUT("main_func task started");
> > >   	result_avg = kzalloc(nruns * sizeof(*result_avg), GFP_KERNEL);
> > > -	buf = kzalloc(64 + nruns * 32, GFP_KERNEL);
> > > +	buf = kzalloc(64 * 20, GFP_KERNEL);
> > >   	if (!result_avg || !buf) {
> > >   		VERBOSE_SCALEOUT_ERRSTRING("out of memory");
> > >   		errexit = true;
> > > @@ -701,10 +703,7 @@ static int main_func(void *arg)
> > >   	if (errexit)
> > >   		goto err;
> > > -	buf[0] = 0;
> > > -	strcat(buf, "\n");
> > > -	strcat(buf, "Runs\tTime(ns)\n");
> > > -
> > > +	pr_alert("Runs\tTime(ns)\n");
> > >   	for (exp = 0; exp < nruns; exp++) {
> > >   		u64 avg;
> > >   		u32 rem;
> > > @@ -712,9 +711,13 @@ static int main_func(void *arg)
> > >   		avg = div_u64_rem(result_avg[exp], 1000, &rem);
> > >   		sprintf(buf1, "%d\t%llu.%03u\n", exp + 1, avg, rem);
> > >   		strcat(buf, buf1);
> > > +		if (strlen(buf) > 800) {
> > > +			pr_alert("%s", buf);
> > > +			buf[0] = 0;
> > > +		}
> > >   	}
> > > -	SCALEOUT("%s", buf);
> > > +	pr_alert("%s", buf);
> > >   err:
> > >   	// This will shutdown everything including us.
> > > -- 
> > > 2.33.0
> > > 
> > > 
> > > 
> > 
> 
> 

Re: [PATCH 2/2] refscale: prevent buffer to pr_alert() being too long

[lizhijian]

On 25/10/2021 06:56, Paul E. McKenney wrote:
>
>>>>    	if (!buf)
>>>>    		return 0;
>>>>    	buf[0] = 0;
>>>> @@ -617,13 +617,15 @@ static u64 process_durations(int n)
>>>>    		if (i % 5 == 0)
>>>>    			strcat(buf, "\n");
>>>> +		if (strlen(buf) > 800) {
>>>> +			pr_alert("%s", buf);
>>> Does the tools/testing/selftests/rcutorture/bin/kvm-recheck-refscale.sh
>>> script also require changes to handle the partial lines?
Looks it doesn't matter for kvm-recheck-refscale.sh where it will not check these output.


>>>>    		u32 rem;
>>>> @@ -712,9 +711,13 @@ static int main_func(void *arg)
>>>>    		avg = div_u64_rem(result_avg[exp], 1000, &rem);
>>>>    		sprintf(buf1, "%d\t%llu.%03u\n", exp + 1, avg, rem);
>>>>    		strcat(buf, buf1);
>>>> +		if (strlen(buf) > 800) {
>>>> +			pr_alert("%s", buf);
>>>> +			buf[0] = 0;
>>>> +		}
>>>>    	}
>>>> -	SCALEOUT("%s", buf);
>>>> +	pr_alert("%s", buf);
it will not introduce partial lines here, buf  is always ended with '\n'

I have checked the regexp in tools/testing/selftests/rcutorture/bin/kvm-recheck-refscale.sh
and verify it with the new result, it always works.


Thanks
Zhijian



>>>>    err:
>>>>    	// This will shutdown everything including us.
>>>> -- 
>>>> 2.33.0
>>>>
>>>>
>>>>
>>
>