Received-SPF: pass (google.com: domain of linux-kernel+bounces-25943-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: John Ogness <john.ogness@linutronix.de>
To: Petr Mladek <pmladek@suse.com>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>, Steven Rostedt
 <rostedt@goodmis.org>, Thomas Gleixner <tglx@linutronix.de>,
 linux-kernel@vger.kernel.org, Mukesh Ojha <quic_mojha@quicinc.com>
Subject: Re: [PATCH printk v3 04/14] printk: ringbuffer: Do not skip
 non-finalized records with prb_next_seq()
In-Reply-To: <ZaF_eJ_BCddZl5z1@alley>
References: <20231214214201.499426-1-john.ogness@linutronix.de>
 <20231214214201.499426-5-john.ogness@linutronix.de>
 <ZaF_eJ_BCddZl5z1@alley>
Date: Mon, 15 Jan 2024 13:01:36 +0106
Message-ID: <874jfeg69z.fsf@jogness.linutronix.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

On 2024-01-12, Petr Mladek <pmladek@suse.com> 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