Hi, I would like to point out an important typo related to memory barriers...
On 2019-06-07, John Ogness <[email protected]> wrote: > diff --git a/lib/printk_ringbuffer.c b/lib/printk_ringbuffer.c > new file mode 100644 > index 000000000000..d0b2b6a549b0 > --- /dev/null > +++ b/lib/printk_ringbuffer.c [...] > +static struct prb_descr *remove_oldest_descr(struct printk_ringbuffer *rb) > +{ > + struct prb_list *l = &rb->descr_list; > + unsigned long oldest_id; > + struct prb_descr *d; > + unsigned long next; > + > + for (;;) { > + oldest_id = READ_ONCE(l->oldest); > + > + /* list empty */ > + if (oldest_id == EOL) > + return NULL; > + > + d = TO_DESCR(rb, oldest_id); > + > + /* only descriptors with _invalid_ data can be removed */ > + if (data_valid(rb, READ_ONCE(rb->data_list.oldest), > + READ_ONCE(rb->data_list.newest), > + READ_ONCE(d->data), > + READ_ONCE(d->data_next))) { > + return NULL; > + } > + > + /* > + * MB6: synchronize link descr > + * > + * In particular: l->oldest is loaded as a data dependency so > + * d->next and the following l->oldest will load afterwards, > + * respectively. > + */ > + next = smp_load_acquire(&d->next); > + > + if (next == EOL && READ_ONCE(l->oldest) == oldest_id) { > + /* > + * The oldest has no next, so this is a list of one > + * descriptor. Lists must always have at least one > + * descriptor. > + */ > + return NULL; > + } > + > + if (cmpxchg(&l->oldest, oldest_id, next) == oldest_id) { > + /* removed successfully */ > + break; > + } This is supposed to be cmpxchg_relaxed(), not cmpxchg(). I did not intend to include the general mb() memory barriers around the RMW operation. (For some reason I thought _relaxed was the default.) Sorry. John Ogness
