Re: [RFC PATCH v1 07/25] printk-rb: add functionality required by printk

[John Ogness]

On 2019-02-18, Petr Mladek <pmladek@xxxxxxxx> wrote:
>> The printk subsystem needs to be able to query the size of the ring
>> buffer, seek to specific entries within the ring buffer, and track
>> if records could not be stored in the ring buffer.
>> 
>> diff --git a/lib/printk_ringbuffer.c b/lib/printk_ringbuffer.c
>> index c2ddf4cb9f92..ce33b5add5a1 100644
>> --- a/lib/printk_ringbuffer.c
>> +++ b/lib/printk_ringbuffer.c
>> @@ -175,11 +175,16 @@ void prb_commit(struct prb_handle *h)
>>  				head = PRB_WRAP_LPOS(rb, head, 1);
>>  				continue;
>>  			}
>> +			while (atomic_long_read(&rb->lost)) {
>> +				atomic_long_dec(&rb->lost);
>> +				rb->seq++;
>
> The lost-messages handling should be in a separate patch.
> At least I do not see any close relation with prb_iter_seek().

Agreed.

> I would personally move prb_iter_seek() to the 5th patch that
> implements the other get/iterator functions.

Agreed.

> On the contrary, the patch adding support for lost messages
> should implement a way how to inform the user about lost messages.
> E.g. to add a warning when some space becomes available again.

The readers will see that messages were lost. I think that is enough. I
don't know how useful it would be to notify writers that space is
available. The writers are holding the prb_cpulock, so they definitely
shouldn't be waiting around for anything.

This situation should be quite rare because it means the _entire_ ring
buffer was filled up by an NMI context that interrupted a context that
was in the reserve/commit window. NMI contexts probably should not be
doing _so_ much printk'ing within a single NMI.

>> +			}
>>  			e->seq = ++rb->seq;
>>  			head += e->size;
>>  			changed = true;
>>  		}
>>  		atomic_long_set_release(&rb->head, res);
>> +
>>  		atomic_dec(&rb->ctx);
>>  
>>  		if (atomic_long_read(&rb->reserve) == res)
>> @@ -486,3 +491,93 @@ int prb_iter_wait_next(struct prb_iterator *iter, char *buf, int size, u64 *seq)
>>  
>>  	return ret;
>>  }
>> +
>> +/*
>> + * prb_iter_seek: Seek forward to a specific record.
>> + * @iter: Iterator to advance.
>> + * @seq: Record number to advance to.
>> + *
>> + * Advance @iter such that a following call to prb_iter_data() will provide
>> + * the contents of the specified record. If a record is specified that does
>> + * not yet exist, advance @iter to the end of the record list.
>> + *
>> + * Note that iterators cannot be rewound. So if a record is requested that
>> + * exists but is previous to @iter in position, @iter is considered invalid.
>> + *
>> + * It is safe to call this function from any context and state.
>> + *
>> + * Returns 1 on succces, 0 if specified record does not yet exist (@iter is
>> + * now at the end of the list), or -EINVAL if @iter is now invalid.
>> + */
>
> Do we really need to distinguish when the iterator is invalid and when
> we are at the end of the buffer?

Sure! There is big difference between "stop iterating because we hit the
newest entry" and "reset the iterator to the oldest entry because we
were overtaken by a writer".

> It seems that the reaction in both situation always is to call
> prb_iter_init(&iter, &printk_rb, &some_seq).

prb_iter_init() is only called to reset the iterator to the oldest
entry. That's all it is really doing. The fact that it can optionally
return a sequence number is just a convenience side-effect implemented
for some printk demands.

> I am still a bit
> confused what your prb_iter_init() does. Therefore I am not
> sure what it is supposed to do.
>
> Anyway, it seems to be typically used when you need to start
> from tail. I would personally do something like (based on my code
> in reply to 5th patch:
>
> int prb_iter_seek_to_seq(struct prb_iterator *iter, u64 seq)
> {
> 	int ret;
>
> 	ret = prb_iter_tail_entry(iter);
>
> 	while (!ret && iter->entry->seq != seq)
> 		ret = prb_iter_next_valid_entry(iter);
>
> 	return ret;
> }

Yes. Moving the loops inside prb_iter_tail_entry() and
prb_iter_next_valid_entry() definitely simplify the code.

> When I see it, I would make the functionality more obvious
> by renaming:
>
>     prb_iter_tail_entry() -> prb_iter_set_tail_entry()

I would say: prb_iter_set_oldest_entry()

>> +int prb_iter_seek(struct prb_iterator *iter, u64 seq)
>> +{
>> +	u64 cur_seq;
>> +	int ret;
>> +
>> +	/* first check if the iterator is already at the wanted seq */
>> +	if (seq == 0) {
>> +		if (iter->lpos == PRB_INIT)
>> +			return 1;
>> +		else
>> +			return -EINVAL;
>> +	}
>> +	if (iter->lpos != PRB_INIT) {
>> +		if (prb_iter_data(iter, NULL, 0, &cur_seq) >= 0) {
>> +			if (cur_seq == seq)
>> +				return 1;
>> +			if (cur_seq > seq)
>> +				return -EINVAL;
>> +		}
>> +	}
>> +
>> +	/* iterate to find the wanted seq */
>> +	for (;;) {
>> +		ret = prb_iter_next(iter, NULL, 0, &cur_seq);
>> +		if (ret <= 0)
>> +			break;
>> +
>> +		if (cur_seq == seq)
>> +			break;
>> +
>> +		if (cur_seq > seq) {
>> +			ret = -EINVAL;
>> +			break;
>> +		}
>
> This is a good example why prb_iter_data() and prb_iter_next() is
> a weird interface. You need to read the documentation very carefully
> to understand the difference (functionality, error codes). At least
> for me, it is far from obvious why they are used this way and
> if it is correct.

Agreed. I prefer your suggested API. They significantly simplify the
reader code, which as you'll see in later printk.c patches, is
everywhere.

John Ogness