Updates and clean-up cache management #502

iuliana-prodan · 2023-07-18T15:20:00Z

Please, check each commit.

cc: @arnopo @carlocaione @TanmayShah-xilinx

carlocaione

Instead of having different macros with different names doing the same thing can we have one single set of macros for flushing and invalidating?

lib/include/openamp/virtqueue.h

iuliana-prodan · 2023-07-18T15:34:32Z

Instead of having different macros with different names doing the same thing can we have one single set of macros for flushing and invalidating?

I totally agree, but TBH I don't know where to add them - in which header so it can be accessed by all?

carlocaione · 2023-07-18T15:36:14Z

I totally agree, but TBH I don't know where to add them - in which header so it can be accessed by all?

IMO if you cannot find a proper header for that just add a new one.

iuliana-prodan · 2023-07-18T15:37:01Z

I totally agree, but TBH I don't know where to add them - in which header so it can be accessed by all?

IMO if you cannot find a proper header for that just add a new one.

ok, I'll look again. Thanks!

iuliana-prodan · 2023-07-18T16:19:45Z

I totally agree, but TBH I don't know where to add them - in which header so it can be accessed by all?

IMO if you cannot find a proper header for that just add a new one.

I've add a new commit to fix this: 731e394

README.md

lib/include/openamp/virtqueue.h

carlocaione

I'm in general ok with this change but we must discuss whether we actually want to have a general VIRTIO_USE_CACHE switch or we want to have a more fine grained control over buffers placement.

carlocaione · 2023-07-28T13:24:59Z

cmake/options.cmake

+option (WITH_DCACHE "Build with all cache operations enabled" OFF)
+
+if (WITH_DCACHE)
+  add_definitions(-DVIRTIO_USE_CACHE)


Suggested change

add_definitions(-DVIRTIO_USE_CACHE)

add_definitions(-DVIRTIO_USE_DCACHE)

done. Thanks!

iuliana-prodan · 2023-08-07T10:58:31Z

I'm in general ok with this change but we must discuss whether we actually want to have a general VIRTIO_USE_CACHE switch or we want to have a more fine grained control over buffers placement.

IMO we should keep all 3, but see here: #502 (comment)
Also, in Zephyr we enable all 3 together with CONFIG_OPENAMP_WITH_DCACHE (https://github.com/zephyrproject-rtos/open-amp/blob/master/CMakeLists.txt#L20), not 1 by 1, so adding 1 for all seems reasonable.

arnopo · 2023-08-16T13:56:59Z

lib/include/openamp/remoteproc_virtio.h

-#define RSC_TABLE_INVALIDATE(x, s)	metal_cache_invalidate(x, s)
+#warning "VIRTIO_CACHED_RSC_TABLE is deprecated, please use VIRTIO_USE_CACHE"
+#endif
+#if defined(VIRTIO_CACHED_RSC_TABLE) || defined(VIRTIO_USE_CACHE)


you update VIRTIO_USE_CACHE to VIRTIO_USE_DCACHE in option.cmake but not in the source files

You're right. Thanks!
I fixed it!

arnopo · 2023-08-16T14:08:45Z

I'm in general ok with this change but we must discuss whether we actually want to have a general VIRTIO_USE_CACHE switch or we want to have a more fine grained control over buffers placement.

IMO we should keep all 3, but see here: #502 (comment) Also, in Zephyr we enable all 3 together with CONFIG_OPENAMP_WITH_DCACHE (https://github.com/zephyrproject-rtos/open-amp/blob/master/CMakeLists.txt#L20), not 1 by 1, so adding 1 for all seems reasonable.

@carlocaione
As mentioned in #502 (comment)
my proposal is:

One strategy would consists in making VIRTIO_CACHED_VRINGS, DVIRTIO_CACHED_BUFFERS and VIRTIO_CACHED_RSC_TABLE
deprecated. If nobody complains during 2 years we will clean them.

@xiaoxiang781216 :
Do you use cache management? If so, do you require fine control, or is one generic compilation configuration sufficent for your needs?

arnopo · 2023-09-07T13:25:39Z

@carlocaione @xiaoxiang781216 any comment/feedback on this PR?

carlocaione · 2023-09-07T13:52:49Z

@arnopo instead of deprecating WITH_DCACHE_VRINGS and WITH_DCACHE_BUFFERS why not simply leave those in place without deprecation and making WITH_DCACHE a more general switch implying the other two?

At least people could decide the lever of granularity to use.

arnopo · 2023-09-11T07:46:24Z

@arnopo instead of deprecating WITH_DCACHE_VRINGS and WITH_DCACHE_BUFFERS why not simply leave those in place without deprecation and making WITH_DCACHE a more general switch implying the other two?

At least people could decide the lever of granularity to use.

If possible, I would avoid multiplying the configs that complexify the support and increase the risk of bugs. Therefore, if no rational to keep them they should be removed.

carlocaione

I don't have any real problem with this. @iuliana-prodan please update https://github.com/zephyrproject-rtos/open-amp/blob/master/CMakeLists.txt and the zephyr open-amp repo when this is merged.

arnopo

LGTM

tnmysh · 2023-09-13T17:14:38Z

LGTM

arnopo · 2023-09-14T09:22:11Z

@iuliana-prodan
There is a merge conflict, could you rebase your patch on top of main branch please?

iuliana-prodan · 2023-09-14T09:23:33Z

@iuliana-prodan There is a merge conflict, could you rebase your patch on top of main branch please?

Sure

iuliana-prodan · 2023-09-14T09:53:32Z

Did the rebase and push the changes.
I see a build failure:

-- ZEPHYR_TOOLCHAIN_VARIANT not set, trying to locate Zephyr SDK
CMake Error at /github/workspace/zephyrproject/zephyr/cmake/modules/FindZephyr-sdk.cmake:109 (find_package):
  Could not find a configuration file for package "Zephyr-sdk" that is
  compatible with requested version "0.16".
-- Configuring incomplete, errors occurred!

  The following configuration files were considered but not accepted:

    /github/workspace/zephyr-sdk-0.15.0/cmake/Zephyr-sdkConfig.cmake, version: 0.15.0

It seems to me that is required zephyr-sdk 0.16 and the continuous integrations uses 0.15.

@arnopo is it possible to update the zephyr-sdk?
Can I do that?
How? :)

