diff mbox series

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

Message ID 20190310181137.2604-8-paolo.valente@linaro.org (mailing list archive)
State New, archived
Headers show
Series block, bfq: fix bugs, reduce exec time and boost performance | expand

Commit Message

Paolo Valente March 10, 2019, 6:11 p.m. UTC 
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(-)

Comments

Holger Hoffstätte March 11, 2019, 9:08 a.m. UTC | #1 
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
Paolo Valente March 11, 2019, 9:13 a.m. UTC | #2 
> 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 mbox series

Patch

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 */