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 <[email protected]>
---
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
On Tue, 21 Apr 2020 09:07:55 -0400
Waiman Long <[email protected]> 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) <[email protected]>
-- Steve
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 <[email protected]>
Acked-by: Tejun Heo <[email protected]>
Thanks.
--
tejun
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
On 4/21/20 10:59 AM, Steven Rostedt wrote:
> On Tue, 21 Apr 2020 09:07:55 -0400
> Waiman Long <[email protected]> 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) <[email protected]>
>
> -- Steve
>
OK, will send a v2 patch to update the commit log. Thanks for the review.
Cheers,
Longman
On 4/21/20 1:16 PM, Steven Rostedt wrote:
> On Tue, 21 Apr 2020 14:36:29 -0400
> Waiman Long <[email protected]> wrote:
>
>>> Acked-by: Steven Rostedt (VMware) <[email protected]>
>>>
>>> -- 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
On Tue, 21 Apr 2020 14:36:29 -0400
Waiman Long <[email protected]> wrote:
> > Acked-by: Steven Rostedt (VMware) <[email protected]>
> >
> > -- 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
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 <[email protected]> wrote:
>>
>>>> Acked-by: Steven Rostedt (VMware) <[email protected]>
>>>>
>>>> -- 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