printk_ringbuffer: Avoid needless read of prb_desc

[PATCH] printk_ringbuffer: Avoid needless read of prb_desc

Posted by Wang Wensheng 2 days, 9 hours ago

Since commit f244b4dc53e5 ("printk: ringbuffer: Improve prb_next_seq()
performance"), desc_read() allows @desc_out to be NULL. We can drop the
local `desc` variable in desc_reopen_last() and prb_first_seq(). Also
update the comment for desc_read().

Signed-off-by: Wang Wensheng <wsw9603@163.com>
---
 kernel/printk/printk_ringbuffer.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
index 85c0c854b3ce..607666d37455 100644
--- a/kernel/printk/printk_ringbuffer.c
+++ b/kernel/printk/printk_ringbuffer.c
@@ -443,8 +443,8 @@ static enum desc_state get_desc_state(unsigned long id,
  * descriptor is in an inconsistent state (miss or reserved), the caller can
  * only expect the descriptor's @state_var field to be valid.
  *
- * The sequence number and caller_id can be optionally retrieved. Like all
- * non-state_var data, they are only valid if the descriptor is in a
+ * The sequence number, caller_id, and desc can be optionally retrieved. Like
+ * all non-state_var data, they are only valid if the descriptor is in a
  * consistent state.
  */
 static enum desc_state desc_read(struct prb_desc_ring *desc_ring,
@@ -1335,7 +1335,6 @@ static struct prb_desc *desc_reopen_last(struct prb_desc_ring *desc_ring,
 {
 	unsigned long prev_state_val;
 	enum desc_state d_state;
-	struct prb_desc desc;
 	struct prb_desc *d;
 	unsigned long id;
 	u32 cid;
@@ -1346,7 +1345,7 @@ static struct prb_desc *desc_reopen_last(struct prb_desc_ring *desc_ring,
 	 * To reduce unnecessarily reopening, first check if the descriptor
 	 * state and caller ID are correct.
 	 */
-	d_state = desc_read(desc_ring, id, &desc, NULL, &cid);
+	d_state = desc_read(desc_ring, id, NULL, NULL, &cid);
 	if (d_state != desc_committed || cid != caller_id)
 		return NULL;
 
@@ -2007,14 +2006,13 @@ u64 prb_first_seq(struct printk_ringbuffer *rb)
 {
 	struct prb_desc_ring *desc_ring = &rb->desc_ring;
 	enum desc_state d_state;
-	struct prb_desc desc;
 	unsigned long id;
 	u64 seq;
 
 	for (;;) {
 		id = atomic_long_read(&rb->desc_ring.tail_id); /* LMM(prb_first_seq:A) */
 
-		d_state = desc_read(desc_ring, id, &desc, &seq, NULL); /* LMM(prb_first_seq:B) */
+		d_state = desc_read(desc_ring, id, NULL, &seq, NULL); /* LMM(prb_first_seq:B) */
 
 		/*
 		 * This loop will not be infinite because the tail is
-- 
2.43.0

Re: [PATCH] printk_ringbuffer: Avoid needless read of prb_desc

Posted by John Ogness 2 hours ago

On 2026-09-22, Wang Wensheng <wsw9603@163.com> wrote:
> diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
> index 85c0c854b3ce..607666d37455 100644
> --- a/kernel/printk/printk_ringbuffer.c
> +++ b/kernel/printk/printk_ringbuffer.c
> @@ -443,8 +443,8 @@ static enum desc_state get_desc_state(unsigned long id,
>   * descriptor is in an inconsistent state (miss or reserved), the caller can
>   * only expect the descriptor's @state_var field to be valid.
>   *
> - * The sequence number and caller_id can be optionally retrieved. Like all
> - * non-state_var data, they are only valid if the descriptor is in a
> + * The sequence number, caller_id, and desc can be optionally retrieved. Like
> + * all non-state_var data, they are only valid if the descriptor is in a
>   * consistent state.
>   */

I realize you are trying to minimally update the documentation. But
really the whole description needs to be appropriately updated. I
suggest:

/*
 * Get the state of a specified descriptor.
 *
 * Optionally, a copy of the descriptor, sequence number and/or caller ID can
 * be retrieved. However, (with the exception of @desc_out->state_var) all
 * retrieved values are invalid if the descriptor is in an inconsistent state
 * (miss or reserved).
 */

>  static enum desc_state desc_read(struct prb_desc_ring *desc_ring,
> @@ -1335,7 +1335,6 @@ static struct prb_desc *desc_reopen_last(struct prb_desc_ring *desc_ring,
>  {
>  	unsigned long prev_state_val;
>  	enum desc_state d_state;
> -	struct prb_desc desc;
>  	struct prb_desc *d;
>  	unsigned long id;
>  	u32 cid;
> @@ -1346,7 +1345,7 @@ static struct prb_desc *desc_reopen_last(struct prb_desc_ring *desc_ring,
>  	 * To reduce unnecessarily reopening, first check if the descriptor
>  	 * state and caller ID are correct.
>  	 */
> -	d_state = desc_read(desc_ring, id, &desc, NULL, &cid);
> +	d_state = desc_read(desc_ring, id, NULL, NULL, &cid);
>  	if (d_state != desc_committed || cid != caller_id)
>  		return NULL;
>  
> @@ -2007,14 +2006,13 @@ u64 prb_first_seq(struct printk_ringbuffer *rb)
>  {
>  	struct prb_desc_ring *desc_ring = &rb->desc_ring;
>  	enum desc_state d_state;
> -	struct prb_desc desc;
>  	unsigned long id;
>  	u64 seq;
>  
>  	for (;;) {
>  		id = atomic_long_read(&rb->desc_ring.tail_id); /* LMM(prb_first_seq:A) */
>  
> -		d_state = desc_read(desc_ring, id, &desc, &seq, NULL); /* LMM(prb_first_seq:B) */
> +		d_state = desc_read(desc_ring, id, NULL, &seq, NULL); /* LMM(prb_first_seq:B) */
>  
>  		/*
>  		 * This loop will not be infinite because the tail is

Using my suggestion for the documentation:

Reviewed-by: John Ogness <john.ogness@linutronix.de>

Re: [PATCH] printk_ringbuffer: Avoid needless read of prb_desc

Posted by Petr Mladek 1 day, 4 hours ago

On Tue 2026-09-22 14:25:20, Wang Wensheng wrote:
> Since commit f244b4dc53e5 ("printk: ringbuffer: Improve prb_next_seq()
> performance"), desc_read() allows @desc_out to be NULL. We can drop the
> local `desc` variable in desc_reopen_last() and prb_first_seq(). Also
> update the comment for desc_read().
> 
> Signed-off-by: Wang Wensheng <wsw9603@163.com>

Great catch! Looks good to me:

Reviewed-by: Petr Mladek <pmladek@suse.com>

I am going to wait few more days for more potential feedback.
I'll commit it the following week if nobody complains.

Best Regards,
Petr