by Holger Hoffstätte

[permalink] [raw]

Subject: Re: [PATCH BUGFIX IMPROVEMENT V2 7/9] block, bfq: print SHARED instead of pid for shared queues in logs

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 <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Paolo Valente <[email protected]>
> ---
> 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

2019-03-11 09:14:18

by Paolo Valente

[permalink] [raw]

Subject: Re: [PATCH BUGFIX IMPROVEMENT V2 7/9] block, bfq: print SHARED instead of pid for shared queues in logs

> Il giorno 11 mar 2019, alle ore 10:08, Holger Hoffstätte <[email protected]> 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 <[email protected]>
>> 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 <[email protected]>
>> Signed-off-by: Paolo Valente <[email protected]>
>> ---
>> 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