buf_ring(9) API precisions

[K. Macy]

You're right. A write memory barrier is needed there.

Thanks

On Thu, Sep 22, 2011 at 12:43 AM, Arnaud Lacombe <lacombar at gmail.com> wrote:
> Hi,
>
> On Mon, Sep 19, 2011 at 8:46 AM, K. Macy <kmacy at freebsd.org> wrote:
>> If the value lags next by one then it is ours. This rule applies to
>> all callers so the rule holds consistently.
>>
> I think you do not understand what I mean, which is that the following:
>
>       while (br->br_prod_tail != prod_head)
>               cpu_spinwait();
>       br->br_prod_bufs++;
>       br->br_prod_bytes += nbytes;
>       br->br_prod_tail = prod_next;
>       critical_exit();
>
> at runtime, can be seen, memory-wise as:
>
>       while (br->br_prod_tail != prod_head)
>               cpu_spinwait();
>       br->br_prod_tail = prod_next;
>       br->br_prod_bufs++;
>       br->br_prod_bytes += nbytes;
>       critical_exit();
>
> That is, there is no memory barrier to enforce completion of the
> load/increment/store/load/load/addition/store operations before
> updating what other thread spin on. Yes, `br_prod_tail' is marked
> `volatile', but there is no guarantee that it will not be re-ordered
> wrt. non-volatile write (to `br_prod_bufs' and `br_prod_bytes').
>
>  - Arnaud
>
>> On Mon, Sep 19, 2011 at 5:53 AM, Arnaud Lacombe <lacombar at gmail.com> wrote:
>>> Hi,
>>>
>>> On Fri, Sep 16, 2011 at 10:41 AM, K. Macy <kmacy at freebsd.org> wrote:
>>>> On Fri, Sep 16, 2011 at 3:02 AM, Arnaud Lacombe <lacombar at gmail.com> wrote:
>>>>> Hi,
>>>>>
>>>>> On Wed, Sep 14, 2011 at 10:53 PM, Arnaud Lacombe <lacombar at gmail.com> wrote:
>>>>>> Hi Kip,
>>>>>>
>>>>>> I've got a few question about the buf_ring(9) API.
>>>>>>
>>>>>> 1) what means the 'drbr_' prefix. I can guess the two last letter, 'b'
>>>>>> and 'r', for Buffer Ring, but what about 'd' and 'r' ?
>>>>>>
>>>>>> 2) in `sys/sys/buf_ring.h', you defined 'struct buf_ring' as:
>>>>>>
>>>>>> struct buf_ring {
>>>>>>        volatile uint32_t       br_prod_head;
>>>>>>        volatile uint32_t       br_prod_tail;
>>>>>>        int                     br_prod_size;
>>>>>>        int                     br_prod_mask;
>>>>>>        uint64_t                br_drops;
>>>>>>        uint64_t                br_prod_bufs;
>>>>>>        uint64_t                br_prod_bytes;
>>>>> shouldn't those 3 fields be updated atomically, especially on 32bits
>>>>> platforms ? That might pose a problem as, AFAIK, FreeBSD do not have
>>>>> MI 64bits atomics operations...
>>>>
>>>> Between the point at which br_prod_tail == prod_head and when we
>>>> update br_prod_tail to point to prod_next we are the exclusive owners
>>>> of the fields in buf_ring. That is why we wait for any other
>>>> enqueueing threads to update br_prod_tail to point to prod_head before
>>>> continuing.
>>>>
>>> How do you enforce ordering ? I do not see anything particular
>>> forbidding the `br->br_prod_tail' to be committed first, leading other
>>> thread to believe they have access to the statistics, while the other
>>> thread has not yet committed its change.
>>>
>>> Thanks,
>>>  - Arnaud
>>>
>>>> Cheers
>>>>
>>>>        /*
>>>>         * If there are other enqueues in progress
>>>>         * that preceeded us, we need to wait for them
>>>>         * to complete
>>>>         */
>>>>        while (br->br_prod_tail != prod_head)
>>>>                cpu_spinwait();
>>>>        br->br_prod_bufs++;
>>>>        br->br_prod_bytes += nbytes;
>>>>        br->br_prod_tail = prod_next;
>>>>        critical_exit();
>>>>
>>>
>>
>