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

[wangwensheng]

On 9/24/26 21:49, John Ogness wrote:
> On 2026-09-22, Wang Wensheng <wsw9603@xxxxxxx> 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@xxxxxxxxxxxxx>
Thanks for your comment. I have sent a new patch with your suggestion.