[PATCH] blk-iocost: Fix systemtap error on iocost_ioc_vrate_adj

[PATCH] blk-iocost: Fix systemtap error on iocost_ioc_vrate_adj

[Waiman Long]

Systemtap 4.2 is unable to correctly interpret the "u32 (*missed_ppm)[2]"
argument of the iocost_ioc_vrate_adj trace entry defined in
include/trace/events/iocost.h leading to the following error:

  /tmp/stapAcz0G0/stap_c89c58b83cea1724e26395efa9ed4939_6321_aux_6.c:78:8:
  error: expected ‘;’, ‘,’ or ‘)’ before ‘*’ token
   , u32[]* __tracepoint_arg_missed_ppm

That argument type is indeed rather complex and hard to read. Looking
at block/blk-iocost.c. It is just a 2-entry u32 array. By simplifying
the argument to a simple "u32 *missed_ppm" and adjusting the trace
entry accordingly, the compilation error was gone.

Fixes: 7caa47151ab2 ("blkcg: implement blk-iocost")
Signed-off-by: Waiman Long <longman@redhat.com>
---
 block/blk-iocost.c            | 4 ++--
 include/trace/events/iocost.h | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index db35ee682294..3ab0c1c704b6 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -1591,7 +1591,7 @@ static void ioc_timer_fn(struct timer_list *timer)
 				      vrate_min, vrate_max);
 		}
 
-		trace_iocost_ioc_vrate_adj(ioc, vrate, &missed_ppm, rq_wait_pct,
+		trace_iocost_ioc_vrate_adj(ioc, vrate, missed_ppm, rq_wait_pct,
 					   nr_lagging, nr_shortages,
 					   nr_surpluses);
 
@@ -1600,7 +1600,7 @@ static void ioc_timer_fn(struct timer_list *timer)
 			ioc->period_us * vrate * INUSE_MARGIN_PCT, 100);
 	} else if (ioc->busy_level != prev_busy_level || nr_lagging) {
 		trace_iocost_ioc_vrate_adj(ioc, atomic64_read(&ioc->vtime_rate),
-					   &missed_ppm, rq_wait_pct, nr_lagging,
+					   missed_ppm, rq_wait_pct, nr_lagging,
 					   nr_shortages, nr_surpluses);
 	}
 
