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

Add flow control service #394

Closed
wants to merge 6 commits into from
Closed

Add flow control service #394

wants to merge 6 commits into from

Conversation

arnopo
Copy link
Collaborator

@arnopo arnopo commented May 20, 2022

This series proposes an implementation for the rpmsg virtio transport backend, of the signaling API proposed by Deepak Kumar Singh on Linux:
"rpmsg and glink signaling API support" [1]

The aim of the pull request is to offer the possibility for an endpoint to inform a remote endpoint about its state, based on a software flow control[2].

For this a new rpmsg service( with a fixed address 64) is proposed.
It is responsible for:

  • transmitting local endpoint flow control information to the remote side,
  • informing a local endpoint about a remote endpoint flow control.

For the rpmsg virtio transport layer the service is negotiated thanks to the virtio feature flag: VIRTIO_RPMSG_F_FC

Notice that this pull request introduces new feature in the rpmsg protocol, So it has to be aligned with Linux implementation.
For this a pull request will sent to the linux remoteproc mailing to implement pending feature.

[1]https://lkml.org/lkml/2022/1/18/867
[2]https://en.wikipedia.org/wiki/Software_flow_control

Create a dedicated channel for the endpoints flow control.
This channel implements the software control allowing to:
- inform the remote side that it is ready or not to communicate,
- be informed by the remote endpoint that it can start or must
  stop to send rpmsg.

In addition the channel can help to bind the local and the remote
endpoint after a NS announcement as it set the local endpoint
dest_addr if is current value is RPMSG_ADDR_ANY.

To enable the feature the VIRTIO_RPMSG_F_FC must be set in the
vdev structure in the resource table.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
Increase readability by creating sub-chapter for endpoint and device
callbacks.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
Add documentation on the flow control callback and API to
inform the remote side about an endpoint state.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
If the virtio rpmsg device feature VIRTIO_RPMSG_F_FC is set, register
the rpmsg flow control endpoint.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
If the WITH_RPMSG_FLOW_CONTROL config is defined:
- set the VIRTIO_RPMSG_F_FC bit in the resource tables,
- return an error if the remote side does not support it.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
Add an example of the flow control usage. The host:
- waits the reception of a flow control RPMSG_EPT_FC_ON
  before starting to send message,
- pauses when it receive a !RPMSG_EPT_FC_ON flow control.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
@arnopo
Copy link
Collaborator Author

arnopo commented May 20, 2022

The series sent on Linux mailing list is available here:
https://lore.kernel.org/lkml/20220520082940.2984914-1-arnaud.pouliquen@foss.st.com/T/#t


int rpmsg_set_flow_control(struct rpmsg_endpoint *ept, uint32_t flags)
{
struct rpmsg_ept_msg msg;
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove the extra space before msg

* An NS announcement has been sent to the remote side which answer with flow control.
* Register the remote endpoint as default destination endpoint.
*/
if (_ept->dest_addr == RPMSG_ADDR_ANY)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this doesn't resolve the first creator of endpoint want to notify the flow info initially.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree, NS_ack would help for this


msg.flags = flags;
msg.src = ept->addr;
msg.dst = ept->dest_addr;
Copy link
Collaborator

Choose a reason for hiding this comment

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

As I mention in other PR, the separated flow control isn't good because:

  1. it isn't efficient to send a dedicated message for the flow control, since the same info could piggyback as a flag field in normal data transfer in most case.
  2. ON/OFF info isn't enough in the advanced flow control since the additional info is required(e.g. the slide window, round trip delay, congestion etc..).

So I would expect the flow control is part of the protocol design, and the general flow control service is useless.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for your feedback, could please send your feedback on Linux mailing list instead. As this feature impact both, the Linux and the OpenAMP. Having a single place would ease discussion.

If you have not subscribed to the Linux remote proc mailing list you can get the "mbox" on patchwork
https://patchwork.kernel.org/project/linux-remoteproc/list/?series=643501

@github-actions
Copy link

This pull request has been marked as a stale pull request because it has been open (more than) 45 days with no activity.

@github-actions github-actions bot added the Stale label Oct 15, 2023
@github-actions github-actions bot closed this Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants