Message-ID: <20110503212656.GG32331@Krystal>
Date:	Tue, 3 May 2011 17:26:57 -0400
From:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	LKML <linux-kernel@...r.kernel.org>, Ingo Molnar <mingo@...e.hu>,
	Thomas Gleixner <tglx@...utronix.de>,
	Frederic Weisbecker <fweisbec@...il.com>
Subject: Re: [RFC patch 01/32] TRACE_EVENT: gradual semicolon removal

(I dropped the large CC list from this thread to fit within LKML 20 CC
list limit. I kept them in bcc so they see this message. Please feel
free to follow the rest of the thread on LKML.)

* Steven Rostedt (rostedt@...dmis.org) wrote:
> On Mon, 2011-05-02 at 17:11 -0400, Mathieu Desnoyers wrote:
> > plain text document attachment
> > (trace-event-gradual-semicolon-removal.patch)
> >  The side-effect of these extra semicolons are:
> > 
> > a) to pollute the preprocessor output, leaving extra ";" between trace event
> > instances in trace points creation.
> 
> Who cares?

Anyone trying to ensure tracepoints are well implemented (and thus that
their resulting preprocessor output is bug-free). I did the exercise
myself, and ended up with:

commit 881fe4bdcdc899674524e30a76c76d298b447008
    tracing: remove duplicate null-pointer check in skb tracepoint
commit 23036f1a340beec19cc451ba9719526c4ffb3a57
    blktrace: add missing probe argument to block_bio_complete

so I think the preprocessor output should be more digestible by
developers, with event some documentation on how to format it so it
becomes readable. The following bash script does it for instance (I am
certain that there are ton of much better ways to do it, but this is
what I use):

#!/bin/sh

# kcpp:
# Create a .cpp file (c preprocessor output) from the same gcc
# invokation that builds an object file.

# kcpp SOURCE OBJECT
# invoke with e.g. kcpp kernel/sched.c kernel/sched.o
#  output file will be kernel/sched.cpp
# needs gcc, sed and indent

SOURCE=$1
OBJECT=$2
FILECPP=$(echo $1 | sed 's/\.c/\.cpp/')
POBJECT=$(echo ${OBJECT} | sed -e 's/\//\\\//' -e 's/\./\\\./')
PFILECPP=$(echo ${FILECPP} | sed -e 's/\//\\\//' -e 's/\./\\\./')

echo "Preprocessing ${SOURCE} -> ${FILECPP}"
echo "Object build monitored: ${OBJECT}"
touch ${SOURCE}
rm -f ${OBJECT}

CPPCMD=$(make V=1 ${OBJECT} | tail -n 1 | sed 's/ -c / -E /')
CPPCMD=$(echo ${CPPCMD} | sed "s/ -o [^ ]* / -o ${PFILECPP} /")
PCPPCMD=$(echo ${CPPCMD} | sed -e 's/"//g')
echo Invoking:
echo ${PCPPCMD}
${PCPPCMD}

echo "Formatting ${FILECPP}... "
mv ${FILECPP} ${FILECPP}.tmp
indent ${FILECPP}.tmp -o ${FILECPP}
echo "done."
echo
echo "Output preprocessor file can be found in ${FILECPP}"


> 
> > 
> > b) to render impossible creation of an array of events. Extra semicolons are
> > not a matter as long as TRACE_EVENT creates statements, structure elements or
> > functions, because extra semicolons are discarded by the compiler. However,
> > when creating an array, the separator is the comma, and the semicolon causes a
> > parse error.
> 
> This is the important part.

Yep.

> 
> > 
> > 
> > * So what is the motivation for removing these semicolons ?
> > 
> > Currently, Ftrace is creating a "ftrace_define_fields_##call" function for each
> > event to define the event fields, which consumes space. It also has the
> > ".fields" list head to keep a dynamically generated list of event fields.
> > 
> > By allowing creation of arrays of events, we can do the following: turn the
> > dynamically created list of event fields into a first-level const array listing
> > event fields. We can use ARRAY_SIZE() on this array to know its size,
> > statically. Then, in a following trace event stage, we can create another const
> > array containing tuples of (pointers to the event-specific arrays, array size).
> > 
> > So we get all the same information Ftrace currently gets with much less code
> > overall, much less read-write data, and less dynamic initialization code.
> 
> That looks like something worth doing.
> 
> > 
> > 
> > * Why do this incrementally ?
> 
> Preaching to the choir?

I've been asked to explain myself on this topic by Frederic last time I
posted this patchset, so I added the explanation to the changelog.

