| Message ID | 20190310181137.2604-8-paolo.valente@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | block, bfq: fix bugs, reduce exec time and boost performance | expand |
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
> Il giorno 11 mar 2019, alle ore 10:08, Holger Hoffstätte <holger@applied-asynchrony.com> ha scritto: > > Hi, > > Guess what - more problems ;-) The curse of the print SHARED :) Thank you very much Holger for testing what I guiltily did not. I'll send a v3 as Francesco fixes this too. Paolo > 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
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) \ +} while (0) #define bfq_log_bfqg(bfqd, bfqg, fmt, args...) do {} while (0) #endif /* CONFIG_BFQ_GROUP_IOSCHED */