From mboxrd@z Thu Jan  1 00:00:00 1970
From: Christoph Hellwig <hch@infradead.org>
Subject: Re: [PATCH 1/3] writeback: initial tracing support
Date: Mon, 21 Jun 2010 04:09:42 -0400
Message-ID: <20100621080942.GA19290@infradead.org>
References: <1277083526-21002-1-git-send-email-david@fromorbit.com>
 <1277083526-21002-2-git-send-email-david@fromorbit.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: axboe@kernel.dk, linux-fsdevel@vger.kernel.org
To: Dave Chinner <david@fromorbit.com>
Return-path: <linux-fsdevel-owner@vger.kernel.org>
Received: from [18.85.46.34] ([18.85.46.34]:50932 "EHLO bombadil.infradead.org"
	rhost-flags-FAIL-FAIL-OK-OK) by vger.kernel.org with ESMTP
	id S1753945Ab0FUIJt (ORCPT <rfc822;linux-fsdevel@vger.kernel.org>);
	Mon, 21 Jun 2010 04:09:49 -0400
Content-Disposition: inline
In-Reply-To: <1277083526-21002-2-git-send-email-david@fromorbit.com>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

On Mon, Jun 21, 2010 at 11:25:24AM +1000, Dave Chinner wrote:
> +#include <linux/ftrace.h>

I think this should be <linux/tracepoint.h>.


I fear this might need some more rediffing after my latests writeback
patches.

> +	TP_fast_assign(
> +		strlcpy(__entry->name, dev_name(bdi->dev), 16);

For most other fs/block dev tracepoints we print the dev_t as
major:minor.  It would be good to have some consitency to match on
for the trace points.

> +		__entry->nr_pages	= args->nr_pages;
> +		__entry->sb		= !!args->sb;

It might be worth to store the dev_t of the superblock here?

> +	TP_printk("%s: pages=%ld, sb=%d, kupdate=%d, range_cyclic=%d "
> +		  "for_background=%d", __entry->name, __entry->nr_pages,
> +			__entry->sb, __entry->for_kupdate,
> +			__entry->range_cyclic, __entry->for_background)

> +TRACE_EVENT(writeback_sched,
> +
> +	TP_PROTO(struct backing_dev_info *bdi, struct bdi_work *work,
> +		 const char *msg),
> +
> +	TP_ARGS(bdi, work, msg),
> +
> +	TP_STRUCT__entry(
> +		__array(char,			name,		16)
> +		__field(unsigned int,		work)
> +		__array(char,			task,		8)
> +	),

Instead of the task field I'd make this a calss with three instances
for the different types of wakeups.

> +
> +	TP_fast_assign(
> +		strlcpy(__entry->name, dev_name(bdi->dev), 16);
> +		__entry->work = (unsigned long) work & 0xffff;

The unsigned long allows to store the whole pointer.  Or we could just
store it as a void pointer and print it using %p to make the types
simpler.

> +	TP_PROTO(const char *name, int start),
> +
> +	TP_ARGS(name, start),
> +
> +	TP_STRUCT__entry(
> +		__array(char,	name,		16)
> +		__field(int,	start)
> +	),
> +
> +	TP_fast_assign(
> +		strlcpy(__entry->name, name, 16);
> +		__entry->start = start;
> +	),
> +
> +	TP_printk("%s: %s", __entry->name,
> +			__entry->start ? "registered" : "unregistered")

Again, the clean way would be to have class with two instances instead
of using one tracepoint for two different events and differenciate them
by a string.