Libsharedringbuffer

[lucypa]

This is a new library as part of the sDDF. It provides an efficient data transport layer between driver and client, and was implemented to improve network I/O performance. It will later be used as part of ongoing VMM projects as well.

Test with: seL4/global-components#33

[axel-h]

Suggested change

	*cookie = &ring->buffers[ring->read_idx % CONFIG_LIB_SHARED_RINGBUFFER_DESC_COUNT];
	*cookie = &ring->buffers[ring->read_idx % CONFIG_LIB_SHARED_RINGBUFFER_DESC_COUNT].cookie;

[axel-h]

This should be a debug log printed afterwards (to have the context when reading the logs) or just merge into the next log.

[lsf37]

Sorry, I gave you the wrong one on the chat, it should be BSD-2-Clause for these

[lsf37]

READMEs and docs are CC-BY-SA-4.0

[lucypa]

Okay thanks, I fixed this up

[lsf37]

Hi @lucypa, the content looks fine from my side, but I will leave a deeper review of that to systems people.

There is something slightly strange in the commit sequence of the PR, it seems to contain commits that are already on the master branch. Might just need a rebase.

Most of the commits in this PR should be squashed into fewer larger commits (X update, style, etc). The commits should be single logical units and ideally go from a working/compilable code state to working/compilable code state, so that git bisect can be used to find issues later. See also the git conventions and git style guide.

The link check is failing, but is fine to ignore for this PR (the failure is unrelated)

[axel-h]

We should use size_t here, so it scales with the architecture. Unless there is a strong reason to fix the structure size across architectures.

[axel-h]

Should be size_t also and not fix is to 32-bit (even is 32-bit might be practically sufficient.

[lucypa]

I thought that size_t would be reserved for lengths/sizes of things. This is just an index into the array

[axel-h]

For portable code, my rule of thumb usually is, that it's either unsigned int (as C's native integer) or size_t (to support the maximum range, which might be more than unsigned int covers) then. If specific fix-size types are used, this should have a good reason or some explanation that we want to ensure something - but this would also become compile time asserts then.
When it comes to structures, a reason might be that the structure's memory layout is important because it get's serialized/deserialized and thus has to work across different architectures.

[axel-h]

Should this get a context parameter for convenience reasons?

[axel-h]

Could the code redundancy a bit (even if the compiler might optimize this anyway)

buff_desc_t *buff_desc = &(ring->buffers[ring->write_idx % CONFIG_LIB_SHARED_RINGBUFFER_DESC_COUNT]);
buff_desc->encoded_addr = buffer
buff_desc->len = len;
buff_desc->cookie = cookie;

[axel-h]

Same here

buff_desc_t *buff_desc = &(ring->buffers[ring->read_idx % CONFIG_LIB_SHARED_RINGBUFFER_DESC_COUNT]);
*addr = buff_desc->encoded_addr;
...

[axel-h]

Actually, this is agnostic of what this memory is. IN the end, this could also be a void* in the end.

[axel-h]

Why do we have the modulo operation here? Isn't this simply:

return (ring->write_idx == ring->read_idx);

[axel-h]

Isn't the a bool type also that we could use here?

[axel-h]

Sam here, why the modulo operation, this will even work for overflows:

return CONFIG_LIB_SHARED_RINGBUFFER_DESC_COUNT == (ring->write_idx - ring->read_idx);

[axel-h]

Thanks for factoring this out. I wonder, How much does this differ from the libvirtqueue actually? Could this be code be unified somehow?

[lucypa]

Hi @axel-h
Thanks for reviewing it.
This design was chosen for the sDDF as it was more transparent and easy to reason about... there seems to be multiple implementations of virtqueues in various repositories and the particular one i looked into didn't quite match up with the paper it provided. This also performed better with camkes when benchmarked with an echo server.
I think the plan is to review the virtqueue implementations for VMM work soon.

[axel-h]

Can you add a comment about the value of the address, e.g which range is allowed here, which spec defines this...

[axel-h]

Do we consider passing NULL for any of these an error actually? We could put an assert here for development convenience, so this is caught and avoids potentially having to debug strange errors later.

[axel-h]

Is the style check demanding this line break?

[axel-h]

please add a newline at the end of files.

[axel-h]

We cold also add a cheap sanity check here that the indices are not running wild or got corrupted:

Suggested change

	return CONFIG_LIB_SHARED_RINGBUFFER_DESC_COUNT == (ring->write_idx - ring->read_idx);
	size_t cnt = ring->write_idx - ring->read_idx;
	assert(cnt <= CONFIG_LIB_SHARED_RINGBUFFER_DESC_COUNT)
	return (CONFIG_LIB_SHARED_RINGBUFFER_DESC_COUNT == cnt);

[axel-h]

nitpicking: I think all we ensure here is, that no writes are reorderd beyond this point, so they are visible. But it's not guaranteed the data really ends up in memory, as this is no cache flush.

[kent-mcleod]

Is it intentional that this increment isn't after the release barrier? Otherwise I think it could potentially complete before the writes to the buff_desc do and cause a reader to read too early.

[lucypa]

No, sorry i think this was an accident. Thanks for pointing it out!

[kent-mcleod]

I'm not sure if it'd end up making a noticeable performance difference, but there's an option for these two lines to be combined into __atomic_store_n(&ring->read_idx, ring->read_idx+1, __ATOMIC_RELEASE); which allows the compiler to generate slightly different instructions on architectures like aarch64 where there is a store-release instruction (stlr) which means an additional dmb ish isn't required as shown in the small assembly examples below which are generated from aarch64-linux-gnu-gcc:

  4005a8:       d5033bbf        dmb     ish
  4005ac:       90000462        adrp    x2, 48c000 <object.0+0x28>
  .. <cut 2 unrelated instructions>
  4005b8:       b9400841        ldr     w1, [x2, #8]
  4005bc:       11000421        add     w1, w1, #0x1
  4005c0:       b9000841        str     w1, [x2, #8]

  4005b0:       b9400801        ldr     w1, [x0, #8]
  4005b4:       11000421        add     w1, w1, #0x1
  4005b8:       889ffc41        stlr    w1, [x2]

[axel-h]

Do not really have a requirement for __atomic_store_n()? There is no race for incrementing the value, there is only one writer. Also, does this guarantee that all other writes are finish before also, or do we still need a barrier to play safe to enforce this?

[kent-mcleod]

It's still a plain store, but with release semantics. So it's not allowed to be re-ordered with any memory operations that come before it in program-order.

[axel-h]

We could simply use our helper function:

Suggested change

	if (!((ring->write_idx - ring->read_idx) % CONFIG_LIB_SHARED_RINGBUFFER_DESC_COUNT)) {
	if (ring_empty(ring) {

[kent-mcleod]

These two includes need to have their order swapped in order to avoid the following compiler warning:

Suggested change

	#include <sel4utils/sel4_zf_logif.h>
	#include <utils/util.h>
	#include <utils/util.h>
	#include <sel4utils/sel4_zf_logif.h>

[267/468] Building C object projects_libs/li...d_ringbuffer.dir/src/shared_ringbuffer.c.obj
In file included from /tmp/tmp.MkaXVLiTOr/projects/projects_libs/libsharedringbuffer/include/shared_ringbuffer/shared_ringbuffer.h:12,
                 from /tmp/tmp.MkaXVLiTOr/projects/projects_libs/libsharedringbuffer/src/shared_ringbuffer.c:7:
/tmp/tmp.MkaXVLiTOr/projects/util_libs/libutils/include/utils/util.h:40:2: warning: #warning "Attempted to set ZF_LOG_LEVEL but _ZF_LOG_LEVEL has already been defined." "Check that <utils/zf_log.h> hasn't been imported before this file, or define ZF_LOG_LEVEL explicitly before including <utils/zf_log.h>." [-Wcpp]
   40 | #warning "Attempted to set ZF_LOG_LEVEL but _ZF_LOG_LEVEL has already been defined." \
      |  ^~~~~~~

[lucypa]

Thanks!