arnopo · 2023-09-14T13:14:52Z

Did the rebase and push the changes. I see a build failure:
-- ZEPHYR_TOOLCHAIN_VARIANT not set, trying to locate Zephyr SDK
CMake Error at /github/workspace/zephyrproject/zephyr/cmake/modules/FindZephyr-sdk.cmake:109 (find_package):
  Could not find a configuration file for package "Zephyr-sdk" that is
  compatible with requested version "0.16".
-- Configuring incomplete, errors occurred!

  The following configuration files were considered but not accepted:

    /github/workspace/zephyr-sdk-0.15.0/cmake/Zephyr-sdkConfig.cmake, version: 0.15.0
It seems to me that is required zephyr-sdk 0.16 and the continuous integrations uses 0.15.

@arnopo is it possible to update the zephyr-sdk? Can I do that? How? :)

The dsk version in the Ci need to be uopdated , I will do it

arnopo · 2023-09-14T14:18:08Z

Ci fixed in #504
Please, could you rebase another time ( sorry for the double work)

Fix typo for WITH_DCACHE_BUFFERS option. Signed-off-by: Iuliana Prodan <[email protected]>

Move VRING_FLUSH and VRING_INVALIDATE defines to header file. Signed-off-by: Iuliana Prodan <[email protected]>

Do buffers flush and invalidate the same as with vrings and resource table: - add defines in header file; - call BUFFER_FLUSH/BUFFER_INVALIDATE where necessary. Signed-off-by: Iuliana Prodan <[email protected]>

Since all cache operations, for vrings, buffer and resource table are using metal_cache_flush and metal_cache_invalidate, define a common function for all. Signed-off-by: Iuliana Prodan <[email protected]>

Add WITH_DCACHE operation used for all cache operations: vrings, buffers and resource table. The other options will be deprecated - add warning message for this. Add info for WITH_DCACHE option in README. Signed-off-by: Iuliana Prodan <[email protected]>

iuliana-prodan · 2023-09-14T16:45:14Z

Ci fixed in #504 Please, could you rebase another time ( sorry for the double work)

Done.
All good now! :)

iuliana-prodan · 2023-09-29T12:20:55Z

I don't have any real problem with this. @iuliana-prodan please update https://github.com/zephyrproject-rtos/open-amp/blob/master/CMakeLists.txt and the zephyr open-amp repo when this is merged.

@carlocaione Sorry for not doing this earlier, but... here it is: zephyrproject-rtos/open-amp#17
I cannot add any reviewers or anything.

iuliana-prodan force-pushed the updates-and-clean-up branch from ed5d27b to 581d0a3 Compare July 18, 2023 15:24

carlocaione suggested changes Jul 18, 2023

View reviewed changes

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

xiaoxiang781216 approved these changes Jul 18, 2023

View reviewed changes

arnopo requested review from edmooring, arnopo and tnmysh July 18, 2023 16:48

arnopo reviewed Jul 18, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Show resolved Hide resolved

carlocaione suggested changes Jul 18, 2023

View reviewed changes

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

iuliana-prodan force-pushed the updates-and-clean-up branch 3 times, most recently from b8d6ec3 to 9293a0b Compare July 24, 2023 10:49

iuliana-prodan requested review from xiaoxiang781216, carlocaione and arnopo July 26, 2023 07:41

carlocaione reviewed Jul 28, 2023

View reviewed changes

iuliana-prodan force-pushed the updates-and-clean-up branch from 9293a0b to 122570d Compare August 7, 2023 10:49

