[PATCH -tip 2/3] Tracing/ftrace: Adapt mmiotrace to the new type of print_line

[PATCH -tip 2/3] Tracing/ftrace: Adapt mmiotrace to the new type of print_line

[Frederic Weisbecker]

Adapt mmiotrace to the new print_line type.
By default, it refuses types it doesn't support.
But a tracer needs to print something to the seq anyway. 
That's why it needs to warn about unsupported types.

Pekka if you disagree with it, please don't hesitate to tell me.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
diff --git a/kernel/trace/trace_mmiotrace.c b/kernel/trace/trace_mmiotrace.c
index 2bbb7dd..69480a5 100644
--- a/kernel/trace/trace_mmiotrace.c
+++ b/kernel/trace/trace_mmiotrace.c
@@ -171,7 +171,7 @@ print_out:
 	return (ret == -EBUSY) ? 0 : ret;
 }
 
-static int mmio_print_rw(struct trace_iterator *iter)
+static enum print_line_t mmio_print_rw(struct trace_iterator *iter)
 {
 	struct trace_entry *entry = iter->ent;
 	struct mmiotrace_rw *rw	= &entry->field.mmiorw;
@@ -209,11 +209,11 @@ static int mmio_print_rw(struct trace_iterator *iter)
 		break;
 	}
 	if (ret)
-		return 1;
-	return 0;
+		return TRACE_TYPE_HANDLED;
+	return TRACE_TYPE_PARTIAL_LINE;
 }
 
-static int mmio_print_map(struct trace_iterator *iter)
+static enum print_line_t mmio_print_map(struct trace_iterator *iter)
 {
 	struct trace_entry *entry = iter->ent;
 	struct mmiotrace_map *m	= &entry->field.mmiomap;
@@ -221,7 +221,7 @@ static int mmio_print_map(struct trace_iterator *iter)
 	unsigned long long t	= ns2usecs(entry->field.t);
 	unsigned long usec_rem	= do_div(t, 1000000ULL);
 	unsigned secs		= (unsigned long)t;
-	int ret = 1;
+	int ret;
 
 	switch (entry->field.mmiorw.opcode) {
 	case MMIO_PROBE:
@@ -241,11 +241,11 @@ static int mmio_print_map(struct trace_iterator *iter)
 		break;
 	}
 	if (ret)
-		return 1;
-	return 0;
+		return TRACE_TYPE_HANDLED;
+	return TRACE_TYPE_PARTIAL_LINE;
 }
 
-static int mmio_print_mark(struct trace_iterator *iter)
+static enum print_line_t mmio_print_mark(struct trace_iterator *iter)
 {
 	struct trace_entry *entry = iter->ent;
 	const char *msg		= entry->field.print.buf;
@@ -258,16 +258,16 @@ static int mmio_print_mark(struct trace_iterator *iter)
 	/* The trailing newline must be in the message. */
 	ret = trace_seq_printf(s, "MARK %lu.%06lu %s", secs, usec_rem, msg);
 	if (!ret)
-		return 0;
+		return TRACE_TYPE_PARTIAL_LINE;
 
 	if (entry->field.flags & TRACE_FLAG_CONT)
 		trace_seq_print_cont(s, iter);
 
-	return 1;
+	return TRACE_TYPE_HANDLED;
 }
 
 /* return 0 to abort printing without consuming current entry in pipe mode */
-static int mmio_print_line(struct trace_iterator *iter)
+static enum print_line_t mmio_print_line(struct trace_iterator *iter)
 {
 	switch (iter->ent->type) {
 	case TRACE_MMIO_RW:
@@ -277,7 +277,9 @@ static int mmio_print_line(struct trace_iterator *iter)
 	case TRACE_PRINT:
 		return mmio_print_mark(iter);
 	default:
-		return 0; /* ignore unknown entries whithout consuming */
+		/* ignore unknown entries */
+		trace_seq_printf(&iter->seq, "Unknown trace type\n");
+		return TRACE_TYPE_HANDLED;
 	}
 }
 

Re: [PATCH -tip 2/3] Tracing/ftrace: Adapt mmiotrace to the new type of print_line

[Pekka Paalanen]

