tsrb: add drop function

[bergzand]

Contribution description

The get function does not support passing NULL as an input buffer. to be
able to drop bytes from the buffer, a dedicated drop function is
required

Issues/PRs references

required for #9870

[kaspar030]

how would it look if this would use tsrb_get_one() in a loop?

[bergzand]

Something like:

size_t tmp = n;
while (tmp && (tsrb_get_one(rb) != -1)) {
    tmp--;
}
return (n - tmp);

But I'm not sure whether tsrb_get_one is bug free. What happens if _pop returns -1 because that's what's in the buffer? Is that sign extended and thus interpreted as if the buffer is empty? (Do you want an issue for this?)

[kaspar030]

What happens if _pop returns -1 because that's what's in the buffer?

That is indeed a serious bug. Seems like slipdev is the only user of tsrb_get_one(), but how could that ever work? :)
We might have to change the whole API to use unsigned char.

[jcarrano]

The get function does not support passing NULL as an input buffer

Then why not make it support it? If something that was previously not allowed is then allowed, you should not be breaking any code.

[bergzand]

Then why not make it support it? If something that was previously not allowed is then allowed, you should not be breaking any code.

Because I expect that the impact of adding a separate function has less impact than adding the necessary checks to the function. In this case I would have to add a check to the tsrb_get() to check every cycle if the value from the buffer should be stored and if the pointer should be incremented.

[jcarrano]

check every cycle

That is only necessary because both functions are implemented with a loop. trsb_get() could be done with two memcpy() and tsrb_drop() with index manipulation, without calling get. And of course merging the two means only one instance of read index manipulation.

[bergzand]

That is only necessary because both functions are implemented with a loop. trsb_get() could be done with two memcpy() and tsrb_drop() with index manipulation, without calling get. And of course merging the two means only one instance of read index manipulation.

Feel free to build a pull request against this PR :)

[kaspar030]

trsb_get() could be done with two memcpy() and tsrb_drop() with index manipulation, without calling get.

Whatever you do, keep in mind that tsrb is supposed to be lock-free. The code is thread safe when there's one producer and one consumer. It is also used for ISR-to-thread-communication, thus mutexes cannot be used for synchronization. Cutting corners by using memcpy might break some assumptions.

[bergzand]

Cutting corners by using memcpy might break some assumptions.

That's exactly the reason why I don't want to burn myself implementing this with memcpy

[kaspar030]

ACK.

[jcarrano]

Feel free to build a pull request against this PR :)

Even if I do, once the drop function makes it into the API we are stuck with it.

[jcarrano]

The code is thread safe when there's one producer and one consumer.

And when writing to an unsigned int is an atomic operation, of which there is no guarantee.

[kaspar030]

And when writing to an unsigned int is an atomic operation, of which there is no guarantee.

Yes.

Is this a general statement like "it cannot be guaranteed on every platform in existence", or "the way it is used here it does not work as expected on RIOT supported architecture X"?

(Hint: aligned volatile unsigned int writes are atomic on MSP430 and ARM, not atomic on AVR 8bit. I'd assume MIPS and RISC-V have atomic word-size memory writes, but I don't know).

[jcarrano]

It means: the C language does not guarantee it. Even if the architecture has single instruction word-write and word-reads, the C language does not guarantee anything and the compiler is free to do whatever it chooses.

Finding a case of breakage in a 16-bit or 32-bit platform is harder than it is to fix it. For 8-bit platforms you are stuck with saving and restoring the interrupt mask.

[kaspar030]

It means: the C language does not guarantee it. Even if the architecture has single instruction word-write and word-reads, the C language does not guarantee anything and the compiler is free to do whatever it chooses.

What's the message here? never go outside of what C guarantees?

[jcarrano]

never go outside of what C guarantees

Of course!! That's the whole point of having a language specification, and when you need to do something that cannot be done with plain standard C, you use compiler-specific extensions (like 'asm' statements, builtins, etc) which are guaranteed (by the compiler).

Going back to the main theme of this PR, my points are:

1. Implementing drop as a separate function only because of performance reasons means translating the limitations of the implementation to the API, while complicating the API and adding code.

2. Those limitations are of this particular implementation and, as I pointed out, the implementation is flawed and we should expect it to change.

3. Having an additional function will make it more difficult to fix.

[miri64]

@jcarrano so are you blocking this PR or can we merge?

[jcarrano]

@miri64 Yes. I think we should try to keep the API simple. If allowing a NULL pointer to tsrb_get is (slightly) less efficient, that can be fixed. That is, if someone finds the inefficiency really problematic, otherwise we are optimizing ahead of time.

Also, considering that this "drop" function did not exist until now (meaning no-one was dropping bytes, or they were doing it with "get_one") there is no performance impact to existing code.

[kaspar030]

Funny, this is basically the opposite of what is requested in #9832.

edit with "this" I mean "I think we should try to keep the API simple." by making one function to two things.

[kaspar030]

@OlegHahm could you do some magic here?

[OlegHahm]

By magic you mean solving the deadlock? I can try...

[OlegHahm]

Okay, a first recap after I read the discussion:
There seems to be different discussions going on at the same time:

1. Should we add the proposed function (trsb_drop()) to the API or not?
   @bergzand and @kaspar030 argument in favor of adding this function, @jcarrano argues against it. IMO introducing the function would be aligned with the paradigm of "separation of concerns", i.e., making one function to do one thing right. From my point adding this function would make the API more simple than overloading the semantic of trsb_get(). Yes, if we decide that the module needs to be fixed, this function needs to be fixed as well, but I consider the module to be small enough to tolerate this.
2. Should we rely on the hardware/architecture to ensure that an unsigned integer atomicly or go with the language standard that does not guarantee that and extend the implementation by a mutex?
   IMO this is a completely separate discussion that should live in a separate issue or mailing list thread. I agree with @jcarrano that we risk hard to find bugs on certain (maybe future) platforms, if we rely on certain hardware behavior. Yet I understand @kaspar030's concern regarding performance. Maybe it helps to reflect to check what we want to achieve for core: a) stay 100% hardware independent and b) be as performant as possible. Of course, a compromise would be to add architecture dependent implementations, e.g., guarded by preprocessor IFs, but that would go against RIOT's architecture.

[jcarrano]

@bergzand I will not bikeshed you. Go ahead and add the function.

[OlegHahm]

@jcarrano, thanks for giving in and helping to resolve this issue. However, I wonder if all your concerns were covered or if you would like to open a new issue regarding your concerns with the hardware-independent thread-safety of the module.

[kaspar030]

new issue regarding your concerns with the hardware-independent thread-safety of the module.

There is: #9882.