-
Notifications
You must be signed in to change notification settings - Fork 847
Remove usage of volatile keyword #2603
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,10 +70,10 @@ typedef struct io_event ink_io_event_t; | |
| #else | ||
|
|
||
| struct ink_aiocb { | ||
| int aio_fildes = 0; | ||
| volatile void *aio_buf = nullptr; /* buffer location */ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be an atomic?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All this patch is doing is removing volatile, which did not give us thread safety. So this shouldn't really change anything with regard to that. If it should be atomic, that might be another issue.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @PSUdaemon We should investigate why each of the variables were volatile and covert them to atomic in this PR. Having the changes to atomics in another PR would make it harder to see what needs/did change to atomic.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bryancall Should I do the conversions with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @SolidWallOfCode I don't think
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @PSUdaemon I would use std::atomic
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good, cause that's what I did :)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think he means on that line. Is this variable shared between threads?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dragon512 no, it's just a member of a struct for AIO. Each AIO operation should have its own struct. There is a lot of "Oh, we are going to update this then something else has to read it. Better make it volatile so the other thing sees the new data" in the code. Totally useless AFAICT.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ya this should be removed... it slowing the code down by forcing register load in the pipeline. |
||
| size_t aio_nbytes = 0; /* length of transfer */ | ||
| off_t aio_offset = 0; /* file offset */ | ||
| int aio_fildes = 0; | ||
| void *aio_buf = nullptr; /* buffer location */ | ||
| size_t aio_nbytes = 0; /* length of transfer */ | ||
| off_t aio_offset = 0; /* file offset */ | ||
|
|
||
| int aio_reqprio = 0; /* request priority offset */ | ||
| int aio_lio_opcode = 0; /* listio operation */ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I'm not sure about changing unsigned long to uint64_t. Are we sure unsigned long is not uint32_t on any system that TS runs on?
If you're on a 32-bit architecture, with no bus-locking 64-bit load/store instructions, I'm pretty sure using std::atomic<uint64_t> will generate code with retry loops.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a) We don't support 32bit
b) This is an example plugin and that is just a counter for monotonically increasing values
TBH, I was sort of just being cute by making that change. There was a comment that said "use a mutex" or similar, so I just added atomics.