Messages in this thread | | | From | John Ogness <> | Subject | Re: [RFC PATCH v2 1/2] printk-rb: add a new printk ringbuffer implementation | Date | Mon, 24 Jun 2019 15:55:26 +0200 |
| |
Hi,
I would like to point out an important typo related to memory barriers...
On 2019-06-07, John Ogness <john.ogness@linutronix.de> 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
| |