[RFC] mtd: add tracepoints for mtd_read and mtd_write

Add tracepoints to mtd subsystem for read and write
operations. This should allow us to identify applications
which are generating too much reads/writes on a flash-device.

Signed-off-by: Daniel Walter <dwalter@sigma-star.at>
---
 drivers/mtd/mtdcore.c      |  6 +++++-
 include/trace/events/mtd.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 1 deletion(-)
 create mode 100644 include/trace/events/mtd.h

On Tue, Nov 17, 2015 at 8:18 PM, Daniel Walter <dwalter@sigma-star.at> wrote:
> Add tracepoints to mtd subsystem for read and write
> operations. This should allow us to identify applications
> which are generating too much reads/writes on a flash-device.
>
> Signed-off-by: Daniel Walter <dwalter@sigma-star.at>
> ---
>  drivers/mtd/mtdcore.c      |  6 +++++-
>  include/trace/events/mtd.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+), 1 deletion(-)
>  create mode 100644 include/trace/events/mtd.h
>
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 8bbbb75..8f85bfb 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -43,6 +43,9 @@
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/partitions.h>
>
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/mtd.h>
> +
>  #include "mtdcore.h"
>
>  static struct backing_dev_info mtd_bdi = {
> @@ -882,7 +885,7 @@ int mtd_read(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen,
>                 return -EINVAL;
>         if (!len)
>                 return 0;
> -
> +       trace_mtd_read(current, len);
>         /*
>          * In the absence of an error, drivers return a non-negative integer
>          * representing the maximum number of bitflips that were corrected on
> @@ -907,6 +910,7 @@ int mtd_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen,
>                 return -EROFS;
>         if (!len)
>                 return 0;
> +       trace_mtd_write(current, len);
>         return mtd->_write(mtd, to, len, retlen, buf);
>  }
>  EXPORT_SYMBOL_GPL(mtd_write);
> diff --git a/include/trace/events/mtd.h b/include/trace/events/mtd.h
> new file mode 100644
> index 0000000..7191b72
> --- /dev/null
> +++ b/include/trace/events/mtd.h
> @@ -0,0 +1,45 @@
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM mtd
> +
> +#if !defined(_TRACE_MTD_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_MTD_H
> +
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(mtd_read,
> +       TP_PROTO(struct task_struct *task, size_t rlen),
> +       TP_ARGS(task, rlen),
> +       TP_STRUCT__entry(
> +               __field(pid_t, pid)
> +               __array(char, comm, TASK_COMM_LEN)
> +               __field(size_t, rlen)
> +       ),
> +       TP_fast_assign(
> +               __entry->pid = task->pid;
> +               memcpy(__entry->comm, task->comm, TASK_COMM_LEN);
> +               __entry->rlen = rlen;
> +       ),
> +       TP_printk("pid=%d comm=%s rlen=%zu",
> +               __entry->pid, __entry->comm, __entry->rlen)
> +);
> +
> +TRACE_EVENT(mtd_write,
> +       TP_PROTO(struct task_struct *task, size_t wlen),
> +       TP_ARGS(task, wlen),
> +       TP_STRUCT__entry(
> +               __field(pid_t, pid)
> +               __array(char, comm, TASK_COMM_LEN)
> +               __field(size_t, wlen)
> +       ),
> +       TP_fast_assign(
> +               __entry->pid = task->pid;
> +               memcpy(__entry->comm, task->comm, TASK_COMM_LEN);
> +               __entry->wlen = wlen;
> +       ),
> +       TP_printk("pid=%d comm=%s wlen=%zu",
> +               __entry->pid, __entry->comm, __entry->wlen)
> +);
> +#endif /* _TRACE_MTD_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>

Brian, any comments on this?

On Fri, Jan 08, 2016 at 10:33:03AM +0100, Richard Weinberger wrote:
> On Tue, Nov 17, 2015 at 8:18 PM, Daniel Walter <dwalter@sigma-star.at> wrote:
> > Add tracepoints to mtd subsystem for read and write
> > operations. This should allow us to identify applications
> > which are generating too much reads/writes on a flash-device.
> >
> > Signed-off-by: Daniel Walter <dwalter@sigma-star.at>
> 
> Brian, any comments on this?

Only a few. I'm not very familiar with writing trace events. I've only
barely used them! And I haven't run this patch yet.

But I thought Daniel reported having some difficulty here -- that it
isn't actually that useful to get real "user" information, since most
writing/erasing happens from background UBI tasks in practice. So is
this still the best mechanism for tracking this information? (Even if
there's better ways to get info directly out of UBI, I guess maybe it'd
still be good to have this infrastructure.)

Also, maybe I'm missing something, but it doesn't look like this
attempts to record *which* MTD is being accessed, right? I'd think
logging mtd->name could help?

Brian

On 01/08/2016 06:57 PM, Brian Norris wrote:
> On Fri, Jan 08, 2016 at 10:33:03AM +0100, Richard Weinberger wrote:
>> On Tue, Nov 17, 2015 at 8:18 PM, Daniel Walter <dwalter@sigma-star.at> wrote:
>>> Add tracepoints to mtd subsystem for read and write
>>> operations. This should allow us to identify applications
>>> which are generating too much reads/writes on a flash-device.
>>>
>>> Signed-off-by: Daniel Walter <dwalter@sigma-star.at>
>>
>> Brian, any comments on this?
> 
> Only a few. I'm not very familiar with writing trace events. I've only
> barely used them! And I haven't run this patch yet.
> 
> But I thought Daniel reported having some difficulty here -- that it
> isn't actually that useful to get real "user" information, since most
> writing/erasing happens from background UBI tasks in practice. So is
> this still the best mechanism for tracking this information? (Even if
> there's better ways to get info directly out of UBI, I guess maybe it'd
> still be good to have this infrastructure.)
> 
> Also, maybe I'm missing something, but it doesn't look like this
> attempts to record *which* MTD is being accessed, right? I'd think
> logging mtd->name could help?
> 
> Brian
> 
Hi,

sorry for the late reply, filters ate your mail :/
I'll add mtd->name, since this sounds like a great idea.
regarding your other mail (mtd_erase/read/write_oob), I'll add
tracepoints for them as well (they have been on my list anyway).

Thanks for the input, expect a v2 later this week.

cheers,

daniel