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

rpmsg: virito: limit the buffer allocate from shared memory pool #241

Merged
merged 1 commit into from
Nov 9, 2020

Conversation

xiaoxiang781216
Copy link
Collaborator

rpmsg_virtio_get_tx_buffer shouldn't allocate the number of
buffer bigger than the virtio ring length of sending
report here: #162 (comment)

rpmsg_virtio_get_tx_buffer shouldn't allocate the number of
buffer bigger than the virtio ring length of sending

Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
@arnopo
Copy link
Collaborator

arnopo commented Nov 3, 2020

This issue seems quite old and related to the first introduction of the zero copy that introduce index (b16ca55)...

Here I would be agree with @kr-satish #162 (comment):
"all buffers should be created at the time of initialization".

I think in a first step we have to figure out if adding new buffer in TX for master, if no more buffer is available, is a good strategy...
I had a look to Linux implementation and i don't see such approach...

@wjliang @MarekNovakNXP and @eanjum: I would like to require your memory for this topic...

  • As you have the legacy, do you know the reason of such implementation?
  • Do you think that it is still needed, or could we cleanup this implementation?

@xiaoxiang781216
Copy link
Collaborator Author

No, Linux use the similar strategy:
https://github.com/torvalds/linux/blob/master/drivers/rpmsg/virtio_rpmsg_bus.c#L447-L468
The difference is how to determine whether to allocate memory from the pool. Linux use last_sbuf, OpenAMP depend on the returned value from rpmsg_virtio_shm_pool_get_buffer before, now use vq_free_cnt.

Copy link
Collaborator

@arnopo arnopo left a comment

Choose a reason for hiding this comment

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

I re-parsed the code to better understood the impact of not limiting the allocation. I think you are right, allocating more buffer than the number of entry of the vrings is a bug.
And i don't see any code that try to extend the TX vrings...
So LGTM

Thanks,

@arnopo arnopo added this to the Release V2021.04 milestone Nov 4, 2020
Copy link
Contributor

@edmooring edmooring left a comment

Choose a reason for hiding this comment

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

This looks good. We really shouldn't be trying to allocate more buffers than there are entries in the ring.

@arnopo arnopo merged commit 38aed4a into OpenAMP:master Nov 9, 2020
@xiaoxiang781216 xiaoxiang781216 deleted the pool branch November 9, 2020 16:32
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