On Fri, 26 Sep 2008 17:44:07 +0200
Frederic Weisbecker <fweisbec@gmail.com> wrote:

> Adapt mmiotrace to the new print_line type.
> By default, it refuses types it doesn't support.
> But a tracer needs to print something to the seq anyway. 
> That's why it needs to warn about unsupported types.
> 
> Pekka if you disagree with it, please don't hesitate to tell me.

Yes, I do disagree about printing stuff that doesn't belong there.
First, I doubt printing bogus text is a right way to fix the
early pipe EOF problem. Second, if you really have to do that,
do it so that it obeys the mmiotrace log format specification.
(I'd recommend a MARK entry.) The spec is in
Documentation/tracers/mmiotrace.txt.

This is just a very quick comment, and I should look at your two
patches better. I hope to do that during the weekend.

Was there supposed to be a third patch, as the subject suggests?

Thanks.


> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> ---
> diff --git a/kernel/trace/trace_mmiotrace.c b/kernel/trace/trace_mmiotrace.c
> index 2bbb7dd..69480a5 100644
> --- a/kernel/trace/trace_mmiotrace.c
> +++ b/kernel/trace/trace_mmiotrace.c
> @@ -171,7 +171,7 @@ print_out:
>  	return (ret == -EBUSY) ? 0 : ret;
>  }
>  
> -static int mmio_print_rw(struct trace_iterator *iter)
> +static enum print_line_t mmio_print_rw(struct trace_iterator *iter)
>  {
>  	struct trace_entry *entry = iter->ent;
>  	struct mmiotrace_rw *rw	= &entry->field.mmiorw;
> @@ -209,11 +209,11 @@ static int mmio_print_rw(struct trace_iterator *iter)
>  		break;
>  	}
>  	if (ret)
> -		return 1;
> -	return 0;
> +		return TRACE_TYPE_HANDLED;
> +	return TRACE_TYPE_PARTIAL_LINE;
>  }
>  
> -static int mmio_print_map(struct trace_iterator *iter)
> +static enum print_line_t mmio_print_map(struct trace_iterator *iter)
>  {
>  	struct trace_entry *entry = iter->ent;
>  	struct mmiotrace_map *m	= &entry->field.mmiomap;
> @@ -221,7 +221,7 @@ static int mmio_print_map(struct trace_iterator *iter)
>  	unsigned long long t	= ns2usecs(entry->field.t);
>  	unsigned long usec_rem	= do_div(t, 1000000ULL);
>  	unsigned secs		= (unsigned long)t;
> -	int ret = 1;
> +	int ret;
>  
>  	switch (entry->field.mmiorw.opcode) {
>  	case MMIO_PROBE:
> @@ -241,11 +241,11 @@ static int mmio_print_map(struct trace_iterator *iter)
>  		break;
>  	}
>  	if (ret)
> -		return 1;
> -	return 0;
> +		return TRACE_TYPE_HANDLED;
> +	return TRACE_TYPE_PARTIAL_LINE;
>  }
>  
> -static int mmio_print_mark(struct trace_iterator *iter)
> +static enum print_line_t mmio_print_mark(struct trace_iterator *iter)
>  {
>  	struct trace_entry *entry = iter->ent;
>  	const char *msg		= entry->field.print.buf;
> @@ -258,16 +258,16 @@ static int mmio_print_mark(struct trace_iterator *iter)
>  	/* The trailing newline must be in the message. */
>  	ret = trace_seq_printf(s, "MARK %lu.%06lu %s", secs, usec_rem, msg);
>  	if (!ret)
> -		return 0;
> +		return TRACE_TYPE_PARTIAL_LINE;
>  
>  	if (entry->field.flags & TRACE_FLAG_CONT)
>  		trace_seq_print_cont(s, iter);
>  
> -	return 1;
> +	return TRACE_TYPE_HANDLED;
>  }
>  
>  /* return 0 to abort printing without consuming current entry in pipe mode */
> -static int mmio_print_line(struct trace_iterator *iter)
> +static enum print_line_t mmio_print_line(struct trace_iterator *iter)
>  {
>  	switch (iter->ent->type) {
>  	case TRACE_MMIO_RW:
> @@ -277,7 +277,9 @@ static int mmio_print_line(struct trace_iterator *iter)
>  	case TRACE_PRINT:
>  		return mmio_print_mark(iter);
>  	default:
> -		return 0; /* ignore unknown entries whithout consuming */
> +		/* ignore unknown entries */
> +		trace_seq_printf(&iter->seq, "Unknown trace type\n");
> +		return TRACE_TYPE_HANDLED;
>  	}
>  }
>  
> 


