Re: [RFC PATCH v2 1/2] printk-rb: add a new printk ringbuffer implementation

[John Ogness]

On 2019-06-25, Sergey Senozhatsky <sergey.senozhatsky.work@xxxxxxxxx> wrote:
> [..]
>> +static void add_descr_list(struct prb_reserved_entry *e)
>> +{
>> +	struct printk_ringbuffer *rb = e->rb;
>> +	struct prb_list *l = &rb->descr_list;
>> +	struct prb_descr *d = e->descr;
>> +	struct prb_descr *newest_d;
>> +	unsigned long newest_id;
>> +
>> +	/* set as newest */
>> +	do {
>> +		/* MB5: synchronize add descr */
>> +		newest_id = smp_load_acquire(&l->newest);
>> +		newest_d = TO_DESCR(rb, newest_id);
>> +
>> +		if (newest_id == EOL)
>> +			WRITE_ONCE(d->seq, 1);
>> +		else
>> +			WRITE_ONCE(d->seq, READ_ONCE(newest_d->seq) + 1);
>> +		/*
>> +		 * MB5: synchronize add descr
>> +		 *
>> +		 * In particular: next written before cmpxchg
>> +		 */
>> +	} while (cmpxchg_release(&l->newest, newest_id, e->id) != newest_id);
>> +
>> +	if (unlikely(newest_id == EOL)) {
>> +		/* no previous newest means we *are* the list, set oldest */
>> +
>> +		/*
>> +		 * MB UNPAIRED
>> +		 *
>> +		 * In particular: Force cmpxchg _after_ cmpxchg on newest.
>> +		 */
>> +		WARN_ON_ONCE(cmpxchg_release(&l->oldest, EOL, e->id) != EOL);

This WARN_ON_ONCE...

>> +	} else {
>> +		/* link to previous chain */
>> +
>> +		/*
>> +		 * MB6: synchronize link descr
>> +		 *
>> +		 * In particular: Force cmpxchg _after_ cmpxchg on newest.
>> +		 */
>> +		WARN_ON_ONCE(cmpxchg_release(&newest_d->next,
>> +					     EOL, e->id) != EOL);

... and this WARN_ON_ONCE should both really be BUG_ON. These situations
will not happen. Actually, they should both be xchg_release(). But until
everyone is happy with the memory barriers, I wanted to leave this bug
checking in place.

>> +	}
>> +}
>
> [..]
>
>> +char *prb_reserve(struct prb_reserved_entry *e, struct printk_ringbuffer *rb,
>> +		  unsigned int size)
>> +{
>> +	struct prb_datablock *b;
>> +	struct prb_descr *d;
>> +	char *buf;
>> +
>> +	if (size == 0)
>> +		return NULL;
>> +
>> +	size += sizeof(struct prb_datablock);
>> +	size = DATA_ALIGN_SIZE(size);
>> +	if (size > DATAARRAY_SIZE(rb))
>> +		return NULL;
>> +
>> +	e->rb = rb;
>> +
>> +	local_irq_save(e->irqflags);
>> +
>> +	if (!assign_descr(e))
>> +		goto err_out;
>> +
>> +	d = e->descr;
>> +	WRITE_ONCE(d->id, e->id);
>> +
>> +	if (!data_reserve(e, size)) {
>> +		/* put invalid descriptor on list, can still be traversed */
>> +		WRITE_ONCE(d->next, EOL);
>> +		add_descr_list(e);
>> +		goto err_out;
>> +	}
>
> I'm wondering if prb can always report about its problems. Including the
> cases when things "go rather bad".
>
> Suppose we have
>
> 	printk()
> 	 prb_reserve()
> 	  !data_reserve()
> 	    add_descr_list()
> 	     WARN_ON_ONCE()
> 	      printk()
> 	       prb_reserve()
> 	        !assign_descr(e)   << lost WARN_ON's "printk" or "printks"?
>
> In general, assuming that there might be more error printk-s either
> called directly directly from prb->printk on indirectly, from
> prb->ABC->printk.
>
> Also note,
> Lost printk-s are not going to be accounted as 'lost' automatically.
> It seems that for printk() there is no way to find out that it has
> recursed from printk->prb_commit but hasn't succeeded in storing
> recursive messages. I'd say that prb_reserve() err_out should probably
> &rb->lost++.

This is a good point. I have no problems with that. In that case, it
should probably be called "fail" instead of "lost".

John Ogness