linux-kernel - Re: [PATCH v3] mmc: Provide tracepoints for request processing

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] [day] [month] [year] [list]

Date:	Wed, 30 Mar 2016 17:42:00 +0800
From:	Baolin Wang <baolin.wang@...aro.org>
To:	Ulf Hansson <ulf.hansson@...aro.org>
Cc:	Steven Rostedt <rostedt@...dmis.org>, mingo@...hat.com,
	Adrian Hunter <adrian.hunter@...el.com>,
	Yangbo Lu <yangbo.lu@...escale.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"JBottomley@...n.com" <JBottomley@...n.com>,
	"Luca Porzio (lporzio)" <lporzio@...ron.com>,
	Jon Hunter <jonathanh@...dia.com>,
	Grant Grundler <grundler@...omium.org>,
	Jens Axboe <axboe@...com>, Fabian Frederick <fabf@...net.be>,
	Yunpeng Gao <yunpeng.gao@...el.com>,
	Dan Williams <dan.j.williams@...el.com>,
	Rabin Vincent <rabin.vincent@...s.com>,
	"Dong, Chuanxiao" <chuanxiao.dong@...el.com>,
	Shawn Lin <shawn.lin@...k-chips.com>,
	Heiko Stübner <heiko@...ech.de>,
	Doug Anderson <dianders@...omium.org>,
	David Jander <david@...tonic.nl>,
	Mark Brown <broonie@...nel.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Takahiro Akashi <takahiro.akashi@...aro.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	linux-mmc <linux-mmc@...r.kernel.org>
Subject: Re: [PATCH v3] mmc: Provide tracepoints for request processing

On 30 March 2016 at 17:25, Ulf Hansson <ulf.hansson@...aro.org> wrote:
> On 28 March 2016 at 08:39, Baolin Wang <baolin.wang@...aro.org> wrote:
>> This patch provides some tracepoints for the lifecycle of a mmc request from
>> starting to completion to help with performance analysis of MMC subsystem.
>>
>> Changes since v2:
>>  - Remove some redundant tracepoints which are repeated in block layer.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@...aro.org>
>> ---
>>  drivers/mmc/core/core.c    |    7 +++
>>  include/trace/events/mmc.h |  129 ++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 136 insertions(+)
>>  create mode 100644 include/trace/events/mmc.h
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index f95d41f..7222e3f 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -36,6 +36,9 @@
>>  #include <linux/mmc/sd.h>
>>  #include <linux/mmc/slot-gpio.h>
>>
>> +#define CREATE_TRACE_POINTS
>> +#include <trace/events/mmc.h>
>> +
>>  #include "core.h"
>>  #include "bus.h"
>>  #include "host.h"
>> @@ -152,6 +155,8 @@ void mmc_request_done(struct mmc_host *host, struct mmc_request *mrq)
>>
>>                 led_trigger_event(host->led, LED_OFF);
>>
>> +               trace_mmc_request_done(host, mrq);
>> +
>
> At this point we will lack information about "retries" and also about
> the re-tune state. I think both would be valuable information to share
> about each request.
>
> So, I would suggest you to move the trace a bit further up, before the
> if-sentence and include "retries" and the "re-tune state" via trace
> print.

Make sense. I'll add the "retries" and the "re-tune state" information
in next version.

>
>>                 if (mrq->sbc) {
>>                         pr_debug("%s: req done <CMD%u>: %d: %08x %08x %08x %08x\n",
>>                                 mmc_hostname(host), mrq->sbc->opcode,
>> @@ -229,6 +234,8 @@ static int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
>>         if (mmc_card_removed(host->card))
>>                 return -ENOMEDIUM;
>>
>> +       trace_mmc_request_start(host, mrq);
>
> This isn't the only place a request will be started from, thus we may
> be missing valuable information about which command/request is being
> sent.
>
> I would move this to __mmc_start_request() instead and similar to my
> upper comment, please add "retries" and "re-tune state" in the trace
> print.

Make sense. Thanks for your comments.

>
>> +
>>         if (mrq->sbc) {
>>                 pr_debug("<%s: starting CMD%u arg %08x flags %08x>\n",
>>                          mmc_hostname(host), mrq->sbc->opcode,
>>
>
> [...]
>
> Kind regards
> Uffe



-- 
Baolin.wang
Best Regards