arnopo requested changes Aug 16, 2023

View reviewed changes

iuliana-prodan force-pushed the updates-and-clean-up branch from 122570d to 505dbba Compare August 16, 2023 14:10

arnopo self-requested a review August 17, 2023 07:28

iuliana-prodan requested a review from carlocaione August 17, 2023 07:53

carlocaione approved these changes Sep 11, 2023

View reviewed changes

arnopo approved these changes Sep 12, 2023

View reviewed changes

tnmysh approved these changes Sep 13, 2023

View reviewed changes

iuliana-prodan force-pushed the updates-and-clean-up branch from 505dbba to 8641e8b Compare September 14, 2023 09:32

iuliana-prodan added 5 commits September 14, 2023 19:24

options: fix typo

50dc259

Fix typo for WITH_DCACHE_BUFFERS option. Signed-off-by: Iuliana Prodan <[email protected]>

virtqueue: move VRING_FLUSH and VRING_INVALIDATE to header

ef91f0c

Move VRING_FLUSH and VRING_INVALIDATE defines to header file. Signed-off-by: Iuliana Prodan <[email protected]>

rpmsg: buffers flush/invalidate

03b7c76

Do buffers flush and invalidate the same as with vrings and resource table: - add defines in header file; - call BUFFER_FLUSH/BUFFER_INVALIDATE where necessary. Signed-off-by: Iuliana Prodan <[email protected]>

virtqueue: define a common cache flush/invalidate

99b9ed3

Since all cache operations, for vrings, buffer and resource table are using metal_cache_flush and metal_cache_invalidate, define a common function for all. Signed-off-by: Iuliana Prodan <[email protected]>

iuliana-prodan force-pushed the updates-and-clean-up branch from 8641e8b to 003311e Compare September 14, 2023 16:25

arnopo changed the title ~~Updates and clean-up~~ Updates and clean-up cache management Sep 18, 2023

arnopo merged commit f2162a6 into OpenAMP:main Sep 18, 2023
2 checks passed

arnopo added this to the Release V2023.10 milestone Oct 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates and clean-up cache management #502

Updates and clean-up cache management #502

iuliana-prodan commented Jul 18, 2023 •

edited

Loading

carlocaione left a comment

iuliana-prodan commented Jul 18, 2023

carlocaione commented Jul 18, 2023

iuliana-prodan commented Jul 18, 2023

iuliana-prodan commented Jul 18, 2023

carlocaione left a comment

carlocaione Jul 28, 2023

iuliana-prodan Aug 7, 2023

iuliana-prodan commented Aug 7, 2023

arnopo Aug 16, 2023

iuliana-prodan Aug 16, 2023

arnopo commented Aug 16, 2023

arnopo commented Sep 7, 2023

carlocaione commented Sep 7, 2023

arnopo commented Sep 11, 2023

carlocaione left a comment

arnopo left a comment

tnmysh commented Sep 13, 2023

arnopo commented Sep 14, 2023

iuliana-prodan commented Sep 14, 2023

iuliana-prodan commented Sep 14, 2023

arnopo commented Sep 14, 2023

arnopo commented Sep 14, 2023

iuliana-prodan commented Sep 14, 2023

iuliana-prodan commented Sep 29, 2023

	add_definitions(-DVIRTIO_USE_CACHE)
	add_definitions(-DVIRTIO_USE_DCACHE)

Updates and clean-up cache management #502

Updates and clean-up cache management #502

Conversation

iuliana-prodan commented Jul 18, 2023 • edited Loading

carlocaione left a comment

Choose a reason for hiding this comment

iuliana-prodan commented Jul 18, 2023

carlocaione commented Jul 18, 2023

iuliana-prodan commented Jul 18, 2023

iuliana-prodan commented Jul 18, 2023

carlocaione left a comment

Choose a reason for hiding this comment

carlocaione Jul 28, 2023

Choose a reason for hiding this comment

iuliana-prodan Aug 7, 2023

Choose a reason for hiding this comment

iuliana-prodan commented Aug 7, 2023

arnopo Aug 16, 2023

Choose a reason for hiding this comment

iuliana-prodan Aug 16, 2023

Choose a reason for hiding this comment

arnopo commented Aug 16, 2023

arnopo commented Sep 7, 2023

carlocaione commented Sep 7, 2023

arnopo commented Sep 11, 2023

carlocaione left a comment

Choose a reason for hiding this comment

arnopo left a comment

Choose a reason for hiding this comment

tnmysh commented Sep 13, 2023

arnopo commented Sep 14, 2023

iuliana-prodan commented Sep 14, 2023

iuliana-prodan commented Sep 14, 2023

arnopo commented Sep 14, 2023

arnopo commented Sep 14, 2023

iuliana-prodan commented Sep 14, 2023

iuliana-prodan commented Sep 29, 2023

iuliana-prodan commented Jul 18, 2023 •

edited

Loading