rpmsg: bring back zero copy transfer #162

xiaoxiang781216 · 2019-02-17T10:25:12Z

Two simplify the review, I split two small independent patch to new PR:
#240
#241

xiaoxiang781216 · 2019-02-17T10:40:50Z

@wjliang, this patch bring back @MarekNovakNXP's zero copy transfer, please review it.

lib/include/openamp/rpmsg.h

lib/rpmsg/rpmsg_virtio.c

wjliang · 2019-02-26T01:20:35Z

thanks for you patch, could you share your use case to have this feature?
From your implementation, you add restrictions to the internal implementation, e.g. you use fixed buffer size, this will limit the future extension. And thus, want to know the need to bring back this feature.

xiaoxiang781216 · 2019-02-26T04:16:09Z

thanks for you patch, could you share your use case to have this feature?

We have several drivers use this feature:
1.rpmsg uart driver for console/gps/modem
2.rpmsg syslog driver for printf/log redirection
3.rpmsg netdev driver to talk with onchip mac layer(modem/wifi/ethernet)
4.rpmsg hostfs driver for file API redirection
5.rpmsg usrsock driver for sock API redirection
6.rpmsg clk driver for clk API redirection
Since these drivers:
1.Transfer the huge or variable length packet
2.The data transfer is the critical performance path
zero-copy could avoid one extra memory copy and then boost the performance.

From your implementation, you add restrictions to the internal implementation, e.g. you use fixed buffer size, this will limit the future extension. And thus, want to know the need to bring back this feature.

Ok, I will refine the implementation to save the length in some reserved field as before.

wjliang · 2019-02-26T18:40:05Z

Ok, I will refine the implementation to save the length in some reserved field as before.

My concern is these rpmsg APIs may only work with single buffer. E.g. it will not work with chained buffers.
However, the existing RPMsg implementation only use single buffer. Maybe we can restrict those new APIs to single buffer only.

Also please also post the zero-copy to kernel RPMsg, as you are using the reserved fields from the RPMsg header.

Thanks for you use cases list, I just have some questions:

