[dv/dpi] tcp_server_create changes #25646
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When the tcp_server is unable to create its listening socket, the simulation now exits. Previously, an error would be printed but the simulation would continue. Clients like dmidpi and jtagdpi should only call tcp_server_create if there's a chance a client will connect, and if the listening port number is being selected to avoid conflict with other processes on the same machine. Signed-off-by: Kip Walker <[email protected]>
kwalker27
requested review from
hcallahan-lowrisc,
rswarbrick,
Razer6 and
andreaskurth
and removed request for
a team
The listening socket creation is made synchronous with the tcp_server_create call, while the processing of socket events is still handled on a dedicated thread. Signed-off-by: Kip Walker <[email protected]>
kwalker27
force-pushed
the
tcp_server-fail
branch
from
9b45c39 to
6b0a9e3
Compare
kwalker27
changed the title
|
The regression failure is what I was afraid of: So, I'd like to hear opinions on whether the suggestion in this PR makes sense or not - and if it does, this work needs to be deferred until (1) dmidpi server setup is made conditional and (2) port collisions are avoided somehow (dynamic port selection?) when a In the current state of things, a user running a single sim on a local machine gets the benefit of |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm proposing to make it a fatal error to fail to open the listening socket rather than simply continuing the simulation after printing an error. My rationale is that it would be better to intentionally open the socket (and optionally provide a mechanism to make the port unique, as was recently done for
jtagdpi), instead of the current "best effort" approach.The changes here: (1) make the socket setup synchronous to the
create()call while leaving the processing on a dedicated pthread and (2) assert if the setup fails.I expect there could be fallout from this (in CI sim jobs, for example) since
dmidpicurrently starts the server unconditionally and uses a static port number (set via SV parameter). So, I expect there might need to be additional changes after discussion whether folks consider whether this policy change is sensible or not.