On Fri, 26 Sep 2008 17:44:07 +0200

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?



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

[ message continues ]

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 !
--

[ message continues ]

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
--

[ message continues ]

On Sat, 27 Sep 2008 14:17:47 +0200

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);
--

[ message continues ]

On Sun, 28 Sep 2008 20:12:59 +0300

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);
--

[ message continues ]

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

Thanks.
--

[ message continues ]

Ok I will test it this evening.
--

[ message continues ]

On Fri, 26 Sep 2008 17:44:07 +0200




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/
--

[ message continues ]

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
--

[ message continues ]

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....
--

[ message continues ]

yeah - resend any bits not yet in tip/master, and also add Pekka's 

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
--

[ message continues ]