Messages in this thread | | | Subject | Re: [PATCH BUGFIX IMPROVEMENT V2 7/9] block, bfq: print SHARED instead of pid for shared queues in logs | From | Holger Hoffstätte <> | Date | Mon, 11 Mar 2019 10:08:46 +0100 |
| |
Hi,
Guess what - more problems ;-) This time when building without CONFIG_BFQ_GROUP_IOSCHED, see below..
On 3/10/19 7:11 PM, Paolo Valente wrote: > From: Francesco Pollicino <fra.fra.800@gmail.com> > > The function "bfq_log_bfqq" prints the pid of the process > associated with the queue passed as input. > > Unfortunately, if the queue is shared, then more than one process > is associated with the queue. The pid that gets printed in this > case is the pid of one of the associated processes. > Which process gets printed depends on the exact sequence of merge > events the queue underwent. So printing such a pid is rather > useless and above all is often rather confusing because it > reports a random pid between those of the associated processes. > > This commit addresses this issue by printing SHARED instead of a pid > if the queue is shared. > > Signed-off-by: Francesco Pollicino <fra.fra.800@gmail.com> > Signed-off-by: Paolo Valente <paolo.valente@linaro.org> > --- > block/bfq-iosched.c | 10 ++++++++++ > block/bfq-iosched.h | 23 +++++++++++++++++++---- > 2 files changed, 29 insertions(+), 4 deletions(-) > > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c > index 500b04df9efa..7d95d9c01036 100644 > --- a/block/bfq-iosched.c > +++ b/block/bfq-iosched.c > @@ -2590,6 +2590,16 @@ bfq_merge_bfqqs(struct bfq_data *bfqd, struct bfq_io_cq *bic, > * assignment causes no harm). > */ > new_bfqq->bic = NULL; > + /* > + * If the queue is shared, the pid is the pid of one of the associated > + * processes. Which pid depends on the exact sequence of merge events > + * the queue underwent. So printing such a pid is useless and confusing > + * because it reports a random pid between those of the associated > + * processes. > + * We mark such a queue with a pid -1, and then print SHARED instead of > + * a pid in logging messages. > + */ > + new_bfqq->pid = -1; > bfqq->bic = NULL; > /* release process reference to bfqq */ > bfq_put_queue(bfqq); > diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h > index 829730b96fb2..6410cc9a064d 100644 > --- a/block/bfq-iosched.h > +++ b/block/bfq-iosched.h > @@ -32,6 +32,8 @@ > #define BFQ_DEFAULT_GRP_IOPRIO 0 > #define BFQ_DEFAULT_GRP_CLASS IOPRIO_CLASS_BE > > +#define MAX_PID_STR_LENGTH 12 > + > /* > * Soft real-time applications are extremely more latency sensitive > * than interactive ones. Over-raise the weight of the former to > @@ -1016,13 +1018,23 @@ void bfq_add_bfqq_busy(struct bfq_data *bfqd, struct bfq_queue *bfqq); > /* --------------- end of interface of B-WF2Q+ ---------------- */ > > /* Logging facilities. */ > +static void bfq_pid_to_str(int pid, char *str, int len) > +{ > + if (pid != -1) > + snprintf(str, len, "%d", pid); > + else > + snprintf(str, len, "SHARED-"); > +} > + > #ifdef CONFIG_BFQ_GROUP_IOSCHED > struct bfq_group *bfqq_group(struct bfq_queue *bfqq); > > #define bfq_log_bfqq(bfqd, bfqq, fmt, args...) do { \ > + char pid_str[MAX_PID_STR_LENGTH]; \ > + bfq_pid_to_str((bfqq)->pid, pid_str, MAX_PID_STR_LENGTH); \ > blk_add_cgroup_trace_msg((bfqd)->queue, \ > bfqg_to_blkg(bfqq_group(bfqq))->blkcg, \ > - "bfq%d%c " fmt, (bfqq)->pid, \ > + "bfq%s%c " fmt, pid_str, \ > bfq_bfqq_sync((bfqq)) ? 'S' : 'A', ##args); \ > } while (0) > > @@ -1033,10 +1045,13 @@ struct bfq_group *bfqq_group(struct bfq_queue *bfqq); > > #else /* CONFIG_BFQ_GROUP_IOSCHED */ > > -#define bfq_log_bfqq(bfqd, bfqq, fmt, args...) \ > - blk_add_trace_msg((bfqd)->queue, "bfq%d%c " fmt, (bfqq)->pid, \ > +#define bfq_log_bfqq(bfqd, bfqq, fmt, args...) do { \ > + char pid_str[MAX_PID_STR_LENGTH]; \ > + bfq_pid_to_str((bfqq)->pid, pid_str, MAX_PID_STR_LENGTH); \ > + blk_add_trace_msg((bfqd)->queue, "bfq%s%c " fmt, pid_str, \ > bfq_bfqq_sync((bfqq)) ? 'S' : 'A', \ > - ##args) > + ##args) \ ---------------------------------------^ compilation error due to missing ;
> +} while (0) > #define bfq_log_bfqg(bfqd, bfqg, fmt, args...) do {} while (0) ^^^^^^^^^^^^^^^^^^^^^^^^ Here you re- and effectively undefine the previous new bfq_log_bfqq() definition with an empty block; I think you wanted to delete the second (empty) definition, otherwise the new code won't do much.
Finally there is now a warning at bfq-cgroup.c:25 that bfq_pid_to_str() is defined but not used (in that compilation unit) since it is defined unconditionally for both cases of CONFIG_BFQ_GROUP_IOSCHED, which is required for bfq-cgroup.c. Since reordering the includes there won't work due to ifdef overlaps, the easiest fix for that is to mark bfq_pid_to_str() as "static inline", as already suggested by Oleksandr.
With those changes it builds.
cheers, Holger
| |