Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\))
Subject: Re: [PATCH BUGFIX IMPROVEMENT V2 7/9] block, bfq: print SHARED
 instead of pid for shared queues in logs
From:   Paolo Valente <paolo.valente@linaro.org>
In-Reply-To: <491be7d2-cbe2-a1f1-e102-b449b3a8ace6@applied-asynchrony.com>
Date:   Mon, 11 Mar 2019 10:13:31 +0100
Cc:     Jens Axboe <axboe@kernel.dk>,
        linux-block <linux-block@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Ulf Hansson <ulf.hansson@linaro.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        Mark Brown <broonie@kernel.org>,
        'Oleksandr Natalenko' via bfq-iosched 
        <bfq-iosched@googlegroups.com>, oleksandr@natalenko.name,
        fra.fra.800@gmail.com, alessio.masola@gmail.com
Content-Transfer-Encoding: quoted-printable
Message-Id: <74D5550B-765A-46C4-9663-F2CEA2E95F5E@linaro.org>
References: <20190310181137.2604-1-paolo.valente@linaro.org>
 <20190310181137.2604-8-paolo.valente@linaro.org>
 <491be7d2-cbe2-a1f1-e102-b449b3a8ace6@applied-asynchrony.com>
To:     =?utf-8?Q?Holger_Hoffst=C3=A4tte?= <holger@applied-asynchrony.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> Il giorno 11 mar 2019, alle ore 10:08, Holger Hoffst=C3=A4tte =
<holger@applied-asynchrony.com> ha scritto:
>=20
> Hi,
>=20
> 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..
>=20
> 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 =3D 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 =3D -1;
>>  	bfqq->bic =3D 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 !=3D -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 ;
>=20
>> +} 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.
>=20
> 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.
>=20
> With those changes it builds.
>=20
> cheers,
> Holger