Skip to content

Conversation

@frozencemetery
Copy link
Member

@frozencemetery frozencemetery commented Oct 9, 2020

Always service timerfd in gpm_epoll_wait()

This may prevent a tight loop around epoll_wait() when both fire.

Signed-off-by: Robbie Harwood <rharwood@redhat.com>
Copy link
Contributor

@cipherboy cipherboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're here, let's initialize ret to something (EINVAL?). I think it'll help make sure we prevent errors based on uninitialized ret values in the future since this is a huge method.

@simo5
Copy link
Contributor

simo5 commented Oct 12, 2020

@frozencemetery can you add a description that explains what this PR is trying to do ?

@frozencemetery
Copy link
Member Author

@simo5, applied description from the second commit. First commit is a refactor.

This may prevent a tight loop around epoll_wait() when both fire.

Signed-off-by: Robbie Harwood <rharwood@redhat.com>
@cipherboy
Copy link
Contributor

(Per IRC), this addresses the feedback I had last time. I won't have time to re-review this as much as I did earlier.

Copy link
Contributor

@simo5 simo5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@simo5
Copy link
Contributor

simo5 commented Oct 26, 2020

@frozencemetery are you planning to add anything else?
If not feel free to merge.

@frozencemetery
Copy link
Member Author

I think we were waiting to hear back if this fixed the issue before merging. (I haven't heard back yet.)

@simo5
Copy link
Contributor

simo5 commented Feb 25, 2021

@frozencemetery have you heard back yet ?

@frozencemetery
Copy link
Member Author

Yes, thanks for the reminder. This did not fix the issue in question - I don't know whether it's better to merge this anyway or to drop it.

@simo5
Copy link
Contributor

simo5 commented Feb 25, 2021

Do you think this improves the code in other ways?
If it does and it is not likely to cause any side issues I would merge, otherwise I would drop.

@frozencemetery
Copy link
Member Author

I think so - if nothing else, there are more comments explaining what's going on.

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.

3 participants