RPMsg netdev, have you considered virtio net?
this feature looks like it is for either a system with a very small memory, or for high message passing performance. For huge data, just curious (I haven't done performance measurement), don't you want the payload to be page aligned and cacheable in some cases?

xiaoxiang781216 · 2019-02-27T03:26:52Z

Ok, I will refine the implementation to save the length in some reserved field as before.

My concern is these rpmsg APIs may only work with single buffer. E.g. it will not work with chained buffers.
However, the existing RPMsg implementation only use single buffer. Maybe we can restrict those new APIs to single buffer only.

If we extend the rpmsg to use the chained buffer, we can extend this API too(e.g. return the chained buffer). Actually, each offical OpenAMP release isn't compatible with the previous one, so I think it isn't a big issue to change this API for the chained buffer.

Also please also post the zero-copy to kernel RPMsg, as you are using the reserved fields from the RPMsg header.

I will provide one, but I think this change is different from:
#155
#160
Since the reserved fields is reused as the temporary storage internally, it can work even the kernel side:
1.Reject the zero copy transfer enhancement
2.Kernel use the reserved to pass the side band info to remote

Thanks for you use cases list, I just have some questions:

RPMsg netdev, have you considered virtio net?

Yes, I have, but:
1.viritio support on OpenAMP is still not mature
2.netdev is used between two NuttX instance(not NuttX<->Linux), because NuttX:
a.Have the own and unique net driver model which is incompatible with Linux Kernel
b.NuttX don't support the virtio net driver at all.
3.virito net is used to simulate a ethernet card, but the backend of rpmsg netdev is real mac layer. So rpmsg netdev expose ioctl like interface to let the up net layer control/query the hardware depend feature.
So I have to define the netdev protocol on top of RPMsg. If the communication is between NuttX<->Linux, VirtioIO net maybe a better opiton.

this feature looks like it is for either a system with a very small memory, or for high message passing performance. For huge data, just curious (I haven't done performance measurement), don't you want the payload to be page aligned and cacheable in some cases?

I expect that the message embedded directly in rpmsg buffer is around 1KB, the huge data(several ten/hundred KB) should pass through as a pointer, because:
1.Many service run on rpmsg send the small packet, ony few of them need send the huge data in one transfer. It isn't efficient to reserve the big buffer for the rare usecase.
2.If the service need send the huge data, normally it's will use the special memory manage strategy(e.g. ION) and is impossible to push the data into the buffer directly.

xiaoxiang781216 · 2019-02-28T06:57:44Z

Rebase to the latest mainline, no function change.

xiaoxiang781216 · 2019-03-01T18:06:41Z

@wjliang fix the build break.

lib/rpmsg/rpmsg_virtio.c

wjliang

with minor comments

lib/rpmsg/rpmsg_virtio.c

arnopo

For me this patch answers to a limited use cases vs big data need:

DMA transfer for copies ( cyclic mode)
big buffers
buffer reuses (data processing on same buffer).
...
I don't want to block it but just a concerns vs a global solution base on a service on top of rpmsg...
i suppose that you define some shared buffers for the Sound Open Firmware use cases, and rpmsg are used for control, right?

Concerning the patch itself: seem correct. Just one remark:
Something may be missing to release all buffers on application crash or on service destruction ( local or remote). But this would means that reserved buffers should be associated to the ept...

wjliang · 2019-03-13T00:25:20Z

Concerning the patch itself: seem correct. Just one remark:
Something may be missing to release all buffers on application crash or on service destruction ( local or remote). But this would means that reserved buffers should be associated to the ept...

Good point, just in some cases of crashing, one end may not even able to do any cleaning.
Do you think it good to make this as another OpenAMP issue for recovery from failure?

xiaoxiang781216 · 2019-03-13T07:55:21Z

For me this patch answers to a limited use cases vs big data need:

DMA transfer for copies ( cyclic mode)

big buffers

buffer reuses (data processing on same buffer).
...

Since many above cases normally involve the dedicated memory management, I expect:
1.Use rpmsg to pass the pointer to these special memory block or
2.Develop the driver on virtio layer directly to remove the buffer management limitation imposed by rpmsg.

I don't want to block it but just a concerns vs a global solution base on a service on top of rpmsg...
i suppose that you define some shared buffers for the Sound Open Firmware use cases, and rpmsg are used for control, right?

Yes, the audio data buffer is preallocated after the hardware parameter is lock down and recycle in the whole playback/capture stage. The buffer location exchange when the state change from stop/pause to run and read/write directly by DSP, from that the rpmsg is just used to notify the buffer position, no real audo data send/receive through IPC.

Concerning the patch itself: seem correct. Just one remark:
Something may be missing to release all buffers on application crash or on service destruction ( local or remote). But this would means that reserved buffers should be associated to the ept...

Yes, it's a problem if application get a buffer but crash before return back. But since the memory protection is very weak for most OpenAMP usage, the action can be taken is to reboot MCU/DSP if some application crash, so what we need here is to teardown all resource and restart if the peer happen reboot.

xiaoxiang781216 · 2019-03-13T08:32:44Z

@arnop2, the new pull request split the change into two patches.

arnopo · 2019-03-13T09:21:03Z

protection is very weak for most OpenAMP usage, the action can be taken is to reboot MCU/DSP if some application crash, so what we need here is to teardown all resource and restart if the peer happen reboot.
This could be OK for a MCU/DSP dedicated for a feature. But OpenAMP can also run on the main MPU or MCU processor... with multi services on top of RPMsg. In this case you would have to stop all the services and reinit the rpmsg.

lib/rpmsg/rpmsg_virtio.c

xiaoxiang781216 · 2020-11-01T15:04:34Z

@arnopo the sgement fault is fixed. @kr-satish, thanks for your help.

kr-satish · 2020-11-02T08:34:45Z

@arnopo the sgement fault is fixed. @kr-satish, thanks for your help.

@xiaoxiang781216 Thanks for the patches.

arnopo · 2020-11-03T18:40:52Z

From my point of view the feature and associated patch are Ok.
Now the remaining things is to limit the increase of the library footprint that is also a strong requirement for low memory systems.
I'm going to make an assessment of the memory footprint based on Zephyr sample to evaluate the impact.
i suppose that the addition of new ops in rpmsg_device_ops will be the main contributor of the footprint increase...

If you have some suggestions to minimize the impact don't hesitate...

arnopo · 2020-11-04T18:21:06Z

I performed measurements for some zehyr samples, building for the NXP expresso and STm32MP1 platforms.
The compiler is GCC with the optimization flag -Os.

I note a library increase of about 10% for the ROM and 16 bytes for the RAM.
That's seems to me very reasonable.

on: https://github.com/zephyrproject-rtos/zephyr/tree/master/samples/subsys/ipc/openamp
target: lpcxpresso54114_m4 (master)

w/o zero copy:

libmetal:	994 bytes 
openamp:	2378 bytes

w zero copy:

libmetal:	994 bytes 
openamp:	2664 bytes (+286)

Application: https://github.com/zephyrproject-rtos/zephyr/tree/master/samples/subsys/ipc/openamp/remote
target: lpcxpresso54114_m0 (slave)

w/o zero copy:

libmetal:	806 bytes 
openamp:	2300 bytes

w zero copy:

libmetal:	806 bytes 
openamp:	2552 bytes (+252)

Application: /https://github.com/zephyrproject-rtos/zephyr/tree/master/samples/subsys/ipc/openamp_rsc_table
target: stm32mp157c_dk2(slave)

w/o zero copy:

libmetal:	968 bytes 
openamp:	2696 bytes

w zero copy:

libmetal:	968 bytes 
openamp:	2868 bytes (+172)

xiaoxiang781216 · 2020-11-05T02:58:20Z

The absolute number is from 172 bytes to 252 bytes. Should we still need add a macro to enable/disable this feature?

arnopo · 2020-11-05T17:53:37Z

The absolute number is from 172 bytes to 252 bytes. Should we still need add a macro to enable/disable this feature?

I tested using macros, which saves about half the extra footprint but adds complexity.
From my point of view, I'll leave it like that and if optimization is needed later, we'll take care of it if necessary.

arnopo · 2020-11-05T17:59:09Z

Hi @galak
As the footprint is also a concern for the Zephyr project, could you give your opinion?
Is the increase of the memory footprint, is reasonable from your point of view?

xiaoxiang781216 · 2020-11-12T13:18:15Z

@arnopo how about we merge this PR now? so it get more time to test before release.

arnopo · 2020-11-12T14:23:22Z

@arnopo how about we merge this PR now? so it get more time to test before release.

I waiting @edmooring and @galak feedback first. As everyone is busy on there own stuff we need to allow a reasonable time for review.

arnopo · 2020-11-24T16:04:01Z

Hi @galak, @edmooring
Gentle reminder :-)

edmooring

If @galak is OK with this, I think we're good to go.

Commit-id: b16ca55 Adding RPMsg Extension layer implementing zero-copy send and receive. Signed-off-by: Chao An <anchao@pinecone.net>

to demo the usage of zero copy API Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>

arnopo · 2020-12-04T13:17:15Z

Hi @xiaoxiang781216
I merged the series with 2 fixes of minor "check" point reported by the check-patch.
Thanks a lot for this major contribution!

wjliang reviewed Feb 26, 2019

View reviewed changes

lib/include/openamp/rpmsg.h Outdated Show resolved Hide resolved

wjliang reviewed Feb 26, 2019

View reviewed changes

lib/include/openamp/rpmsg.h Outdated Show resolved Hide resolved

wjliang reviewed Feb 26, 2019

View reviewed changes

lib/rpmsg/rpmsg_virtio.c Outdated Show resolved Hide resolved

xiaoxiang781216 force-pushed the zero-copy branch 2 times, most recently from 6804841 to 4b29dcd Compare February 28, 2019 06:54

xiaoxiang781216 force-pushed the zero-copy branch from 4b29dcd to f3d7efe Compare March 1, 2019 10:33

xiaoxiang781216 closed this Mar 1, 2019

xiaoxiang781216 reopened this Mar 1, 2019

xiaoxiang781216 force-pushed the zero-copy branch 2 times, most recently from cf04b18 to 5f2287c Compare March 1, 2019 17:51

wjliang reviewed Mar 3, 2019

View reviewed changes

lib/rpmsg/rpmsg_virtio.c Outdated Show resolved Hide resolved

wjliang reviewed Mar 3, 2019

View reviewed changes

lib/rpmsg/rpmsg_virtio.c Outdated Show resolved Hide resolved

wjliang requested review from galak and arnopo March 3, 2019 08:02

xiaoxiang781216 force-pushed the zero-copy branch 2 times, most recently from c24071d to 5e3ed51 Compare March 4, 2019 09:43

wjliang approved these changes Mar 10, 2019

View reviewed changes

lib/rpmsg/rpmsg_virtio.c Show resolved Hide resolved

arnopo reviewed Mar 12, 2019

View reviewed changes

xiaoxiang781216 force-pushed the zero-copy branch from 5e3ed51 to 36cd627 Compare March 13, 2019 08:30

kr-satish reviewed Oct 28, 2020

View reviewed changes

lib/rpmsg/rpmsg_virtio.c Show resolved Hide resolved

xiaoxiang781216 changed the title ~~[DON'T MERGE] rpmsg: bring back zero copy transfer~~ rpmsg: bring back zero copy transfer Nov 1, 2020

xiaoxiang781216 marked this pull request as draft November 1, 2020 03:29

xiaoxiang781216 mentioned this pull request Nov 1, 2020

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

Merged

xiaoxiang781216 force-pushed the zero-copy branch 2 times, most recently from 31c6a30 to a05f227 Compare November 1, 2020 14:48

xiaoxiang781216 marked this pull request as ready for review November 1, 2020 15:00

arnopo self-requested a review November 3, 2020 18:23

arnopo approved these changes Nov 5, 2020

View reviewed changes

xiaoxiang781216 force-pushed the zero-copy branch 2 times, most recently from 8649c90 to e0f42a4 Compare November 9, 2020 16:45

edmooring approved these changes Nov 25, 2020

View reviewed changes

anchao and others added 2 commits December 1, 2020 21:02

rpmsg: bring back zero copy transfer

fcad657

Commit-id: b16ca55 Adding RPMsg Extension layer implementing zero-copy send and receive. Signed-off-by: Chao An <anchao@pinecone.net>

apps: add msg-test-rpmsg-nocopy-ping test

cad86c7

to demo the usage of zero copy API Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>

xiaoxiang781216 force-pushed the zero-copy branch from e0f42a4 to cad86c7 Compare December 1, 2020 13:02

arnopo linked an issue Dec 3, 2020 that may be closed by this pull request

Add support for zero copy send & receive #97

Closed

arnopo closed this Dec 4, 2020

xiaoxiang781216 deleted the zero-copy branch December 4, 2020 16:57

doki-nordic mentioned this pull request Aug 9, 2021

sm_ipt: Add Shared Memory Inter-Processor Transport nrfconnect/sdk-nrfxlib#524

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpmsg: bring back zero copy transfer #162

rpmsg: bring back zero copy transfer #162

xiaoxiang781216 commented Feb 17, 2019 •

edited

Loading

xiaoxiang781216 commented Feb 17, 2019

wjliang commented Feb 26, 2019

xiaoxiang781216 commented Feb 26, 2019

wjliang commented Feb 26, 2019

xiaoxiang781216 commented Feb 27, 2019 •

edited

Loading

xiaoxiang781216 commented Feb 28, 2019

xiaoxiang781216 commented Mar 1, 2019

wjliang left a comment

arnopo left a comment

wjliang commented Mar 13, 2019

xiaoxiang781216 commented Mar 13, 2019 •

edited

Loading

xiaoxiang781216 commented Mar 13, 2019

arnopo commented Mar 13, 2019

xiaoxiang781216 commented Nov 1, 2020

kr-satish commented Nov 2, 2020

arnopo commented Nov 3, 2020

arnopo commented Nov 4, 2020

on: https://github.com/zephyrproject-rtos/zephyr/tree/master/samples/subsys/ipc/openamp

Application: https://github.com/zephyrproject-rtos/zephyr/tree/master/samples/subsys/ipc/openamp/remote

Application: /https://github.com/zephyrproject-rtos/zephyr/tree/master/samples/subsys/ipc/openamp_rsc_table

xiaoxiang781216 commented Nov 5, 2020

arnopo commented Nov 5, 2020

arnopo commented Nov 5, 2020

xiaoxiang781216 commented Nov 12, 2020

arnopo commented Nov 12, 2020

arnopo commented Nov 24, 2020

edmooring left a comment

arnopo commented Dec 4, 2020

rpmsg: bring back zero copy transfer #162

rpmsg: bring back zero copy transfer #162

Conversation

xiaoxiang781216 commented Feb 17, 2019 • edited Loading

xiaoxiang781216 commented Feb 17, 2019

wjliang commented Feb 26, 2019

xiaoxiang781216 commented Feb 26, 2019

wjliang commented Feb 26, 2019

xiaoxiang781216 commented Feb 27, 2019 • edited Loading

xiaoxiang781216 commented Feb 28, 2019

xiaoxiang781216 commented Mar 1, 2019

wjliang left a comment

Choose a reason for hiding this comment

arnopo left a comment

Choose a reason for hiding this comment

wjliang commented Mar 13, 2019

xiaoxiang781216 commented Mar 13, 2019 • edited Loading

xiaoxiang781216 commented Mar 13, 2019

arnopo commented Mar 13, 2019

xiaoxiang781216 commented Nov 1, 2020

kr-satish commented Nov 2, 2020

arnopo commented Nov 3, 2020

arnopo commented Nov 4, 2020

on: https://github.com/zephyrproject-rtos/zephyr/tree/master/samples/subsys/ipc/openamp

w/o zero copy:

w zero copy:

Application: https://github.com/zephyrproject-rtos/zephyr/tree/master/samples/subsys/ipc/openamp/remote

w/o zero copy:

w zero copy:

Application: /https://github.com/zephyrproject-rtos/zephyr/tree/master/samples/subsys/ipc/openamp_rsc_table

w/o zero copy:

w zero copy:

xiaoxiang781216 commented Nov 5, 2020

arnopo commented Nov 5, 2020

arnopo commented Nov 5, 2020

xiaoxiang781216 commented Nov 12, 2020

arnopo commented Nov 12, 2020

arnopo commented Nov 24, 2020

edmooring left a comment

Choose a reason for hiding this comment

arnopo commented Dec 4, 2020

xiaoxiang781216 commented Feb 17, 2019 •

edited

Loading

xiaoxiang781216 commented Feb 27, 2019 •

edited

Loading

xiaoxiang781216 commented Mar 13, 2019 •

edited

Loading