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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <491be7d2-cbe2-a1f1-e102-b449b3a8ace6@applied-asynchrony.com>
Date:   Mon, 11 Mar 2019 10:08:46 +0100
From:   Holger Hoffstätte <holger@...lied-asynchrony.com>
To:     Paolo Valente <paolo.valente@...aro.org>,
        Jens Axboe <axboe@...nel.dk>
Cc:     linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
        ulf.hansson@...aro.org, linus.walleij@...aro.org,
        broonie@...nel.org, bfq-iosched@...glegroups.com,
        oleksandr@...alenko.name, fra.fra.800@...il.com,
        alessio.masola@...il.com
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 <fra.fra.800@...il.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@...il.com>
> Signed-off-by: Paolo Valente <paolo.valente@...aro.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