[RFC] Ring buffers


Dmitriy Korovkin
 

Colleagues, any comment on this?

In it's current state the ring buffers appear as internal kernel objects
used by event loger and ipm console.

The proposal here is to make them kernel objects available for application
developers, that includes the following steps:

- move the ring buffers declaration to include/kernel.h and chage routine
names according to the public API naming convention;

- pend writing threads for a certain timeout if no space available;

- pend reading threads (with timeout) if the buffer is empty.

Is there a need for this?

Regards,
Dmitriy Korovkin


Andy Ross
 

Korovkin, Dmitriy (Wind River) wrote (on Friday, September 23, 2016 12:33PM):
- move the ring buffers declaration to include/kernel.h and chage routine
names according to the public API naming convention;

- pend writing threads for a certain timeout if no space available;

- pend reading threads (with timeout) if the buffer is empty.
That sounds more like a hybrid between a ring buffer (a data
structure) and a fifo (an IPC mechanism). I think there's value for a
generic buffer for bytewise streamlike data which isn't tied to a
particular idea of syncronization. Maybe split them up and have a
"stream" IPC abstraction that the user can wrap around a ring buffer
they provide?

Andy


Dmitriy Korovkin
 

On Fri, 23 Sep 2016, Andy Ross wrote:

Korovkin, Dmitriy (Wind River) wrote (on Friday, September 23, 2016 12:33PM):
- move the ring buffers declaration to include/kernel.h and chage routine
names according to the public API naming convention;

- pend writing threads for a certain timeout if no space available;

- pend reading threads (with timeout) if the buffer is empty.
That sounds more like a hybrid between a ring buffer (a data
structure) and a fifo (an IPC mechanism). I think there's value for a
generic buffer for bytewise streamlike data which isn't tied to a
particular idea of syncronization. Maybe split them up and have a
"stream" IPC abstraction that the user can wrap around a ring buffer
they provide?
User can always add a semaphore. Or specify K_NO_WAIT as a timeout value.

/Dmitriy

Andy


Boie, Andrew P
 

On Fri, 2016-09-23 at 15:33 -0400, Dmitriy Korovkin wrote:
Colleagues, any comment on this?

In it's current state the ring buffers appear as internal kernel
objects 
used by event loger and ipm console.

The proposal here is to make them kernel objects available for
application 
developers, that includes the following steps:
NACK, for reasons below.


- move the ring buffers declaration to include/kernel.h and chage
routine
The header is currently in misc/ring_buffer.h. This is available to
applications, why do you want to move it?

   names according to the public API naming convention;
Why do you think the names need to be changed?


- pend writing threads for a certain timeout if no space available;

- pend reading threads (with timeout) if the buffer is empty.
No, don't bake this stuff into the basic ring buffer structure. If you
want to implement this kind of protocl you can build on top of the
existing ring buffer code.

It says as much in the documentation:

/**
 * @brief Place an entry into the ring buffer
 *
 * Concurrency control is not implemented, however no synchronization
is needed
 * between put() and get() operations as they independently work on the
 * tail and head values, respectively.
 * Any use-cases involving multiple producers will need to synchronize
use
 * of this function, by either disabling preemption or using a mutex.
 */

There is no one-size-fits-all concurrency model that makes everyone
happy, these considerations are application specific. The fact that
concurrency control is not included is definitely by design.

Andrew


Boie, Andrew P
 

On Fri, 2016-09-23 at 13:27 -0700, Boie, Andrew P wrote:
NACK, for reasons below.
Just to qualify this -- if you wanted to create a general purpose
kernel object type that builds on what is in ring_buffer.h, I would not
be opposed.

But I want the existing definition left as-is as it is intentionally
lightweight and for scenarios where you have a single producer and
single consumer you don't need extra concurrency control.

Andrew


Mitsis, Peter <Peter.Mitsis@...>
 

It occurs to me that a buffered pipe may be leveraged as a generic ring buffer with concurrency control--though not a light-weight one.

Peter

-----Original Message-----
From: Boie, Andrew P [mailto:andrew.p.boie(a)intel.com]
Sent: September-23-16 4:32 PM
To: devel(a)lists.zephyrproject.org; Korovkin, Dmitriy
Subject: [devel] Re: [RFC] Ring buffers

On Fri, 2016-09-23 at 13:27 -0700, Boie, Andrew P wrote:
NACK, for reasons below.
Just to qualify this -- if you wanted to create a general purpose kernel
object type that builds on what is in ring_buffer.h, I would not be
opposed.

But I want the existing definition left as-is as it is intentionally
lightweight and for scenarios where you have a single producer and single
consumer you don't need extra concurrency control.

Andrew


Dmitriy Korovkin
 

On Fri, 23 Sep 2016, Boie, Andrew P wrote:

On Fri, 2016-09-23 at 13:27 -0700, Boie, Andrew P wrote:
NACK, for reasons below.
Just to qualify this -- if you wanted to create a general purpose
kernel object type that builds on what is in ring_buffer.h, I would not
be opposed.

But I want the existing definition left as-is as it is intentionally
lightweight and for scenarios where you have a single producer and
single consumer you don't need extra concurrency control.
Just for the clarification.
I never stated the idea of concurrency protocol. As I stated previously,
the idea that is discussed here is to make ring buffers available for
users the same way as it is done for other kernel objects, which is for
unified kernel means placing object declaration into kernel.h. Not that
having heder in another place makes it unavailable, it is a matter of
convenience.

