On Tue 2019-02-12 15:29:44, John Ogness wrote:
> Add a blocking read function for readers. An irq_work function is
> used to signal the wait queue so that write notification can
> be triggered from any context.
I would be more precise what exacly is problematic in which context.
Something like:
An irq_work function is used because wake_up() cannot be called safely
from NMI and scheduler context.
> Signed-off-by: John Ogness <john.ogness@xxxxxxxxxxxxx>
> ---
> include/linux/printk_ringbuffer.h | 20 ++++++++++++++++
> lib/printk_ringbuffer.c | 49 +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 69 insertions(+)
>
> diff --git a/include/linux/printk_ringbuffer.h b/include/linux/printk_ringbuffer.h
> index 5fdaf632c111..106f20ef8b4d 100644
> --- a/include/linux/printk_ringbuffer.h
> +++ b/include/linux/printk_ringbuffer.h
> @@ -2,8 +2,10 @@
> #ifndef _LINUX_PRINTK_RINGBUFFER_H
> #define _LINUX_PRINTK_RINGBUFFER_H
>
> +#include <linux/irq_work.h>
> #include <linux/atomic.h>
> #include <linux/percpu.h>
> +#include <linux/wait.h>
>
> struct prb_cpulock {
> atomic_t owner;
> @@ -22,6 +24,10 @@ struct printk_ringbuffer {
>
> struct prb_cpulock *cpulock;
> atomic_t ctx;
> +
> + struct wait_queue_head *wq;
> + atomic_long_t wq_counter;
> + struct irq_work *wq_work;
> };
>
> struct prb_entry {
> @@ -59,6 +65,15 @@ struct prb_iterator {
> #define DECLARE_STATIC_PRINTKRB(name, szbits, cpulockptr) \
> static char _##name##_buffer[1 << (szbits)] \
> __aligned(__alignof__(long)); \
> +static DECLARE_WAIT_QUEUE_HEAD(_##name##_wait); \
> +static void _##name##_wake_work_func(struct irq_work *irq_work) \
> +{ \
> + wake_up_interruptible_all(&_##name##_wait); \
> +} \
All ring buffers might share the same generic function, something like:
void prb_wake_readers_work_func(struct irq_work *irq_work)
{
struct printk_ringbuffer *rb;
rb = container_of(irq_work, struct printk_ring_buffer, wq_work);
wake_up_interruptible_all(rb->wq); \
}
> +static struct irq_work _##name##_wake_work = { \
> + .func = _##name##_wake_work_func, \
> + .flags = IRQ_WORK_LAZY, \
> +}; \
> static struct printk_ringbuffer name = { \
> .buffer = &_##name##_buffer[0], \
> .size_bits = szbits, \
> diff --git a/lib/printk_ringbuffer.c b/lib/printk_ringbuffer.c
> index 1d1e886a0966..c2ddf4cb9f92 100644
> --- a/lib/printk_ringbuffer.c
> +++ b/lib/printk_ringbuffer.c
> @@ -185,6 +188,12 @@ void prb_commit(struct prb_handle *h)
> }
>
> prb_unlock(rb->cpulock, h->cpu);
> +
> + if (changed) {
> + atomic_long_inc(&rb->wq_counter);
> + if (wq_has_sleeper(rb->wq))
> + irq_work_queue(rb->wq_work);
> + }
> }
>
> /*
> @@ -437,3 +446,43 @@ int prb_iter_next(struct prb_iterator *iter, char *buf, int size, u64 *seq)
>
> return 1;
> }
> +
> +/*
> + * prb_iter_wait_next: Advance to the next record, blocking if none available.
> + * @iter: Iterator tracking the current position.
> + * @buf: A buffer to store the data of the next record. May be NULL.
> + * @size: The size of @buf. (Ignored if @buf is NULL.)
> + * @seq: The sequence number of the next record. May be NULL.
> + *
> + * If a next record is already available, this function works like
> + * prb_iter_next(). Otherwise block interruptible until a next record is
> + * available.
> + *
> + * When a next record is available, @iter is advanced and (if specified)
> + * the data and/or sequence number of that record are provided.
> + *
> + * This function might sleep.
> + *
> + * Returns 1 if @iter was advanced, -EINVAL if @iter is now invalid, or
> + * -ERESTARTSYS if interrupted by a signal.
> + */
> +int prb_iter_wait_next(struct prb_iterator *iter, char *buf, int size, u64 *seq)
> +{
> + unsigned long last_seen;
> + int ret;
> +
> + for (;;) {
> + last_seen = atomic_long_read(&iter->rb->wq_counter);
> +
> + ret = prb_iter_next(iter, buf, size, seq);
> + if (ret != 0)
> + break;
> +
> + ret = wait_event_interruptible(*iter->rb->wq,
> + last_seen != atomic_long_read(&iter->rb->wq_counter));
Do we really need yet another counter here?
I think that rb->seq might do the same job. Or if there is problem
with atomicity then rb->head might work as well. Or do I miss
anything?
Best Regards,
Petr
> + if (ret < 0)
> + break;
> + }
> +
> + return ret;
> +}
> --
> 2.11.0
>
