Skip to content

Conversation

@zdtyuiop4444
Copy link
Contributor

@zdtyuiop4444 zdtyuiop4444 commented Nov 25, 2024

拉取/合并请求描述:(PR description)

[

为什么提交这份PR (why to submit this PR)

由于小核默认开启了D-Cache且没有缓存一致性保证,所以在使用共享内存时会出现大小核读取的数据不一致的情况

你的解决方案是什么 (what is your solution)

从官方的duo-buildroot-sdk库中迁移D-Cache相关函数,这样就可以选择关闭D-Cache或在读前写后调用flush_dcache_range函数进行同步

请提供验证的bsp和config (provide the config and bsp)

  • BSP: bsp/cvitek/c906_little/board

  • .config:

  • action:

]

当前拉取/合并请求的状态 Intent for your PR

必须选择一项 Choose one (Mandatory):

  • 本拉取/合并请求是一个草稿版本 This PR is for a code-review and is intended to get feedback
  • 本拉取/合并请求是一个成熟版本 This PR is mature, and ready to be integrated into the repo

代码质量 Code Quality:

我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:

  • 已经仔细查看过代码改动的对比 Already check the difference between PR and old code
  • 代码风格正确,包括缩进空格,命名及其他风格 Style guide is adhered to, including spacing, naming and other styles
  • 没有垃圾代码,代码尽量精简,不包含#if 0代码,不包含已经被注释了的代码 All redundant code is removed and cleaned up
  • 所有变更均有原因及合理的,并且不会影响到其他软件组件代码或BSP All modifications are justified and not affect other components or BSP
  • 对难懂代码均提供对应的注释 I've commented appropriately where code is tricky
  • 代码是高质量的 Code in this PR is of high quality
  • 已经使用formatting 等源码格式化工具确保格式符合RT-Thread代码规范 This PR complies with RT-Thread code specification
  • 如果是新增bsp, 已经添加ci检查到.github/workflows/bsp_buildings.yml 详细请参考链接BSP自查

@github-actions github-actions bot added BSP BSP: Cvitek BSP related with cvitek labels Nov 25, 2024
#include <stdint.h>
#include <sys/types.h>

void flush_dcache_range(uintptr_t addr, size_t size);
Copy link
Member

Choose a reason for hiding this comment

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

void rt_hw_cpu_icache_enable(void);

可以实现成cache的ops

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已经修改成了ops的形式


#include "cache.h"

inline void rt_hw_cpu_dcache_enable(void)
Copy link
Member

Choose a reason for hiding this comment

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

是否也有icache操作?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

根据数据手册添加了icache相关操作,修改了config防止cache相关函数定义为空

@zdtyuiop4444 zdtyuiop4444 force-pushed the master branch 2 times, most recently from 4bf57af to 15ea743 Compare December 2, 2024 10:51
@BernardXiong BernardXiong requested a review from unicornx December 6, 2024 14:27
@unicornx
Copy link
Contributor

unicornx commented Dec 10, 2024

@zdtyuiop4444 请问你在做什么实验时发现了这个问题?难道你有做大小核通过共享内存通讯吗?这个场景是什么?

@unicornx
Copy link
Contributor

unicornx commented Dec 10, 2024

@zdtyuiop4444 请补全 commit 信息。具体要求参考:https://github.com/plctlab/plct-rt-thread/tree/notes/0.notes#%E5%A6%82%E4%BD%95%E6%8F%90%E4%BA%A4-git-commit

而且这个 commit 的标题需要修改(包括整个 PR 的标题),不仅仅是 dcache,还有 icache

CACHE_OP_RANGE(DCACHE_IPA_A0, start, size);
}