Next, when I was saying of making reading thread wait if the buffer is
empty and writing thread wait if there is no free space, this does not
resolve and does not intend to resolve the problem of several concurrent
threads trying to access the ring buffer - such a problem may be resolved
with mutexes.

My apologies, if I did not make it clear enough, I hope I have done it now
to avoid the further confusion.

Regards,
/Dmitriy.


Dmitriy Korovkin
 

On Fri, 23 Sep 2016, Mitsis, Peter wrote:

It occurs to me that a buffered pipe may be leveraged as a generic
ring buffer with concurrency control--though not a light-weight one.
Peter, this is a very good statement. The question is what we get from
ring buffer that we can not get from fifo?

Thanks!
/Dmitriy


Boie, Andrew P
 

On Fri, 2016-09-23 at 16:46 -0400, Dmitriy Korovkin wrote:
Next, when I was saying of making reading thread wait if the buffer
is 
empty and writing thread wait if there is no free space, this does
not 
resolve and does not intend to resolve the problem of several
concurrent 
threads trying to access the ring buffer - such a problem may be
resolved 
with mutexes.
I contend this doesn't belong in the base ring buffer implementation
either. I feel you are assuming that this policy of waiting is
appropriate for all situations and I disagree.

Ring buffers aren't just used in thread context. If an ISR wants to
stick something in the ring buffer, we can't wait here.

Also what if the thead would rather do something else than block if the
buffer is full? This is why the API returns -ENOSPC.

So I maintain that if you want to build something on top of base
ring_buffer.h for the common cases, that is fine, but don't assume they
are what's appropriate for all uses, the base API needs to stay how it
is.

Andrew


Benjamin Walsh <benjamin.walsh@...>
 

On Fri, Sep 23, 2016 at 09:24:46PM +0000, Boie, Andrew P wrote:
On Fri, 2016-09-23 at 16:46 -0400, Dmitriy Korovkin wrote:
Next, when I was saying of making reading thread wait if the buffer
is 
empty and writing thread wait if there is no free space, this does
not 
resolve and does not intend to resolve the problem of several
concurrent 
threads trying to access the ring buffer - such a problem may be
resolved 
with mutexes.
I contend this doesn't belong in the base ring buffer implementation
either. I feel you are assuming that this policy of waiting is
appropriate for all situations and I disagree.

Ring buffers aren't just used in thread context. If an ISR wants to
stick something in the ring buffer, we can't wait here.

Also what if the thead would rather do something else than block if the
buffer is full? This is why the API returns -ENOSPC.

So I maintain that if you want to build something on top of base
ring_buffer.h for the common cases, that is fine, but don't assume they
are what's appropriate for all uses, the base API needs to stay how it
is.
Point taken, and entirely valid. We won't touch the functionality of the
ring buffers.

I think that we should still have the code to under kernel/ though, and
rename APIs to k_ring_buf_<whatever>.


Andy Ross
 

Benjamin Walsh wrote (on Friday, September 23, 2016 2:36PM):
I think that we should still have the code to under kernel/ though,
and rename APIs to k_ring_buf_<whatever>.
Naming isn't a big deal, but I'll reiterate my previous point: a ring
buffer is a data structure, not an OS abstraction provided by a
kernel. It's equally useful to application or subsystem code, or I
dunno, Windows C++ apps. It's not meaningfully a "Zephyr" thing.

I mean, the dlist implementation is used pervasively in the kernel,
but I don't think anyone would argue it belongs in kernel.h instead
include/misc/dlist.h.

Andy


Boie, Andrew P
 

On Fri, 2016-09-23 at 14:56 -0700, Andy Ross wrote:
Benjamin Walsh wrote (on Friday, September 23, 2016 2:36PM):

I think that we should still have the code to under kernel/ though,
and rename APIs to k_ring_buf_<whatever>.
Naming isn't a big deal, but I'll reiterate my previous point: a ring
buffer is a data structure, not an OS abstraction provided by a
kernel.  It's equally useful to application or subsystem code, or I
dunno, Windows C++ apps.  It's not meaningfully a "Zephyr" thing.

I mean, the dlist implementation is used pervasively in the kernel,
but I don't think anyone would argue it belongs in kernel.h instead
include/misc/dlist.h.
I agree with Andy for reasons above, plus if you *did* move it, it
would be subject to our deprecation policy (maintain both APIs for two
releases with the old one marked __deprecated).

Andrew


Benjamin Walsh <benjamin.walsh@...>
 

On Fri, Sep 23, 2016 at 06:12:41PM -0400, Boie, Andrew P wrote:
On Fri, 2016-09-23 at 14:56 -0700, Andy Ross wrote:
Benjamin Walsh wrote (on Friday, September 23, 2016 2:36PM):

I think that we should still have the code to under kernel/ though,
and rename APIs to k_ring_buf_<whatever>.
Naming isn't a big deal, but I'll reiterate my previous point: a ring
buffer is a data structure, not an OS abstraction provided by a
kernel. It's equally useful to application or subsystem code, or I
dunno, Windows C++ apps. It's not meaningfully a "Zephyr" thing.

I mean, the dlist implementation is used pervasively in the kernel,
but I don't think anyone would argue it belongs in kernel.h instead
include/misc/dlist.h.
I agree with Andy for reasons above, plus if you *did* move it, it
would be subject to our deprecation policy (maintain both APIs for two
releases with the old one marked __deprecated).
All valid points. We won't touch them.