> 
> > 
> > After this preliminary patch, the semicolon removal can be done gradually
> > without breaking the build: we can be in an intermediate state with some files
> > having semicolons and others without. This is therefore good for
> > bissectability, and seems like a nice way to bring in an internal API change
> > without making developers suffer too much. Then, once things are stabilized, we
> > can start modifying the Ftrace code to generate the more space-efficient arrays
> > (possibly in a release-cycle or so), knowing that this enforces a strict
> > requirement on semicolon removal.
> 
> Mathieu, I like the patch, but I think you explain too much ;)

Or too little, depending on the maintainer. ;-)

More seriously, I can remove pieces of changelog text if you think this
is too much. It's your call.

> 
> 
> > 
> > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
> > Acked-by: Frederic Weisbecker <fweisbec@...il.com>
> > CC: Alexander Graf <agraf@...e.de>
> > CC: Alex Elder <aelder@....com>
> > CC: Anton Blanchard <anton@...ba.org>
> > CC: Arjan van de Ven <arjan@...ux.intel.com>
> > CC: Avi Kivity <avi@...hat.com>
> > CC: Benjamin Herrenschmidt <benh@...nel.crashing.org>
> > CC: Christoph Hellwig <hch@....de>
> > CC: Chris Wilson <chris@...is-wilson.co.uk>
> > CC: Dave Airlie <airlied@...hat.com>
> > CC: Dave Chinner <david@...morbit.com>
> > CC: Dave Chinner <dchinner@...hat.com>
> > CC: David S. Miller <davem@...emloft.net>
> > CC: Gleb Natapov <gleb@...hat.com>
> > CC: Hidetoshi Seto <seto.hidetoshi@...fujitsu.com>
> > CC: Ingo Molnar <mingo@...e.hu>
> > CC: James Bottomley <James.Bottomley@...e.de>
> > CC: Jean Pihet <j-pihet@...com>
> > CC: Jeff Moyer <jmoyer@...hat.com>
> > CC: Jens Axboe <axboe@...nel.dk>
> > CC: Jeremy Kerr <jk@...abs.org>
> > CC: Jesse Barnes <jbarnes@...tuousgeek.org>
> > CC: Joerg Roedel <joerg.roedel@....com>
> > CC: Johannes Berg <johannes@...solutions.net>
> > CC: John W. Linville <linville@...driver.com>
> > CC: Josh Stone <jistone@...hat.com>
> > CC: Kei Tokunaga <tokunaga.keiich@...fujitsu.com>
> > CC: Koki Sanagi <sanagi.koki@...fujitsu.com>
> > CC: KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
> > CC: Larry Woodman <lwoodman@...hat.com>
> > CC: Len Brown <len.brown@...el.com>
> > CC: Li Zefan <lizf@...fujitsu.com>
> > CC: Marcelo Tosatti <mtosatti@...hat.com>
> > CC: Martin K. Petersen <martin.petersen@...cle.com>
> > CC: Masami Hiramatsu <mhiramat@...hat.com>
> > CC: Mel Gorman <mel@....ul.ie>
> > CC: Neil Horman <nhorman@...driver.com>
> > CC: Oleg Nesterov <oleg@...hat.com>
> > CC: Paul Mackerras <paulus@...ba.org>
> > CC: Peter Zijlstra <peterz@...radead.org>
> > CC: Reinette Chatre <reinette.chatre@...el.com>
> > CC: Rik van Riel <riel@...hat.com>
> > CC: Roland McGrath <roland@...hat.com>
> > CC: Rusty Russell <rusty@...tcorp.com.au>
> > CC: Steven Rostedt <rostedt@...dmis.org>
> > CC: Steven Whitehouse <swhiteho@...hat.com>
> > CC: Tejun Heo <tj@...nel.org>
> > CC: Theodore Ts'o <tytso@....edu>
> > CC: Thomas Gleixner <tglx@...utronix.de>
> > CC: Thomas Renninger <trenn@...e.de>
> > CC: Tomohiro Kusumi <kusumi.tomohiro@...fujitsu.com>
> > CC: Vadim Rozenfeld <vrozenfe@...hat.com>
> > CC: Xiao Guangrong <xiaoguangrong@...fujitsu.com>
> > CC: Zhu Yi <yi.zhu@...el.com>
> 
> I don't think you have enough Cc's
> 

Yeah, I'll cut down this list. Given your explanation in another mail,
people interested to know the context of a patch should have enough
information with the patch changelog to know what is happening, and to
get to LKML to find the other relevant changes.

Thanks,

Mathieu



> -- Steve
> 
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists