Re: assign_desc() barriers: Re: [RFC PATCH v4 1/9] printk-rb: add a new printk ringbuffer implementation
From: John Ogness
Date: Mon Aug 26 2019 - 04:22:02 EST
On 2019-08-25, John Ogness <john.ogness@xxxxxxxxxxxxx> wrote:
>>>>>> --- /dev/null
>>>>>> +++ b/kernel/printk/ringbuffer.c
>>>>>> +static bool assign_desc(struct prb_reserved_entry *e)
>>>>>> + atomic_long_set_release(&d->id, atomic_long_read(&d->id) +
>>>>>> + DESCS_COUNT(rb));
>>>>> atomic_long_set_release() might be a bit confusing here.
>>>>> There is no related acquire.
>>> As the comment states, this release is for prb_getdesc() users. The
>>> only prb_getdesc() user is _dataring_pop(). (i.e. the descriptor's
>>> ID is not what _dataring_pop() was expecting), then the tail must
>>> have moved and _dataring_pop() needs to see that. Since there are no
>>> data dependencies between descriptor ID and tail_pos, an explicit
>>> memory barrier is used. More on this below.
>> + The two related barriers are in different source files
>> and APIs:
>> + assign_desc() in ringbuffer.c; ringbuffer API
>> + _dataring_pop in dataring.c; dataring API
> Agreed. This is a consequence of the ID management being within the
> high-level ringbuffer code. I could have added an smp_rmb() to the
> NULL case in prb_getdesc(). Then both barriers would be in the same
> file. However, this would mean smp_rmb() is called many times
> (particularly by readers) when it is not necessary.
What I wrote here is wrong. prb_getdesc() is not called "many times
(particularly by readers)". It is only called once within the writer
Looking at this again, I think it would be better to move the smp_rmb()
into the NULL case of prb_getdesc(). Then both barrier pairs are located
(and documented) in the same file. This also simplifies the
documentation by not saying "the caller's smp_rmb() everywhere".
I would also change _dataring_pop() so that the smp_rmb() is located
within the handling of the other two failed checks (begin_lpos !=
tail_lpos and !_datablock_valid()). Then the out: at the end is just
After modifying the code in this way, I think it looks more straight
forward and would have avoided your confusion: The RMB in
dataring.c:_dataring_pop() matches the MB in dataring.c:dataring_push()
and the RMB in ringbuffer.c:prb_getdesc() matches the SET_RELEASE in