search

dhess / c-ringbuf

A ring buffer implemented in C
Other
418 stars 143 forks source link

Add read thread and write thread support #4

Closed tverbin closed 6 years ago

tverbin commented 6 years ago

With this fix ringbuf should work fine with one thread writes to buffer and one thread reads from it. Tested with my code base.

dhess commented 6 years ago

As far as I know, ssize_t is not atomic, so the C spec does not guarantee that writes to a ssize_t value will be atomic, nor seen in a sequentially-consistent order in a multiprocessor system. (See https://en.wikipedia.org/wiki/Sequential_consistency)

Therefore, though this change might be thread-safe on some architectures or implementations, I do not believe it is guaranteed to be thread-safe, and I must reject it, unless you can demonstrate that my reasoning is incorrect in all cases on all architectures.

I am not up-to-date on recent C specifications, but unless some quite radical changes have been made to C's concurrency support and memory ordering model (or lack thereof), I do not believe there is any way to make this implementation portably thread-safe without explicit pthreads support.

tverbin commented 6 years ago

The problem isn't whether or not ssize_t is atomic, because it's a local variable. The real issue is whether or not head and tail are atomic, meaning whether or not pointers are atomic. As far as I know only C11 has specification for an atomic API. On most popular architectures pointers set instruction should be atomic but of course instructions order might be changed on compilation time or run time. In conclusion, your'e right.

dhess commented 6 years ago

Yes, of course you're right, the ssize_t variable is local, and the issue is the pointers.

I will look into the C11 spec, thanks for the info re: an atomic API. It would be nice to make this thread-safe without needing to drag pthreads into it.

Synss commented 5 years ago

@levush

I do not think that ssize_t is standard C (maybe POSIX) and in any case, it is not guaranteed to go lower than –1.

ptrdiff_t could be better but I cannot find it either in the N1570 draft so it may not be standard either.

static-void commented 5 years ago

I will look into the C11 spec, thanks for the info re: an atomic API. It would be nice to make this thread-safe without needing to drag pthreads into it.

As an alternative for systems without C11, you could use the gcc __atomic built-ins https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html#g_t_005f_005fatomic-Builtins

(or the __sync built-ins, which are available in earlier gcc versions https://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html)

BTW I am using this in a production project with multiple threads, but I wrap the entire buffer in a pthread mutex. I am using it for audio processing where the underlying library is libsoundio, which has its own "magic ringbuffer" implementation, but I had issues with that implementation so switched to your library.

dhess commented 5 years ago

@static-void Thanks for the report. It’s gratifying to see this code being used in a production project!

BTW, you’re using this code in exactly the way it was intended — not as a finished product, but as a solid, portable base for adding your own functionality on top (in your case, pthread support).

static-void commented 5 years ago

I found the library self explanatory, and I like that it's no frills just a ring buffer when you need one.

I did add a function void ringbuf_discard(struct ringbuf_t *rb, size_t count); I guess I should make a pull request when I find time, but I don't know if I will find time...