Re: [PATCH printk v3 04/14] printk: ringbuffer: Do not skip non-finalized records with prb_next_seq()

From: John Ogness
Date: Mon Jan 15 2024 - 06:55:57 EST


On 2024-01-12, Petr Mladek <pmladek@xxxxxxxx> wrote:
>> u64 prb_next_seq(struct printk_ringbuffer *rb)
>> {
>> - struct prb_desc_ring *desc_ring = &rb->desc_ring;
>> - enum desc_state d_state;
>> - unsigned long id;
>> u64 seq;
>>
>> - /* Check if the cached @id still points to a valid @seq. */
>> - id = atomic_long_read(&desc_ring->last_finalized_id);
>> - d_state = desc_read(desc_ring, id, NULL, &seq, NULL);
>> + seq = desc_last_finalized_seq(rb);
>
> desc_last_finalized_seq() does internally:
>
> ulseq = atomic_long_read_acquire(&desc_ring->last_finalized_seq
> ); /* LMM(desc_last_finalized_seq:A) */
>
>
> It guarantees that this CPU would see the data which were seen by the
> CPU which updated desc_ring->last_finalized_seq.
>
> So far so good.
>
> The problem is that I somehow miss the counter part. Maybe,
> it is not needed. It just looks strange.

As the comments in desc_last_finalized_seq() state: "This pairs with
desc_update_last_finalized:A."

desc_update_last_finalized() successfully reads a record and then uses a
cmpxchg_release() to set the new @last_finalized_seq value (of the
record it just read). That is the counter part.

>> - if (d_state == desc_finalized || d_state == desc_reusable) {
>> - /*
>> - * Begin searching after the last finalized record.
>> - *
>> - * On 0, the search must begin at 0 because of hack#2
>> - * of the bootstrapping phase it is not known if a
>> - * record at index 0 exists.
>> - */
>> - if (seq != 0)
>> - seq++;
>> - } else {
>> - /*
>> - * The information about the last finalized sequence number
>> - * has gone. It should happen only when there is a flood of
>> - * new messages and the ringbuffer is rapidly recycled.
>> - * Give up and start from the beginning.
>> - */
>> - seq = 0;
>> - }
>> + /*
>> + * Begin searching after the last finalized record.
>> + *
>> + * On 0, the search must begin at 0 because of hack#2
>> + * of the bootstrapping phase it is not known if a
>> + * record at index 0 exists.
>> + */
>> + if (seq != 0)
>> + seq++;
>>
>> /*
>> * The information about the last finalized @seq might be inaccurate.
>
> Below is the code:
>
> while (_prb_read_valid(rb, &seq, NULL, NULL))
> seq++;
>
> Maybe, the release() should be here to make sure that the CPU which
> would see this "seq" would also the data.

The acquire is with @last_finalized_seq. So the release must also be
with @last_finalized_seq. The important thing is that the CPU that
updates @last_finalized_seq has actually read the corresponding record
beforehand. That is exactly what desc_update_last_finalized() does.

John