Re: [PATCH 4/6] tracing: Fix waking up tracing readers

2024-03-08 Thread Steven Rostedt
On Fri, 08 Mar 2024 13:38:20 -0500
Steven Rostedt  wrote:

> +static DEFINE_MUTEX(wait_mutex);
> +
> +static bool wait_woken_prepare(struct trace_iterator *iter, int *wait_index)
> +{
> + bool woken = false;
> +
> + mutex_lock(_mutex);
> + if (iter->waking)
> + woken = true;
> + *wait_index = iter->wait_index;
> + mutex_unlock(_mutex);
> +
> + return woken;
> +}

The last patch adds this code after a prepare_to_wait(), which triggered
the warning:

  do not call blocking ops when !TASK_RUNNING; state=1 set at 
[<797e3e20>] prepare_to_wait+0x48/0xf0

Which is correct. The prepare_to_wait() set task state to
TASK_INTERRUPTIBLE, so I can not call a mutex after that.

I'll send a v2 where I switch this over to spin locks.

-- Steve


> +
> +static bool wait_woken_check(struct trace_iterator *iter, int *wait_index)
> +{
> + bool woken = false;
> +
> + mutex_lock(_mutex);
> + if (iter->waking || *wait_index != iter->wait_index)
> + woken = true;
> + mutex_unlock(_mutex);
> +
> + return woken;
> +}
> +
> +static void wait_woken_set(struct trace_iterator *iter)
> +{
> + mutex_lock(_mutex);
> + iter->waking++;
> + iter->wait_index++;
> + mutex_unlock(_mutex);
> +}
> +
> +static void wait_woken_clear(struct trace_iterator *iter)
> +{
> + mutex_lock(_mutex);
> + iter->waking--;
> + mutex_unlock(_mutex);
> +}
> +



[PATCH 4/6] tracing: Fix waking up tracing readers

2024-03-08 Thread Steven Rostedt
From: "Steven Rostedt (Google)" 

When the tracing_pipe_raw file is closed, if there are readers still
blocked on it, they need to be woken up. Currently a wait_index is used.
When the readers need to be woken, the index is updated and they are all
woken up.

But there is a race where a new reader could be coming in just as the file
is being closed, and it could still block if the wake up happens just
before the reader enters the wait.

Add another field called "waking" and wrap both the waking and wait_index
around a new wait_mutex to synchronize them.

When a reader comes in, it will save the current wait_index, but if waking
is set, then it will not block no matter what wait_index is.

After it wakes from the wait, if either the waking is set or the
wait_index is not the same as what it read before, then it will not block.

The waker will set waking and increment the wait_index. For the .flush()
function, it will not clear waking so that all new readers must not block.

There's an ioctl() that kicks all current waiters, but does not care about
new waiters. It will set the waking count back to what it was when it came
in.

There's still a race with the wait_on_pipe() with respect to the
ring_buffer_wait(), but that will be dealt with separately.

Link: 
https://lore.kernel.org/all/CAHk-=whs5MdtNjzFkTyaUy=vHi=qwwgpi0jgte6oyuymnsr...@mail.gmail.com/

Cc: sta...@vger.kernel.org
Fixes: f3ddb74ad0790 ("tracing: Wake up ring buffer waiters on closing of the 
file")
Signed-off-by: Steven Rostedt (Google) 
---
 include/linux/trace_events.h |   3 +-
 kernel/trace/trace.c | 101 +--
 2 files changed, 86 insertions(+), 18 deletions(-)

diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index d68ff9b1247f..adf8e163a7be 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -103,7 +103,8 @@ struct trace_iterator {
unsigned inttemp_size;
char*fmt;   /* modified format holder */
unsigned intfmt_size;
-   longwait_index;
+   int wait_index;
+   int waking; /* set by a waker */
 
/* trace_seq for __print_flags() and __print_symbolic() etc. */
struct trace_seqtmp_seq;
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index c9c898307348..4e8f6cdeafd5 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1955,6 +1955,65 @@ update_max_tr_single(struct trace_array *tr, struct 
task_struct *tsk, int cpu)
 
 #endif /* CONFIG_TRACER_MAX_TRACE */
 
+/*
+ * In order to wake up readers and have them return back to user space,
+ * the iterator has two counters:
+ *
+ *  wait_index - always increases every time a waker wakes up the readers.
+ *  waking - Set by the waker when waking and cleared afterward.
+ *
+ * Both are protected together with the wait_mutex.
+ * When waking, the lock is taken and both indexes are incremented.
+ * The reader will first prepare the wait by taking the lock,
+ * if waking is set, it will sleep regardless of what wait_index is.
+ * Then after it sleeps it checks if wait_index has been updated
+ * and if it has, it will not sleep again.
+ *
+ * Note, if wait_woken_clear() is not called, then all new readers
+ * will not sleep (this happens in closing the file).
+ */
+static DEFINE_MUTEX(wait_mutex);
+
+static bool wait_woken_prepare(struct trace_iterator *iter, int *wait_index)
+{
+   bool woken = false;
+
+   mutex_lock(_mutex);
+   if (iter->waking)
+   woken = true;
+   *wait_index = iter->wait_index;
+   mutex_unlock(_mutex);
+
+   return woken;
+}
+
+static bool wait_woken_check(struct trace_iterator *iter, int *wait_index)
+{
+   bool woken = false;
+
+   mutex_lock(_mutex);
+   if (iter->waking || *wait_index != iter->wait_index)
+   woken = true;
+   mutex_unlock(_mutex);
+
+   return woken;
+}
+
+static void wait_woken_set(struct trace_iterator *iter)
+{
+   mutex_lock(_mutex);
+   iter->waking++;
+   iter->wait_index++;
+   mutex_unlock(_mutex);
+}
+
+static void wait_woken_clear(struct trace_iterator *iter)
+{
+   mutex_lock(_mutex);
+   iter->waking--;
+   mutex_unlock(_mutex);
+}
+
 static int wait_on_pipe(struct trace_iterator *iter, int full)
 {
int ret;
@@ -8312,9 +8371,11 @@ tracing_buffers_read(struct file *filp, char __user 
*ubuf,
struct ftrace_buffer_info *info = filp->private_data;
struct trace_iterator *iter = >iter;
void *trace_data;
+   int wait_index;
int page_size;
ssize_t ret = 0;
ssize_t size;
+   bool woken;
 
if (!count)
return 0;
@@ -8353,6 +8414,7 @@ tracing_buffers_read(struct file *filp, char __user *ubuf,
if (info->read < page_size)
goto read;
 
+   woken = wait_woken_prepare(iter, _index);