diff --git a/include/trace/events/iocost.h b/include/trace/events/iocost.h
index 7ecaa65b7106..c2f580fd371b 100644
--- a/include/trace/events/iocost.h
+++ b/include/trace/events/iocost.h
@@ -130,7 +130,7 @@ DEFINE_EVENT(iocg_inuse_update, iocost_inuse_reset,
 
 TRACE_EVENT(iocost_ioc_vrate_adj,
 
-	TP_PROTO(struct ioc *ioc, u64 new_vrate, u32 (*missed_ppm)[2],
+	TP_PROTO(struct ioc *ioc, u64 new_vrate, u32 *missed_ppm,
 		u32 rq_wait_pct, int nr_lagging, int nr_shortages,
 		int nr_surpluses),
 
@@ -155,8 +155,8 @@ TRACE_EVENT(iocost_ioc_vrate_adj,
 		__entry->old_vrate = atomic64_read(&ioc->vtime_rate);;
 		__entry->new_vrate = new_vrate;
 		__entry->busy_level = ioc->busy_level;
-		__entry->read_missed_ppm = (*missed_ppm)[READ];
-		__entry->write_missed_ppm = (*missed_ppm)[WRITE];
+		__entry->read_missed_ppm = missed_ppm[READ];
+		__entry->write_missed_ppm = missed_ppm[WRITE];
 		__entry->rq_wait_pct = rq_wait_pct;
 		__entry->nr_lagging = nr_lagging;
 		__entry->nr_shortages = nr_shortages;
-- 
2.18.1

Re: [PATCH] blk-iocost: Fix systemtap error on iocost_ioc_vrate_adj

[Steven Rostedt]

On Tue, 21 Apr 2020 09:07:55 -0400
Waiman Long <longman@redhat.com> wrote:

> diff --git a/include/trace/events/iocost.h b/include/trace/events/iocost.h
> index 7ecaa65b7106..c2f580fd371b 100644
> --- a/include/trace/events/iocost.h
> +++ b/include/trace/events/iocost.h
> @@ -130,7 +130,7 @@ DEFINE_EVENT(iocg_inuse_update, iocost_inuse_reset,
>  
>  TRACE_EVENT(iocost_ioc_vrate_adj,
>  
> -	TP_PROTO(struct ioc *ioc, u64 new_vrate, u32 (*missed_ppm)[2],
> +	TP_PROTO(struct ioc *ioc, u64 new_vrate, u32 *missed_ppm,
>  		u32 rq_wait_pct, int nr_lagging, int nr_shortages,
>  		int nr_surpluses),
>  
> @@ -155,8 +155,8 @@ TRACE_EVENT(iocost_ioc_vrate_adj,
>  		__entry->old_vrate = atomic64_read(&ioc->vtime_rate);;
>  		__entry->new_vrate = new_vrate;
>  		__entry->busy_level = ioc->busy_level;
> -		__entry->read_missed_ppm = (*missed_ppm)[READ];
> -		__entry->write_missed_ppm = (*missed_ppm)[WRITE];
> +		__entry->read_missed_ppm = missed_ppm[READ];
> +		__entry->write_missed_ppm = missed_ppm[WRITE];
>  		__entry->rq_wait_pct = rq_wait_pct;
>  		__entry->nr_lagging = nr_lagging;
>  		__entry->nr_shortages = nr_shortages;

Regardless if this helps systemtap or not, I like the patch because the
current code is rather ugly, and this patch makes it more readable.

Suggestion: change the topic to remove systemtap, as that's not going to be
the true reason for acceptance of this patch. It should just be about
cleaning up the trace event itself.

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

Re: [PATCH] blk-iocost: Fix systemtap error on iocost_ioc_vrate_adj

[Tejun Heo]

On Tue, Apr 21, 2020 at 09:07:55AM -0400, Waiman Long wrote:
> Systemtap 4.2 is unable to correctly interpret the "u32 (*missed_ppm)[2]"
> argument of the iocost_ioc_vrate_adj trace entry defined in
> include/trace/events/iocost.h leading to the following error:
> 
>   /tmp/stapAcz0G0/stap_c89c58b83cea1724e26395efa9ed4939_6321_aux_6.c:78:8:
>   error: expected ‘;’, ‘,’ or ‘)’ before ‘*’ token
>    , u32[]* __tracepoint_arg_missed_ppm
> 
> That argument type is indeed rather complex and hard to read. Looking
> at block/blk-iocost.c. It is just a 2-entry u32 array. By simplifying
> the argument to a simple "u32 *missed_ppm" and adjusting the trace
> entry accordingly, the compilation error was gone.
> 
> Fixes: 7caa47151ab2 ("blkcg: implement blk-iocost")
> Signed-off-by: Waiman Long <longman@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH] blk-iocost: Fix systemtap error on iocost_ioc_vrate_adj

[Jens Axboe]

On 4/21/20 7:07 AM, Waiman Long wrote:
> Systemtap 4.2 is unable to correctly interpret the "u32 (*missed_ppm)[2]"
> argument of the iocost_ioc_vrate_adj trace entry defined in
> include/trace/events/iocost.h leading to the following error:
> 
>   /tmp/stapAcz0G0/stap_c89c58b83cea1724e26395efa9ed4939_6321_aux_6.c:78:8:
>   error: expected ‘;’, ‘,’ or ‘)’ before ‘*’ token
>    , u32[]* __tracepoint_arg_missed_ppm
> 
> That argument type is indeed rather complex and hard to read. Looking
> at block/blk-iocost.c. It is just a 2-entry u32 array. By simplifying
> the argument to a simple "u32 *missed_ppm" and adjusting the trace
> entry accordingly, the compilation error was gone.

Applied, thanks.

-- 
Jens Axboe

Re: [PATCH] blk-iocost: Fix systemtap error on iocost_ioc_vrate_adj

[Waiman Long]

On 4/21/20 10:59 AM, Steven Rostedt wrote:
> On Tue, 21 Apr 2020 09:07:55 -0400
> Waiman Long <longman@redhat.com> wrote:
>
>> diff --git a/include/trace/events/iocost.h b/include/trace/events/iocost.h
>> index 7ecaa65b7106..c2f580fd371b 100644
>> --- a/include/trace/events/iocost.h
>> +++ b/include/trace/events/iocost.h
>> @@ -130,7 +130,7 @@ DEFINE_EVENT(iocg_inuse_update, iocost_inuse_reset,
>>  
>>  TRACE_EVENT(iocost_ioc_vrate_adj,
>>  
>> -	TP_PROTO(struct ioc *ioc, u64 new_vrate, u32 (*missed_ppm)[2],
>> +	TP_PROTO(struct ioc *ioc, u64 new_vrate, u32 *missed_ppm,
>>  		u32 rq_wait_pct, int nr_lagging, int nr_shortages,
>>  		int nr_surpluses),
>>  
>> @@ -155,8 +155,8 @@ TRACE_EVENT(iocost_ioc_vrate_adj,
>>  		__entry->old_vrate = atomic64_read(&ioc->vtime_rate);;
>>  		__entry->new_vrate = new_vrate;
>>  		__entry->busy_level = ioc->busy_level;
>> -		__entry->read_missed_ppm = (*missed_ppm)[READ];
>> -		__entry->write_missed_ppm = (*missed_ppm)[WRITE];
>> +		__entry->read_missed_ppm = missed_ppm[READ];
>> +		__entry->write_missed_ppm = missed_ppm[WRITE];
>>  		__entry->rq_wait_pct = rq_wait_pct;
>>  		__entry->nr_lagging = nr_lagging;
>>  		__entry->nr_shortages = nr_shortages;
> Regardless if this helps systemtap or not, I like the patch because the
> current code is rather ugly, and this patch makes it more readable.
>
> Suggestion: change the topic to remove systemtap, as that's not going to be
> the true reason for acceptance of this patch. It should just be about
> cleaning up the trace event itself.
>
> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>
> -- Steve
>
OK, will send a v2 patch to update the commit log. Thanks for the review.

Cheers,
Longman

Re: [PATCH] blk-iocost: Fix systemtap error on iocost_ioc_vrate_adj

[Steven Rostedt]

On Tue, 21 Apr 2020 14:36:29 -0400
Waiman Long <longman@redhat.com> wrote:

> > Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> >
> > -- Steve
> >  
> OK, will send a v2 patch to update the commit log. Thanks for the review.

I think Jens already took this patch.  Doesn't sound like a v2 is needed.

-- Steve

Re: [PATCH] blk-iocost: Fix systemtap error on iocost_ioc_vrate_adj

[Jens Axboe]

On 4/21/20 1:16 PM, Steven Rostedt wrote:
> On Tue, 21 Apr 2020 14:36:29 -0400
> Waiman Long <longman@redhat.com> wrote:
> 
>>> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>>>
>>> -- Steve
>>>  
>> OK, will send a v2 patch to update the commit log. Thanks for the review.
> 
> I think Jens already took this patch.  Doesn't sound like a v2 is needed.

I did, with modified subject line.


-- 
Jens Axboe

Re: [PATCH] blk-iocost: Fix systemtap error on iocost_ioc_vrate_adj

[Waiman Long]

On 4/21/20 3:17 PM, Jens Axboe wrote:
> On 4/21/20 1:16 PM, Steven Rostedt wrote:
>> On Tue, 21 Apr 2020 14:36:29 -0400
>> Waiman Long <longman@redhat.com> wrote:
>>
>>>> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>>>>
>>>> -- Steve
>>>>  
>>> OK, will send a v2 patch to update the commit log. Thanks for the review.
>> I think Jens already took this patch.  Doesn't sound like a v2 is needed.
> I did, with modified subject line.
>
>
Oh, I see. Thanks for taking that.

Cheers,
Longman