Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add functions to take/release just producer or consumer #78

Closed
wants to merge 3 commits into from

Conversation

timvisee
Copy link

@timvisee timvisee commented Nov 12, 2020

This implements:

  • BBBuffer::try_take_producer: split-off just the producer
  • BBBuffer::try_take_consumer: split-off just the consumer
  • BBBuffer::try_release_producer: release (give back) just the producer
  • BBBuffer::try_release_consumer: release (give back) just the consumer

I choose to use an AtomicU8 for the already_split state with a bit for the producer and consumer parts which are defined in BIT_PRODUCER and BIT_CONSUMER respectively. I thought this would be the right approach as updating the state for both the producer and consumer can still be done in a single atomic operation. That wouldn't be the case when using two separate AtomicBool's.

Things to resolve before merge:

  • Do we still need to zero the buffer when taking just the producer or consumer, like try_split? (currently commented-out)
  • Do we still need to reinitialize the buffer when releasing just the producer or consumer, like try_release?
  • This uses fetch_or and fetch_add, do we need to implement this for thumbv6?

The above things I'm currently uncertain about. My knowledge is lacking on this, thus I'd like to ask to make sure the implementation is sound. The PR branch is open, so feel free to make an edit.

Fixes #40

Related #67

@timvisee timvisee marked this pull request as draft November 12, 2020 11:12
@timvisee
Copy link
Author

Ping @jamesmunns (due to draft PR)

@timvisee
Copy link
Author

timvisee commented Nov 12, 2020

This uses fetch_or and fetch_add, do we need to implement this for thumbv6?

Apparently, yes: https://github.com/jamesmunns/bbqueue/runs/1390103704

Edit: this is now implemented

@timvisee timvisee force-pushed the separate-producer-consumer branch 2 times, most recently from 7ecf2de to 4a37dd9 Compare November 12, 2020 11:33
@jamesmunns
Copy link
Owner

Do we still need to zero the buffer when taking just the producer or consumer, like try_split? (currently commented-out)

Yes, I would suggest doing either on the first one that is taken, or doing it for the consumer alone is probably enough. Otherwise it is undefined behavior.

Do we still need to reinitialize the buffer when releasing just the producer or consumer, like try_release?

This is optional. It's generally not safe to do unless BOTH halves have been released. When in doubt, skip it.

timvisee added a commit to timvisee/bbqueue that referenced this pull request Nov 12, 2020
@timvisee
Copy link
Author

timvisee commented Nov 12, 2020

Do we still need to zero the buffer when taking just the producer or consumer, like try_split? (currently commented-out)

Yes, I would suggest doing either on the first one that is taken, or doing it for the consumer alone is probably enough. Otherwise it is undefined behavior.

Do we still need to reinitialize the buffer when releasing just the producer or consumer, like try_release?

This is optional. It's generally not safe to do unless BOTH halves have been released. When in doubt, skip it.

This has been implemented in de43a30

Zeroes on try_take_consumer. Reinitializes when both the producer and consumer are released.

@timvisee timvisee marked this pull request as ready for review November 13, 2020 08:58
@jamesmunns
Copy link
Owner

Hey @timvisee, I'll plan on reviewing this PR this weekend, but I realized that I was wrong - you'll need to zero-init the buffer when the PRODUCER (not consumer) is taken - as the producer will be the one to "see" the contents of the data first (e.g. if a producer is taken, then a grant is requested, the grant will be able to "see" uninitialized data if no consumer has been taken). Sorry for the wrong guidance before.

@timvisee
Copy link
Author

timvisee commented Dec 1, 2020

@jamesmunns It now zeroes when the producer is taken. I amended this change to the last commit (03b9774).

@ithinuel ithinuel force-pushed the separate-producer-consumer branch from 88ee375 to 9a0a2cf Compare November 27, 2023 00:13
@ithinuel
Copy link
Collaborator

Will conflict with #103

@Sympatron
Copy link
Collaborator

These conflicts should be resolvable though.

@timvisee If you still want this merged, you can rebase this on top of #103. Once #103 is merged, we could merge this too.

@timvisee
Copy link
Author

timvisee commented Dec 9, 2024

@Sympatron Thanks!

I'll rebase and reevaluate this PR later today. I did not keep track of all the recent changes and so this may very well not be valid anymore.

@Sympatron
Copy link
Collaborator

Some changes will be necessary, because #103 uses portable-atomic for cross platform atomics, but those should be minor and the overall goal of this PR should still be applicable.

@timvisee
Copy link
Author

timvisee commented Dec 9, 2024

Some changes will be necessary, because #103 uses portable-atomic for cross platform atomics, but those should be minor and the overall goal of this PR should still be applicable.

Just rebasing turned out to be more difficult than I had anticipated. I think it'd be better to implement this from scratch.

Sadly, I currently don't have the resources available to do so. If someone else wants to make the effort, please feel free!

I consider this PR stale and will therefore close it now.

@timvisee timvisee closed this Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow taking just Producer or Consumer
4 participants