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

[dv/dpi] tcpserver single connection and retry select on EINTR #23889

Open
wants to merge 2 commits into
base: integrated_dev
Choose a base branch
from

Conversation

kwalker27
Copy link

@kwalker27 kwalker27 commented Jul 2, 2024

Two small patches to the tcp_server code, used by other transactors such as jtagdpi and dmidpi. These improve the usability of the transactors in a shared, remote environment such as an emulator.

Without this change, an interrupted select() call is treated as a connection error and the client is dropped. This makes connections very unstable.

Instead of asserting in `client_tryaccept` when a client is already
attached, reject the new connection by accepting and immediately
closing.

Signed-off-by: Kip Walker <[email protected]>
@kwalker27 kwalker27 requested a review from a team as a code owner July 2, 2024 19:22
@kwalker27 kwalker27 requested review from hcallahan-lowrisc and removed request for a team July 2, 2024 19:22
@kwalker27
Copy link
Author

heads up @sameo

@sameo sameo requested review from rswarbrick and matutem July 2, 2024 19:46
@sameo sameo added the Component:DV DV issue: testbench, test case, etc. label Jul 3, 2024
Copy link
Contributor

@rswarbrick rswarbrick left a comment

Choose a reason for hiding this comment

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

Sorry for the very slow review. This looks sensible, but I think the second commit could be tweaked slightly.

Comment on lines 355 to 357
// On interrupt we want to retry; according to the man page
// the timeout is now undefined.
timeout.tv_sec = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not completely sure about this bit. If I understand the man page correctly, it is just saying that the function might have trashed timeout (see the section called "The timeout").

So I don't think we need to do this change, but we should probably set all of timeout every time, so just need to move the write to tv_sec inside the loop.

Copy link
Contributor

Choose a reason for hiding this comment

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

Duh. I've just re-read your change and now I understand what you're doing! I think it would still be much simpler to move this line to appear just before the write to tv_usec. I think the existing code is a bit silly (because timeout.tv_sec might get trashed even if the select call succeeds), but the fix is very easy.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback, I pushed an update.

Copy link
Author

Choose a reason for hiding this comment

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

(oops, pushed again to remove the now-extraneous initial init of tv_sec).

When select returns -1 and errno is EINTR, the select should just be
retried instead of treating it as an error and disconnecting the
client.

Also move the timeout initialization into the loop right before
calling select, since it can get trashed.

Signed-off-by: Kip Walker <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component:DV DV issue: testbench, test case, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants