Skip to content

[Hexagon] Handle TCP server binding to unknown port#10945

Merged
mehrdadh merged 1 commit intoapache:mainfrom
kparzysz-quic:hexagon-rpc-server-port
Apr 14, 2022
Merged

[Hexagon] Handle TCP server binding to unknown port#10945
mehrdadh merged 1 commit intoapache:mainfrom
kparzysz-quic:hexagon-rpc-server-port

Conversation

@kparzysz-quic
Copy link
Contributor

@kparzysz-quic kparzysz-quic commented Apr 8, 2022

The server IP address will be obtained from the RPC tracker, but multiple servers must be distinguishable. To enable this, set a unique key when starting a server, and use that key when starting a session.

cc @mehrdadh

The server IP address will be obtained from the RPC tracker, but multiple
servers must be distinguishable. To enable this, set a unique key when
starting a server, and use that key when starting a session.
@github-actions github-actions bot requested a review from mehrdadh April 8, 2022 21:08
@kparzysz-quic
Copy link
Contributor Author

To add more context here: the RPC server will connect to a first available port in some range (which typically starts at 7070). If there are two servers running on the same machine, one of them will get 7070, and the other 7071. The clients will not know the actual port, instead they will query the RPC tracker for a server with a given device key. Since the Hexagon RPC server always used the same key, the tracker could not distinguish between the one bound to 7070, and the one bound to 7071. To avoid this ambiguity, make the device key unique (by attaching the PID of the process creating it to it).

@kparzysz-quic
Copy link
Contributor Author

cc: @masahi

Copy link
Member

@mehrdadh mehrdadh left a comment

Choose a reason for hiding this comment

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

Overall LGTM, only one comment about setting the default key

}
self._rpc_info.update(rpc_info)
self._workspace = self._create_workspace(workspace)
self._device_key = self.HEXAGON_REMOTE_DEVICE_KEY
Copy link
Member

@mehrdadh mehrdadh Apr 11, 2022

Choose a reason for hiding this comment

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

wouldn't this always set the key to be self.HEXAGON_REMOTE_DEVICE_KEY? My understanding is that the goal is to be able to set the key to distinguish RPC server between different tests/runs?

Copy link
Contributor Author

@kparzysz-quic kparzysz-quic Apr 11, 2022

Choose a reason for hiding this comment

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

You're right: the idea is to get the PID of the process that starts the server. The launcher itself may be created once ahead of time, and then we'd have the same PID for every server. Since the base class only has an abstract method start_server, the appending of the PID is done in the actual implementations. I'm setting the member _device_key, here so that it exists in the object.

At the moment, the PID is only appended for the simulator. Do you think we should do that for Android as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have any additional comments? Should I add this for Android as well, or is it ok as it is?

Copy link
Member

Choose a reason for hiding this comment

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

This is okay as it is. Not sure if this could solve some of the existing issues with test flakiness on Android. We can consider it later.

@mehrdadh mehrdadh self-assigned this Apr 14, 2022
Copy link
Member

@mehrdadh mehrdadh left a comment

Choose a reason for hiding this comment

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

Thanks!

@mehrdadh mehrdadh merged commit 985fc93 into apache:main Apr 14, 2022
@kparzysz-quic kparzysz-quic deleted the hexagon-rpc-server-port branch April 14, 2022 14:24
Lucien0 pushed a commit to Lucien0/tvm that referenced this pull request Apr 19, 2022
The server IP address will be obtained from the RPC tracker, but multiple
servers must be distinguishable. To enable this, set a unique key when
starting a server, and use that key when starting a session.
altanh pushed a commit to altanh/tvm that referenced this pull request Apr 28, 2022
The server IP address will be obtained from the RPC tracker, but multiple
servers must be distinguishable. To enable this, set a unique key when
starting a server, and use that key when starting a session.
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.

2 participants