Skip to main content
added 107 characters in body
Source Link
J_H
  • 38.9k
  • 3
  • 36
  • 147

The producer may as well use memset, to go as fast as possible on whatever CPU technology we ran on.

The producer may as well use memset, to go as fast as possible on whatever CPU technology we ran on.

added 66 characters in body
Source Link
J_H
  • 38.9k
  • 3
  • 36
  • 147

cite your reference

You didn't mention any documentation URLs. It would really help the review context if you explain what a ZCL (zigbee cluster?) or a ZCL3B is.


use braces

Sooner or later, someone is going to maintain this code. It might not be you. They might fall into the trap you've laid for them.


cite your reference

You didn't mention any documentation URLs. It would really help the review context if you explain what a ZCL (zigbee cluster?) or a ZCL3B is. On first reading I did not get that this denotes a zero copy lock-free triple buffer, nor did google. In a double- or triple- buffering context, the zero copy aspect seems a bit redundant. Maybe shorten the identifier?

cite your reference

You didn't mention any documentation URLs. It would really help the review context if you explain what a ZCL (zigbee cluster?) or a ZCL3B is.


use braces

Sooner or later, someone is going to maintain this code. It might not be you. They might fall into the trap you've laid for them.

use braces

Sooner or later, someone is going to maintain this code. It might not be you. They might fall into the trap you've laid for them.


cite your reference

You didn't mention any documentation URLs. It would really help the review context if you explain what a ZCL (zigbee cluster?) or a ZCL3B is. On first reading I did not get that this denotes a zero copy lock-free triple buffer, nor did google. In a double- or triple- buffering context, the zero copy aspect seems a bit redundant. Maybe shorten the identifier?

Source Link
J_H
  • 38.9k
  • 3
  • 36
  • 147

cite your reference

You didn't mention any documentation URLs. It would really help the review context if you explain what a ZCL (zigbee cluster?) or a ZCL3B is.


use braces

Even a single-line if body deserves { } braces.

Sooner or later, someone is going to maintain this code. It might not be you. They might fall into the trap you've laid for them.


compound type

This just isn't enough:

#define ZCL3B_LOCK_FLAG 0x80

You really need to /* explain */ that you're adding 128 to what would otherwise be a valid buffer index. That is, we have {flag_bit, five_padding_zeros, index}. As written, I would expect this to go into a "flags byte". Alternatively, consider defining a 0x3 mask on the subsequent line, again with an eye on explaining the fields together in one place.

It is nice that you've grouped this with zcl3b_idx_t, thank you. I just had no idea they were deliberately grouped, upon my first reading.


magic buf len

typedef struct {
    uint32_t data[128];
} msg_t;
        ...
        for (int i = 1; i < sizeof(msg->data) / sizeof(msg->data[0]); i++) {
        ...
        for (int i = 0; i < sizeof(msg->data) / sizeof(msg->data[0]); i++)

The magic number 128 appears in three places, albeit two of them will be automatically dealt with if a maintainer e.g. doubles the buffer size.

Better to define a constant for the number of entries.


don't ignore errors

static int create_shm(size_t buf_size)
{
    ...
    int r = memfd_create("zcl3b", MFD_ALLOW_SEALING);
    if (r < 0)
        return -errno;
    ...
int main()
{
    int fd = create_shm(sizeof(msg_t));
    pid_t pid = fork();

and then eventually zcl3b_map_init will notice the negative file descriptor.

Prefer to deal with errors where they happen, rather than letting them cascade down the call stacks of two separate processes. The contracts of these functions would then be much simpler.

If we write bugs, we want to write shallow bugs that some poor maintainer down the road can easily diagnose.

The goto fail is very nice, it's a standard pattern.

Dealing with other failed sys calls, such as fork(), would also be good.


synchronous writes at init

    atomic_store_explicit(map->xchg, BUFFER_NONE, memory_order_relaxed);

This achieves atomic writes with relaxed_ordering where we probably wanted sequential consistency across processes. It only runs once per process so it would be hard to observe race lossage.


one more stat

        msg_t *msg = zcl3b_consumer_update(&consumer);

        if (msg == NULL) {
            usleep(1000);
            continue;
        }

In consumer_task, consider incrementing a separate statistic there. (I suppose you already have it, if you sum statistics and subtract that from NUM_CYCLES.)

Alternatively, consider writing a "wait for producer startup" loop that will only let us continue on to the main for loop once it has seen non-zero data from the producer.


torture test

The point of this code is to make a convincing argument that consumer never sees unlocked (inconsistent) data. So consider performing the verification reads according to some shuffled permuted order, rather than sequentially. Or more simply, do the reads in reverse order.


This code achieves its design goals.

I would be willing to delegate or accept maintenance tasks on it.