-- 
Pekka Paalanen
http://www.iki.fi/pq/

Re: [PATCH -tip 2/3] Tracing/ftrace: Adapt mmiotrace to the new type of print_line

[Frédéric Weisbecker]

2008/9/26 Pekka Paalanen <pq@iki.fi>:
> On Fri, 26 Sep 2008 17:44:07 +0200
> Frederic Weisbecker <fweisbec@gmail.com> wrote:
>
>> Adapt mmiotrace to the new print_line type.
>> By default, it refuses types it doesn't support.
>> But a tracer needs to print something to the seq anyway.
>> That's why it needs to warn about unsupported types.
>>
>> Pekka if you disagree with it, please don't hesitate to tell me.
>
> Yes, I do disagree about printing stuff that doesn't belong there.
> First, I doubt printing bogus text is a right way to fix the
> early pipe EOF problem. Second, if you really have to do that,
> do it so that it obeys the mmiotrace log format specification.
> (I'd recommend a MARK entry.) The spec is in
> Documentation/tracers/mmiotrace.txt.
>
> This is just a very quick comment, and I should look at your two
> patches better. I hope to do that during the weekend.
>
> Was there supposed to be a third patch, as the subject suggests?
>
> Thanks.
>
>


No problem, I have one other option: ignoring and sleep again before
receiving another entries.
I can do another patch this week end to implement that.
The third patch only concerns the boot tracer. It relays to other
output functions.

Just tell me if you agree with the ignoring...

Thanks !

Re: [PATCH -tip 2/3] Tracing/ftrace: Adapt mmiotrace to the new type of print_line

[Ingo Molnar]

* Frédéric Weisbecker <fweisbec@gmail.com> wrote:

> No problem, I have one other option: ignoring and sleep again before 
> receiving another entries. I can do another patch this week end to 
> implement that. The third patch only concerns the boot tracer. It 
> relays to other output functions.
> 
> Just tell me if you agree with the ignoring...

ok, i'll defer your patchset to until there's tentative agreement from 
Pekka, as this issue affects both the mmiotrace and fastboot tracers.

	Ingo

trace_pipe tentative fix (Re: [PATCH -tip 2/3] Tracing/ftrace: Adapt mmiotrace to the new type of print_line)

[Pekka Paalanen]

On Sat, 27 Sep 2008 14:17:47 +0200
"Frédéric Weisbecker" <fweisbec@gmail.com> wrote:

> 2008/9/26 Pekka Paalanen <pq@iki.fi>:
> > Yes, I do disagree about printing stuff that doesn't belong there.
> > First, I doubt printing bogus text is a right way to fix the
> > early pipe EOF problem. Second, if you really have to do that,
> > do it so that it obeys the mmiotrace log format specification.
> > (I'd recommend a MARK entry.) The spec is in
> > Documentation/tracers/mmiotrace.txt.
> 
> No problem, I have one other option: ignoring and sleep again before
> receiving another entries.
> I can do another patch this week end to implement that.
> The third patch only concerns the boot tracer. It relays to other
> output functions.
> 
> Just tell me if you agree with the ignoring...

If I understand you suggestion, it looks like the right thing to do.
Here is a tentative fix, which has not even been compile-tested.

Is it so that the problem is triggered by consuming a trace entry
which does not produce any output? If that entry is all there is
in the ring at a time of a read call, then the last call to
trace_seq_to_user() returns -EBUSY, because there is nothing to
copy to user. What I failed to understand when I wrote that
piece of code, is that returning 0 means EOF. The only cases
when we do want to return an EOF are near the
	while (trace_empty(iter)) {
loop.

Frederic, could you test the fix, and if it works, send it to Ingo?


Thanks.

---

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 6ada059..d85659a 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2616,6 +2616,7 @@ tracing_read_pipe(struct file *filp, char __user *ubuf,
                        goto out;
        }
 
+waitagain:
        while (trace_empty(iter)) {
 
                if ((filp->f_flags & O_NONBLOCK)) {
@@ -2749,8 +2750,13 @@ tracing_read_pipe(struct file *filp, char __user *ubuf,
        sret = trace_seq_to_user(&iter->seq, ubuf, cnt);
        if (iter->seq.readpos >= iter->seq.len)
                trace_seq_reset(&iter->seq);
+
+       /*
+        * If there was nothing to send to user, inspite of consuming trace
+        * entries, go back to wait for more entries.
+        */
        if (sret == -EBUSY)
-               sret = 0;
+               goto waitagain;
 
 out:
        mutex_unlock(&trace_types_lock);

Re: trace_pipe tentative fix

[Pekka Paalanen]

On Sun, 28 Sep 2008 20:12:59 +0300
Pekka Paalanen <pq@iki.fi> wrote:

> If I understand you suggestion, it looks like the right thing to do.
> Here is a tentative fix, which has not even been compile-tested.
> 
> Is it so that the problem is triggered by consuming a trace entry
> which does not produce any output? If that entry is all there is
> in the ring at a time of a read call, then the last call to
> trace_seq_to_user() returns -EBUSY, because there is nothing to
> copy to user. What I failed to understand when I wrote that
> piece of code, is that returning 0 means EOF. The only cases
> when we do want to return an EOF are near the
> 	while (trace_empty(iter)) {
> loop.
> 
> Frederic, could you test the fix, and if it works, send it to Ingo?

Whoops, sret was left in a bad state. Here's a new one.

---

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 6ada059..16b8a22 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2605,7 +2605,6 @@ tracing_read_pipe(struct file *filp, char __user *ubuf,
        sret = trace_seq_to_user(&iter->seq, ubuf, cnt);
        if (sret != -EBUSY)
                return sret;
-       sret = 0;
 
        trace_seq_reset(&iter->seq);
 
@@ -2616,6 +2615,8 @@ tracing_read_pipe(struct file *filp, char __user *ubuf,
                        goto out;
        }
 
+waitagain:
+       sret = 0;
        while (trace_empty(iter)) {
 
                if ((filp->f_flags & O_NONBLOCK)) {
@@ -2749,8 +2750,13 @@ tracing_read_pipe(struct file *filp, char __user *ubuf,
        sret = trace_seq_to_user(&iter->seq, ubuf, cnt);
        if (iter->seq.readpos >= iter->seq.len)
                trace_seq_reset(&iter->seq);
+
+       /*
+        * If there was nothing to send to user, inspite of consuming trace
+        * entries, go back to wait for more entries.
+        */
        if (sret == -EBUSY)
-               sret = 0;
+               goto waitagain;
 
 out:
        mutex_unlock(&trace_types_lock);

Re: trace_pipe tentative fix

[Frédéric Weisbecker]

2008/9/28 Pekka Paalanen <pq@iki.fi>:
> On Sun, 28 Sep 2008 20:12:59 +0300
> Pekka Paalanen <pq@iki.fi> wrote:
>
>> If I understand you suggestion, it looks like the right thing to do.
>> Here is a tentative fix, which has not even been compile-tested.
>>
>> Is it so that the problem is triggered by consuming a trace entry
>> which does not produce any output? If that entry is all there is
>> in the ring at a time of a read call, then the last call to
>> trace_seq_to_user() returns -EBUSY, because there is nothing to
>> copy to user. What I failed to understand when I wrote that
>> piece of code, is that returning 0 means EOF. The only cases
>> when we do want to return an EOF are near the
>>       while (trace_empty(iter)) {
>> loop.
>>
>> Frederic, could you test the fix, and if it works, send it to Ingo?
>
> Whoops, sret was left in a bad state. Here's a new one.
>
> ---
>
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 6ada059..16b8a22 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -2605,7 +2605,6 @@ tracing_read_pipe(struct file *filp, char __user *ubuf,
>        sret = trace_seq_to_user(&iter->seq, ubuf, cnt);
>        if (sret != -EBUSY)
>                return sret;
> -       sret = 0;
>
>        trace_seq_reset(&iter->seq);
>
> @@ -2616,6 +2615,8 @@ tracing_read_pipe(struct file *filp, char __user *ubuf,
>                        goto out;
>        }
>
> +waitagain:
> +       sret = 0;
>        while (trace_empty(iter)) {
>
>                if ((filp->f_flags & O_NONBLOCK)) {
> @@ -2749,8 +2750,13 @@ tracing_read_pipe(struct file *filp, char __user *ubuf,
>        sret = trace_seq_to_user(&iter->seq, ubuf, cnt);
>        if (iter->seq.readpos >= iter->seq.len)
>                trace_seq_reset(&iter->seq);
> +
> +       /*
> +        * If there was nothing to send to user, inspite of consuming trace
> +        * entries, go back to wait for more entries.
> +        */
>        if (sret == -EBUSY)
> -               sret = 0;
> +               goto waitagain;
>
>  out:
>        mutex_unlock(&trace_types_lock);
>

Tested! And, no problem as far as I can see :)
So I'm going to send the new patchset.

Thanks.

Re: trace_pipe tentative fix (Re: [PATCH -tip 2/3] Tracing/ftrace: Adapt mmiotrace to the new type of print_line)

[Frédéric Weisbecker]

2008/9/28 Pekka Paalanen <pq@iki.fi>:
> On Sat, 27 Sep 2008 14:17:47 +0200
> "Frédéric Weisbecker" <fweisbec@gmail.com> wrote:
>
>> 2008/9/26 Pekka Paalanen <pq@iki.fi>:
>> > Yes, I do disagree about printing stuff that doesn't belong there.
>> > First, I doubt printing bogus text is a right way to fix the
>> > early pipe EOF problem. Second, if you really have to do that,
>> > do it so that it obeys the mmiotrace log format specification.
>> > (I'd recommend a MARK entry.) The spec is in
>> > Documentation/tracers/mmiotrace.txt.
>>
>> No problem, I have one other option: ignoring and sleep again before
>> receiving another entries.
>> I can do another patch this week end to implement that.
>> The third patch only concerns the boot tracer. It relays to other
>> output functions.
>>
>> Just tell me if you agree with the ignoring...
>
> If I understand you suggestion, it looks like the right thing to do.
> Here is a tentative fix, which has not even been compile-tested.
>
> Is it so that the problem is triggered by consuming a trace entry
> which does not produce any output? If that entry is all there is
> in the ring at a time of a read call, then the last call to
> trace_seq_to_user() returns -EBUSY, because there is nothing to
> copy to user. What I failed to understand when I wrote that
> piece of code, is that returning 0 means EOF. The only cases
> when we do want to return an EOF are near the
>        while (trace_empty(iter)) {
> loop.
>
> Frederic, could you test the fix, and if it works, send it to Ingo?
>
>
> Thanks.
>
> ---
>
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 6ada059..d85659a 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -2616,6 +2616,7 @@ tracing_read_pipe(struct file *filp, char __user *ubuf,
>                        goto out;
>        }
>
> +waitagain:
>        while (trace_empty(iter)) {
>
>                if ((filp->f_flags & O_NONBLOCK)) {
> @@ -2749,8 +2750,13 @@ tracing_read_pipe(struct file *filp, char __user *ubuf,
>        sret = trace_seq_to_user(&iter->seq, ubuf, cnt);
>        if (iter->seq.readpos >= iter->seq.len)
>                trace_seq_reset(&iter->seq);
> +
> +       /*
> +        * If there was nothing to send to user, inspite of consuming trace
> +        * entries, go back to wait for more entries.
> +        */
>        if (sret == -EBUSY)
> -               sret = 0;
> +               goto waitagain;
>
>  out:
>        mutex_unlock(&trace_types_lock);
>

Ok I will test it this evening.

Re: [PATCH -tip 2/3] Tracing/ftrace: Adapt mmiotrace to the new type of print_line

[Pekka Paalanen]

On Fri, 26 Sep 2008 17:44:07 +0200
Frederic Weisbecker <fweisbec@gmail.com> wrote:

> Adapt mmiotrace to the new print_line type.
> By default, it refuses types it doesn't support.
> But a tracer needs to print something to the seq anyway. 
> That's why it needs to warn about unsupported types.
> 
> Pekka if you disagree with it, please don't hesitate to tell me.
> 
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>

Like with the first patch, this looks good except the last hunk.


> ---
> diff --git a/kernel/trace/trace_mmiotrace.c b/kernel/trace/trace_mmiotrace.c
> index 2bbb7dd..69480a5 100644
> --- a/kernel/trace/trace_mmiotrace.c
> +++ b/kernel/trace/trace_mmiotrace.c
> @@ -171,7 +171,7 @@ print_out:
>  	return (ret == -EBUSY) ? 0 : ret;
>  }
>  
> -static int mmio_print_rw(struct trace_iterator *iter)
> +static enum print_line_t mmio_print_rw(struct trace_iterator *iter)
>  {
>  	struct trace_entry *entry = iter->ent;
>  	struct mmiotrace_rw *rw	= &entry->field.mmiorw;
> @@ -209,11 +209,11 @@ static int mmio_print_rw(struct trace_iterator *iter)
>  		break;
>  	}
>  	if (ret)
> -		return 1;
> -	return 0;
> +		return TRACE_TYPE_HANDLED;
> +	return TRACE_TYPE_PARTIAL_LINE;
>  }
>  
> -static int mmio_print_map(struct trace_iterator *iter)
> +static enum print_line_t mmio_print_map(struct trace_iterator *iter)
>  {
>  	struct trace_entry *entry = iter->ent;
>  	struct mmiotrace_map *m	= &entry->field.mmiomap;
> @@ -221,7 +221,7 @@ static int mmio_print_map(struct trace_iterator *iter)
>  	unsigned long long t	= ns2usecs(entry->field.t);
>  	unsigned long usec_rem	= do_div(t, 1000000ULL);
>  	unsigned secs		= (unsigned long)t;
> -	int ret = 1;
> +	int ret;
>  
>  	switch (entry->field.mmiorw.opcode) {
>  	case MMIO_PROBE:
> @@ -241,11 +241,11 @@ static int mmio_print_map(struct trace_iterator *iter)
>  		break;
>  	}
>  	if (ret)
> -		return 1;
> -	return 0;
> +		return TRACE_TYPE_HANDLED;
> +	return TRACE_TYPE_PARTIAL_LINE;
>  }
>  
> -static int mmio_print_mark(struct trace_iterator *iter)
> +static enum print_line_t mmio_print_mark(struct trace_iterator *iter)
>  {
>  	struct trace_entry *entry = iter->ent;
>  	const char *msg		= entry->field.print.buf;
> @@ -258,16 +258,16 @@ static int mmio_print_mark(struct trace_iterator *iter)
>  	/* The trailing newline must be in the message. */
>  	ret = trace_seq_printf(s, "MARK %lu.%06lu %s", secs, usec_rem, msg);
>  	if (!ret)
> -		return 0;
> +		return TRACE_TYPE_PARTIAL_LINE;
>  
>  	if (entry->field.flags & TRACE_FLAG_CONT)
>  		trace_seq_print_cont(s, iter);
>  
> -	return 1;
> +	return TRACE_TYPE_HANDLED;
>  }
>  
>  /* return 0 to abort printing without consuming current entry in pipe mode */

This comment is now redundant.


> -static int mmio_print_line(struct trace_iterator *iter)
> +static enum print_line_t mmio_print_line(struct trace_iterator *iter)
>  {
>  	switch (iter->ent->type) {
>  	case TRACE_MMIO_RW:
> @@ -277,7 +277,9 @@ static int mmio_print_line(struct trace_iterator *iter)
>  	case TRACE_PRINT:
>  		return mmio_print_mark(iter);
>  	default:
> -		return 0; /* ignore unknown entries whithout consuming */
> +		/* ignore unknown entries */
> +		trace_seq_printf(&iter->seq, "Unknown trace type\n");

Leave this printf out.

> +		return TRACE_TYPE_HANDLED;
>  	}
>  }

Otherwise, very good.

As Ingo reverted the two earlier patches, you need to rebase these two.
I'd like to see these in upstream. Let's handle the pipe closing bug
in a different patch.


Thanks.

-- 
Pekka Paalanen
http://www.iki.fi/pq/

Re: [PATCH -tip 2/3] Tracing/ftrace: Adapt mmiotrace to the new type of print_line

[Ingo Molnar]

* Pekka Paalanen <pq@iki.fi> wrote:

> > +		return TRACE_TYPE_HANDLED;
> >  	}
> >  }
> 
> Otherwise, very good.
> 
> As Ingo reverted the two earlier patches, you need to rebase these 
> two. I'd like to see these in upstream. Let's handle the pipe closing 
> bug in a different patch.

good. Frederic, please just resubmit all fixes you have against 
tip/master, with Pekka's Acked-by added. You can submit Pekka's patch as 
well, so that it's a coherent series. The customary way for that is to 
add in a From line as the first line of the changelog:

 From: Pekka Paalanen <pq@iki.fi>

and add your signoff to the end of the changelog. This way Git will show 
Pekka as the author and the path of the patch is well understood.

Thanks,

	Ingo

Re: [PATCH -tip 2/3] Tracing/ftrace: Adapt mmiotrace to the new type of print_line

[Frédéric Weisbecker]

2008/9/29 Ingo Molnar <mingo@elte.hu>:
>
> * Pekka Paalanen <pq@iki.fi> wrote:
>
>> > +           return TRACE_TYPE_HANDLED;
>> >     }
>> >  }
>>
>> Otherwise, very good.
>>
>> As Ingo reverted the two earlier patches, you need to rebase these
>> two. I'd like to see these in upstream. Let's handle the pipe closing
>> bug in a different patch.
>
> good. Frederic, please just resubmit all fixes you have against
> tip/master, with Pekka's Acked-by added. You can submit Pekka's patch as
> well, so that it's a coherent series. The customary way for that is to
> add in a From line as the first line of the changelog:
>
>  From: Pekka Paalanen <pq@iki.fi>
>
> and add your signoff to the end of the changelog. This way Git will show
> Pekka as the author and the path of the patch is well understood.
>

Ok so... I resend my patches (without changes?) but I adapt them to
latest -tip master?
And I add the pekka's patches (whit appropriate "from" line) with them?

Sorry I 'm a bit lost....

Re: [PATCH -tip 2/3] Tracing/ftrace: Adapt mmiotrace to the new type of print_line

[Ingo Molnar]

* Frédéric Weisbecker <fweisbec@gmail.com> wrote:

> 2008/9/29 Ingo Molnar <mingo@elte.hu>:
> >
> > * Pekka Paalanen <pq@iki.fi> wrote:
> >
> >> > +           return TRACE_TYPE_HANDLED;
> >> >     }
> >> >  }
> >>
> >> Otherwise, very good.
> >>
> >> As Ingo reverted the two earlier patches, you need to rebase these
> >> two. I'd like to see these in upstream. Let's handle the pipe closing
> >> bug in a different patch.
> >
> > good. Frederic, please just resubmit all fixes you have against
> > tip/master, with Pekka's Acked-by added. You can submit Pekka's patch as
> > well, so that it's a coherent series. The customary way for that is to
> > add in a From line as the first line of the changelog:
> >
> >  From: Pekka Paalanen <pq@iki.fi>
> >
> > and add your signoff to the end of the changelog. This way Git will show
> > Pekka as the author and the path of the patch is well understood.
> >
> 
> Ok so... I resend my patches (without changes?) but I adapt them to
> latest -tip master?

yeah - resend any bits not yet in tip/master, and also add Pekka's 
Acked-by line to them where he said they are OK.

> And I add the pekka's patches (whit appropriate "from" line) with 
> them?

Correct. Also mark it v2 perhaps in the announcement.

when there's NACKs and discussion it's (much) easier to pick up a new 
series than picking out the bits from the discussion. That way the 
resubmission might be redundant in a large part, but it's a summary of 
your discussion with Pekka and the result of a consensus. I.e. not 
redundant in terms of a workflow step.

	Ingo