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/ --
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 ! --
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 --
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);
--
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);
--
Tested! And, no problem as far as I can see :) So I'm going to send the new patchset. Thanks. --
Ok I will test it this evening. --
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/ --
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 --
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.... --
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 --
