| Message ID | 20200421130755.18370-1-longman@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-iocost: Fix systemtap error on iocost_ioc_vrate_adj | expand |
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
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.
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.
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
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
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.
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
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;
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(-)