inline void inv_icache_range(uintptr_t start, size_t size) {
Copy link
Contributor

Choose a reason for hiding this comment

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

请把这个函数放到 icache 的相关函数中去。

* Change Logs:
* Date Author Notes
* 2024/11/27 zdtyuiop4444 Add Icache operation
* 2024/11/26 zdtyuiop4444 The first version
Copy link
Contributor

Choose a reason for hiding this comment

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

只要保留一条 change log 就好

Copy link
Contributor

@unicornx unicornx left a comment

Choose a reason for hiding this comment

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

请见上面的 comments

@unicornx
Copy link
Contributor

@zdtyuiop4444 请问你在做什么实验时发现了这个问题?难道你有做大小核通过共享内存通讯吗?这个场景是什么?

目前 riscv 这边我希望所有的 pr 之前能够通过 issue 充分讨论一下修改的需求,为此我新建了一个 issue:

请麻烦您去这个 issue 上再把遇到的问题和需求再描述一下。

特别的我有个问题,按照你在 pr 上的说法,

可以选择关闭D-Cache或在读前写后调用flush_dcache_range函数进行同步

那么目前这么改是采用了 flush 的策略?我想确认一下这里对内核哪里的 RT_HW_CACHE_FLUSH 操作有影响了?

以上问题请在 #9769 中描述一下,谢谢。

@zdtyuiop4444
Copy link
Contributor Author

@zdtyuiop4444 请问你在做什么实验时发现了这个问题?难道你有做大小核通过共享内存通讯吗?这个场景是什么?

我最近正在做一个这样的应用:小核采集传感器数据发给大核,大核通过网络传输到云端。
这样小核读到的数据要想办法传到大核,直接通过mailbox传输的速度达不到要求,所以就想着使用共享内存的方式传输数据。
在共享内存传数据的时候发现有缓存一致性的问题,大核不能及时看到小核对内存的修改,反过来也一样。跟milkv的工程师交流之后发现是小核这边DCache写回问题。

@zdtyuiop4444 请补全 commit 信息。具体要求参考:https://github.com/plctlab/plct-rt-thread/tree/notes/0.notes#%E5%A6%82%E4%BD%95%E6%8F%90%E4%BA%A4-git-commit

而且这个 commit 的标题需要修改(包括整个 PR 的标题),不仅仅是 dcache,还有 icache

之前没有看到这个,我研究一下改掉。

@zdtyuiop4444 请问你在做什么实验时发现了这个问题?难道你有做大小核通过共享内存通讯吗?这个场景是什么?

目前 riscv 这边我希望所有的 pr 之前能够通过 issue 充分讨论一下修改的需求,为此我新建了一个 issue:

请麻烦您去这个 issue 上再把遇到的问题和需求再描述一下。

特别的我有个问题,按照你在 pr 上的说法,

可以选择关闭D-Cache或在读前写后调用flush_dcache_range函数进行同步

那么目前这么改是采用了 flush 的策略?我想确认一下这里对内核哪里的 RT_HW_CACHE_FLUSH 操作有影响了?

以上问题请在 #9769 中描述一下,谢谢。

首先确实是采用了flush的策略,然后第二个问题有点没看懂,RT_HW_CACHE_FLUSH似乎是一个宏定义,我看在libcpu下也有c906相关的cache定义,但这些代码应该只在RT SMART才会被编译,所以在小核上应该不会有影响。

@unicornx
Copy link
Contributor

首先确实是采用了flush的策略,然后第二个问题有点没看懂,RT_HW_CACHE_FLUSH似乎是一个宏定义,我看在libcpu下也有c906相关的cache定义,但这些代码应该只在RT SMART才会被编译,所以在小核上应该不会有影响。

我原来是想问增加你的改动后,内核中哪里会调用到你增加的 cache update 操作,不过看了你在 issue 中的描述,我理解你增加这些函数后,应该会是在你的测试程序中调用 flush 的操作,只不过这部分代码目前并不在 RTT 中,对吧?

@unicornx
Copy link
Contributor

备注一下,我发现对 ICACHE/DCACHE 的操作会涉及 c906 的专有寄存器 mhcr,这导致工具链必须使用 玄铁的

@zdtyuiop4444
Copy link
Contributor Author

首先确实是采用了flush的策略,然后第二个问题有点没看懂,RT_HW_CACHE_FLUSH似乎是一个宏定义,我看在libcpu下也有c906相关的cache定义,但这些代码应该只在RT SMART才会被编译,所以在小核上应该不会有影响。

我原来是想问增加你的改动后,内核中哪里会调用到你增加的 cache update 操作,不过看了你在 issue 中的描述,我理解你增加这些函数后,应该会是在你的测试程序中调用 flush 的操作,只不过这部分代码目前并不在 RTT 中,对吧?

是的,测试代码要配合mailbox用,那个修改我还没整理好

@unicornx
Copy link
Contributor

This PR fixed #9769

By default, the small core enables D-Cache without ensuring cache
coherence. Therefore, when using shared memory, inconsistencies can
occur in the data read by the small core and the big core.

Solution: Migrate cache-related functions from the official
duo-buildroot-sdk library to implement cache-related operations in
rthw.h. This allows you to either disable D-Cache or call the
flush_dcache_range function before reading and after writing for
synchronization.

It is recommended to use the flush_dcache_range function, as disabling
D-Cache can have a significant performance impact.

Signed-off-by: zdtyuiop4444 <[email protected]>
@unicornx
Copy link
Contributor

unicornx commented Jan 9, 2025

@zdtyuiop4444 你为什么在 review approve 后还 push 代码,如果这样是需要重新 review 的。

我不确定你又改了什么,所以我重新发起了 review。

@zdtyuiop4444
Copy link
Contributor Author

@zdtyuiop4444 你为什么在 review approve 后还 push 代码,如果这样是需要重新 review 的。

我不确定你又改了什么,所以我重新发起了 review。

非常抱歉,我看这个pullrequest一直没有合并,以为原因是因为我这边有一个check没有通过,所以更改了一个空行来重新触发check,排查check不通过的原因,没有意识到不实际修改任何内容也需要重新approve。
代码只在bsp/cvitek/c906_little/board/cache.h的第39行后面多加了一个空行,用来重新触发check。

@unicornx
Copy link
Contributor

unicornx commented Jan 9, 2025

@zdtyuiop4444 你为什么在 review approve 后还 push 代码,如果这样是需要重新 review 的。
我不确定你又改了什么,所以我重新发起了 review。

非常抱歉,我看这个pullrequest一直没有合并,以为原因是因为我这边有一个check没有通过,所以更改了一个空行来重新触发check,排查check不通过的原因,没有意识到不实际修改任何内容也需要重新approve。 代码只在bsp/cvitek/c906_little/board/cache.h的第39行后面多加了一个空行,用来重新触发check。

我看 CI 上的 check 现实都是 pass 的。没有合并应该只是 maintainer 没有 merge 而已。你可以采用各种方式催促(PR 上 comment 也是其中之一)。但不要修改代码,否则 review 就失去意义了。

@zdtyuiop4444
Copy link
Contributor Author

@zdtyuiop4444 你为什么在 review approve 后还 push 代码,如果这样是需要重新 review 的。
我不确定你又改了什么,所以我重新发起了 review。

非常抱歉,我看这个pullrequest一直没有合并,以为原因是因为我这边有一个check没有通过,所以更改了一个空行来重新触发check,排查check不通过的原因,没有意识到不实际修改任何内容也需要重新approve。 代码只在bsp/cvitek/c906_little/board/cache.h的第39行后面多加了一个空行,用来重新触发check。

我看 CI 上的 check 现实都是 pass 的。没有合并应该只是 maintainer 没有 merge 而已。你可以采用各种方式催促(PR 上 comment 也是其中之一)。但不要修改代码,否则 review 就失去意义了。

感谢,以后有疑问一定先在PR里面讨论好

@Rbb666 Rbb666 merged commit 6cbb2c3 into RT-Thread:master Jan 16, 2025
46 checks passed
@unicornx
Copy link
Contributor

unicornx commented Jan 16, 2025

@zdtyuiop4444 这个 PR 合入后,编译小核会有不少 warnings:

See #9920

你还有空 fix 一下?只fix cache 相关的吧。还有些其他的 warning ,目前由另外一个 pr #9865 在 fixing。

@zdtyuiop4444
Copy link
Contributor Author

@zdtyuiop4444 这个 PR 合入后,编译小核会有不少 warnings:

See #9920

你还有空 fix 一下?只fix cache 相关的吧。还有些其他的 warning ,目前由另外一个 pr #9865 在 fixing。

好的,我提一个新pr修一下

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BSP: Cvitek BSP related with cvitek BSP

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants