linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2][next] printk: ringbuffer: support dataless records
@ 2020-07-21 13:25 John Ogness
  2020-07-27  8:53 ` Sergey Senozhatsky
  2020-08-04 10:14 ` Petr Mladek
  0 siblings, 2 replies; 3+ messages in thread
From: John Ogness @ 2020-07-21 13:25 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Sergey Senozhatsky, Sergey Senozhatsky, Steven Rostedt,
	Linus Torvalds, Greg Kroah-Hartman, Peter Zijlstra,
	Thomas Gleixner, Marco Elver, kexec, linux-kernel

With commit ("printk: use the lockless ringbuffer"), printk()
started silently dropping messages without text because such
records are not supported by the new printk ringbuffer.

Add support for such records.

Currently dataless records are denoted by INVALID_LPOS in order
to recognize failed prb_reserve() calls. Change the ringbuffer
to instead use two different identifiers (FAILED_LPOS and
NO_LPOS) to distinguish between failed prb_reserve() records and
successful dataless records, respectively.

Fixes: ("printk: use the lockless ringbuffer")
Fixes: https://lkml.kernel.org/r/20200718121053.GA691245@elver.google.com
Reported-by: Marco Elver <elver@google.com>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 based on next-20200721

 chages since v1:
 - Instead of handling empty text messages as special case errors,
   allow such messages to be handled as any other valid messages.
   This also allows the empty text message to be counted as a line.

 kernel/printk/printk_ringbuffer.c | 72 +++++++++++++++----------------
 kernel/printk/printk_ringbuffer.h | 15 ++++---
 2 files changed, 43 insertions(+), 44 deletions(-)

diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
index 7355ca99e852..0659b50872b5 100644
--- a/kernel/printk/printk_ringbuffer.c
+++ b/kernel/printk/printk_ringbuffer.c
@@ -264,6 +264,9 @@
 /* Determine how many times the data array has wrapped. */
 #define DATA_WRAPS(data_ring, lpos)	((lpos) >> (data_ring)->size_bits)
 
+/* Determine if a logical position refers to a data-less block. */
+#define LPOS_DATALESS(lpos)		((lpos) & 1UL)
+
 /* Get the logical position at index 0 of the current wrap. */
 #define DATA_THIS_WRAP_START_LPOS(data_ring, lpos) \
 ((lpos) & ~DATA_SIZE_MASK(data_ring))
@@ -320,21 +323,13 @@ static unsigned int to_blk_size(unsigned int size)
  * block does not exceed the maximum possible size that could fit within the
  * ringbuffer. This function provides that basic size check so that the
  * assumption is safe.
- *
- * Writers are also not allowed to write 0-sized (data-less) records. Such
- * records are used only internally by the ringbuffer.
  */
 static bool data_check_size(struct prb_data_ring *data_ring, unsigned int size)
 {
 	struct prb_data_block *db = NULL;
 
-	/*
-	 * Writers are not allowed to write data-less records. Such records
-	 * are used only internally by the ringbuffer to denote records where
-	 * their data failed to allocate or have been lost.
-	 */
 	if (size == 0)
-		return false;
+		return true;
 
 	/*
 	 * Ensure the alignment padded size could possibly fit in the data
@@ -568,8 +563,8 @@ static bool data_push_tail(struct printk_ringbuffer *rb,
 	unsigned long tail_lpos;
 	unsigned long next_lpos;
 
-	/* If @lpos is not valid, there is nothing to do. */
-	if (lpos == INVALID_LPOS)
+	/* If @lpos is from a data-less block, there is nothing to do. */
+	if (LPOS_DATALESS(lpos))
 		return true;
 
 	/*
@@ -962,8 +957,8 @@ static char *data_alloc(struct printk_ringbuffer *rb,
 
 	if (size == 0) {
 		/* Specify a data-less block. */
-		blk_lpos->begin = INVALID_LPOS;
-		blk_lpos->next = INVALID_LPOS;
+		blk_lpos->begin = NO_LPOS;
+		blk_lpos->next = NO_LPOS;
 		return NULL;
 	}
 
@@ -976,8 +971,8 @@ static char *data_alloc(struct printk_ringbuffer *rb,
 
 		if (!data_push_tail(rb, data_ring, next_lpos - DATA_SIZE(data_ring))) {
 			/* Failed to allocate, specify a data-less block. */
-			blk_lpos->begin = INVALID_LPOS;
-			blk_lpos->next = INVALID_LPOS;
+			blk_lpos->begin = FAILED_LPOS;
+			blk_lpos->next = FAILED_LPOS;
 			return NULL;
 		}
 
@@ -1025,6 +1020,10 @@ static char *data_alloc(struct printk_ringbuffer *rb,
 static unsigned int space_used(struct prb_data_ring *data_ring,
 			       struct prb_data_blk_lpos *blk_lpos)
 {
+	/* Data-less blocks take no space. */
+	if (LPOS_DATALESS(blk_lpos->begin))
+		return 0;
+
 	if (DATA_WRAPS(data_ring, blk_lpos->begin) == DATA_WRAPS(data_ring, blk_lpos->next)) {
 		/* Data block does not wrap. */
 		return (DATA_INDEX(data_ring, blk_lpos->next) -
@@ -1080,11 +1079,8 @@ bool prb_reserve(struct prb_reserved_entry *e, struct printk_ringbuffer *rb,
 	if (!data_check_size(&rb->text_data_ring, r->text_buf_size))
 		goto fail;
 
-	/* Records are allowed to not have dictionaries. */
-	if (r->dict_buf_size) {
-		if (!data_check_size(&rb->dict_data_ring, r->dict_buf_size))
-			goto fail;
-	}
+	if (!data_check_size(&rb->dict_data_ring, r->dict_buf_size))
+		goto fail;
 
 	/*
 	 * Descriptors in the reserved state act as blockers to all further
@@ -1205,15 +1201,18 @@ void prb_commit(struct prb_reserved_entry *e)
  * values to possibly detect bugs in the writer code. A WARN_ON_ONCE() is
  * triggered if an internal error is detected.
  */
-static char *get_data(struct prb_data_ring *data_ring,
-		      struct prb_data_blk_lpos *blk_lpos,
-		      unsigned int *data_size)
+static const char *get_data(struct prb_data_ring *data_ring,
+			    struct prb_data_blk_lpos *blk_lpos,
+			    unsigned int *data_size)
 {
 	struct prb_data_block *db;
 
 	/* Data-less data block description. */
-	if (blk_lpos->begin == INVALID_LPOS &&
-	    blk_lpos->next == INVALID_LPOS) {
+	if (LPOS_DATALESS(blk_lpos->begin) && LPOS_DATALESS(blk_lpos->next)) {
+		if (blk_lpos->begin == NO_LPOS && blk_lpos->next == NO_LPOS) {
+			*data_size = 0;
+			return "";
+		}
 		return NULL;
 	}
 
@@ -1256,11 +1255,11 @@ static char *get_data(struct prb_data_ring *data_ring,
  * (even if @text_size is 0). Each '\n' processed is counted as an additional
  * line.
  */
-static unsigned int count_lines(char *text, unsigned int text_size)
+static unsigned int count_lines(const char *text, unsigned int text_size)
 {
 	unsigned int next_size = text_size;
 	unsigned int line_count = 1;
-	char *next = text;
+	const char *next = text;
 
 	while (next_size) {
 		next = memchr(next, '\n', next_size);
@@ -1287,7 +1286,7 @@ static bool copy_data(struct prb_data_ring *data_ring,
 		      unsigned int buf_size, unsigned int *line_count)
 {
 	unsigned int data_size;
-	char *data;
+	const char *data;
 
 	/* Caller might not want any data. */
 	if ((!buf || !buf_size) && !line_count)
@@ -1317,8 +1316,7 @@ static bool copy_data(struct prb_data_ring *data_ring,
 
 	data_size = min_t(u16, buf_size, len);
 
-	if (!WARN_ON_ONCE(!data_size))
-		memcpy(&buf[0], data, data_size); /* LMM(copy_data:A) */
+	memcpy(&buf[0], data, data_size); /* LMM(copy_data:A) */
 	return true;
 }
 
@@ -1355,11 +1353,11 @@ static int desc_read_committed_seq(struct prb_desc_ring *desc_ring,
 
 	/*
 	 * A descriptor in the reusable state may no longer have its data
-	 * available; report it as a data-less record. Or the record may
-	 * actually be a data-less record.
+	 * available; report it as existing but with lost data. Or the record
+	 * may actually be a record with lost data.
 	 */
 	if (d_state == desc_reusable ||
-	    (blk_lpos->begin == INVALID_LPOS && blk_lpos->next == INVALID_LPOS)) {
+	    (blk_lpos->begin == FAILED_LPOS && blk_lpos->next == FAILED_LPOS)) {
 		return -ENOENT;
 	}
 
@@ -1659,10 +1657,10 @@ void prb_init(struct printk_ringbuffer *rb,
 
 	descs[_DESCS_COUNT(descbits) - 1].info.seq = 0;
 	atomic_long_set(&(descs[_DESCS_COUNT(descbits) - 1].state_var), DESC0_SV(descbits));
-	descs[_DESCS_COUNT(descbits) - 1].text_blk_lpos.begin = INVALID_LPOS;
-	descs[_DESCS_COUNT(descbits) - 1].text_blk_lpos.next = INVALID_LPOS;
-	descs[_DESCS_COUNT(descbits) - 1].dict_blk_lpos.begin = INVALID_LPOS;
-	descs[_DESCS_COUNT(descbits) - 1].dict_blk_lpos.next = INVALID_LPOS;
+	descs[_DESCS_COUNT(descbits) - 1].text_blk_lpos.begin = FAILED_LPOS;
+	descs[_DESCS_COUNT(descbits) - 1].text_blk_lpos.next = FAILED_LPOS;
+	descs[_DESCS_COUNT(descbits) - 1].dict_blk_lpos.begin = FAILED_LPOS;
+	descs[_DESCS_COUNT(descbits) - 1].dict_blk_lpos.next = FAILED_LPOS;
 }
 
 /**
diff --git a/kernel/printk/printk_ringbuffer.h b/kernel/printk/printk_ringbuffer.h
index 3e46a7423c13..e6302da041f9 100644
--- a/kernel/printk/printk_ringbuffer.h
+++ b/kernel/printk/printk_ringbuffer.h
@@ -120,12 +120,13 @@ struct prb_reserved_entry {
 #define DESC_FLAGS_MASK			(DESC_COMMITTED_MASK | DESC_REUSE_MASK)
 #define DESC_ID_MASK			(~DESC_FLAGS_MASK)
 #define DESC_ID(sv)			((sv) & DESC_ID_MASK)
-#define INVALID_LPOS			1
+#define FAILED_LPOS			0x1
+#define NO_LPOS				0x3
 
-#define INVALID_BLK_LPOS	\
+#define FAILED_BLK_LPOS	\
 {				\
-	.begin	= INVALID_LPOS,	\
-	.next	= INVALID_LPOS,	\
+	.begin	= FAILED_LPOS,	\
+	.next	= FAILED_LPOS,	\
 }
 
 /*
@@ -147,7 +148,7 @@ struct prb_reserved_entry {
  *
  * To satisfy Req1, the tail initially points to a descriptor that is
  * minimally initialized (having no data block, i.e. data-less with the
- * data block's lpos @begin and @next values set to INVALID_LPOS).
+ * data block's lpos @begin and @next values set to FAILED_LPOS).
  *
  * To satisfy Req2, the initial tail descriptor is initialized to the
  * reusable state. Readers recognize reusable descriptors as existing
@@ -242,8 +243,8 @@ static struct prb_desc _##name##_descs[_DESCS_COUNT(descbits)] = {				\
 		/* reusable */									\
 		.state_var	= ATOMIC_INIT(DESC0_SV(descbits)),				\
 		/* no associated data block */							\
-		.text_blk_lpos	= INVALID_BLK_LPOS,						\
-		.dict_blk_lpos	= INVALID_BLK_LPOS,						\
+		.text_blk_lpos	= FAILED_BLK_LPOS,						\
+		.dict_blk_lpos	= FAILED_BLK_LPOS,						\
 	},											\
 };												\
 static struct printk_ringbuffer name = {							\
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH v2][next] printk: ringbuffer: support dataless records
  2020-07-21 13:25 [PATCH v2][next] printk: ringbuffer: support dataless records John Ogness
@ 2020-07-27  8:53 ` Sergey Senozhatsky
  2020-08-04 10:14 ` Petr Mladek
  1 sibling, 0 replies; 3+ messages in thread
From: Sergey Senozhatsky @ 2020-07-27  8:53 UTC (permalink / raw)
  To: John Ogness
  Cc: Petr Mladek, Sergey Senozhatsky, Sergey Senozhatsky,
	Steven Rostedt, Linus Torvalds, Greg Kroah-Hartman,
	Peter Zijlstra, Thomas Gleixner, Marco Elver, kexec,
	linux-kernel

On (20/07/21 15:31), John Ogness wrote:
> With commit ("printk: use the lockless ringbuffer"), printk()
> started silently dropping messages without text because such
> records are not supported by the new printk ringbuffer.
> 
> Add support for such records.
> 
> Currently dataless records are denoted by INVALID_LPOS in order
> to recognize failed prb_reserve() calls. Change the ringbuffer
> to instead use two different identifiers (FAILED_LPOS and
> NO_LPOS) to distinguish between failed prb_reserve() records and
> successful dataless records, respectively.
> 
> Fixes: ("printk: use the lockless ringbuffer")
> Fixes: https://lkml.kernel.org/r/20200718121053.GA691245@elver.google.com
> Reported-by: Marco Elver <elver@google.com>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Applied to printk-rework, thanks.

	-ss

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2][next] printk: ringbuffer: support dataless records
  2020-07-21 13:25 [PATCH v2][next] printk: ringbuffer: support dataless records John Ogness
  2020-07-27  8:53 ` Sergey Senozhatsky
@ 2020-08-04 10:14 ` Petr Mladek
  1 sibling, 0 replies; 3+ messages in thread
From: Petr Mladek @ 2020-08-04 10:14 UTC (permalink / raw)
  To: John Ogness
  Cc: Sergey Senozhatsky, Sergey Senozhatsky, Steven Rostedt,
	Linus Torvalds, Greg Kroah-Hartman, Peter Zijlstra,
	Thomas Gleixner, Marco Elver, kexec, linux-kernel

On Tue 2020-07-21 15:31:28, John Ogness wrote:
> With commit ("printk: use the lockless ringbuffer"), printk()
> started silently dropping messages without text because such
> records are not supported by the new printk ringbuffer.
> 
> Add support for such records.
> 
> Currently dataless records are denoted by INVALID_LPOS in order
> to recognize failed prb_reserve() calls. Change the ringbuffer
> to instead use two different identifiers (FAILED_LPOS and
> NO_LPOS) to distinguish between failed prb_reserve() records and
> successful dataless records, respectively.
> 
> diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
> index 7355ca99e852..0659b50872b5 100644
> --- a/kernel/printk/printk_ringbuffer.c
> +++ b/kernel/printk/printk_ringbuffer.c
>  static bool data_check_size(struct prb_data_ring *data_ring, unsigned int size)
>  {
>  	struct prb_data_block *db = NULL;
>  
> -	/*
> -	 * Writers are not allowed to write data-less records. Such records
> -	 * are used only internally by the ringbuffer to denote records where
> -	 * their data failed to allocate or have been lost.
> -	 */
>  	if (size == 0)
> -		return false;
> +		return true;

Nit: This might deserve a comment why size == 0 is handled
     a special way.specially. I think about something like:

	/*
	 * Empty data blocks are handled by special lpos values in
	 * the record descriptor. No space is needed in the data ring.
	 */

or simply

	/* Data-less records take no space in the data ring. */

>  	/*
>  	 * Ensure the alignment padded size could possibly fit in the data

> @@ -1025,6 +1020,10 @@ static char *data_alloc(struct printk_ringbuffer *rb,
>  static unsigned int space_used(struct prb_data_ring *data_ring,
>  			       struct prb_data_blk_lpos *blk_lpos)
>  {
> +	/* Data-less blocks take no space. */
> +	if (LPOS_DATALESS(blk_lpos->begin))
> +		return 0;

Nit: It seems that all the other locations check also blk_lpos->next,
     for example, get_data() does:

	if (LPOS_DATALESS(blk_lpos->begin) && LPOS_DATALESS(blk_lpos->next)) {


     Both approaches are error prone. I would either simplify the
     other locations and check only lpos->begin. But better might
     be to be on the safe side do a paranoid check, like:

bool is_dataless(struct prb_data_blk_lpos *blk_lpos)
{
	if (LPOS_DATALESS(blk_lpos->begin) || LPOS_DATALESS(blk_lpos->next)) {
		WARN_ON_ONCE(blk_lpos->begin != blk_lpos->next);
		return true;
	}

	return false;
}

> +
>  	if (DATA_WRAPS(data_ring, blk_lpos->begin) == DATA_WRAPS(data_ring, blk_lpos->next)) {
>  		/* Data block does not wrap. */
>  		return (DATA_INDEX(data_ring, blk_lpos->next) -

Anyway, the patch looks fine. It is already pushed in
printk/linux.git. So, if you agree with my nits, we should
solve them with separate patches on top of the existing ones.

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2020-08-04 10:14 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-21 13:25 [PATCH v2][next] printk: ringbuffer: support dataless records John Ogness
2020-07-27  8:53 ` Sergey Senozhatsky
2020-08-04 10:14 ` Petr Mladek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).