On 2019-06-25, Sergey Senozhatsky <sergey.senozhatsky.w...@gmail.com> 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
