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

[kernel]将ipc中的上下文检查级别提高 #5576

Closed
wants to merge 0 commits into from

Conversation

jiladahe1997
Copy link
Contributor

@jiladahe1997 jiladahe1997 commented Jan 28, 2022

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

[
使用信号量、互斥锁等ipc通信机制时,要求上下文“必须”是线程上下文,否则会导致
严重的异常(死锁、hardfault等)。

例如,线程A成功获取了互斥锁M,然后进入了中断,在中断中又尝试获取互斥锁M,此时会变成死锁。

上下文检查以宏定义的方式声明: RT_DEBUG_IN_THREAD_CONTEXT,此前一直位于 rtdebug.h
文件中,并根据 RT_DEBUG 宏定义来控制开关。

作为比较关键的检查,应该提高其重要级别,以防止有人关闭 RT_DEBUG 时,也自动关闭了
上下文检查。

参考:
FreeRTOS/queue.c :1542行
https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/dca4f80a6be40f77848749ea18efd92b9d8ef66a/queue.c
Linux Kernel /mutex.c: 280行
https://github.com/torvalds/linux/blob/master/kernel/locking/mutex.c

Signed-off-by: Mingrui Ren [email protected]




顺便一提,我遇到的问题是,在 rtthread_startup() 函数中(线程调度初始化完成之前),调用 rt_mutex_take()rt_mutex_release() 函数后,struct rt_mutex.value 的值会意外的变成2,导致后续可以有多个线程同时持有互斥锁。

总而言之,调用IPC通信机制的时候,上下文不能是: 中断 和 系统初始化 阶段。

]

以下的内容不应该在提交PR时的message修改,修改下述message,PR会被直接关闭。请在提交PR后,浏览器查看PR并对以下检查项逐项check,没问题后逐条在页面上打钩。
The following content must not be changed in the submitted PR message. Otherwise, the PR will be closed immediately. After submitted PR, please use a web browser to visit PR, and check items one by one, and ticked them if no problem.

当前拉取/合并请求的状态 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

@mysterywolf
Copy link
Member

这个放在下个版本,对api所处环境的检查确实很重要

@mysterywolf mysterywolf added proposal proposal for future version v4.1.0 labels Jan 28, 2022
@jiladahe1997
Copy link
Contributor Author

这个放在下个版本,对api所处环境的检查确实很重要

@mysterywolf 好的,感谢回复。 到时候如需修改PR欢迎留言。

@mysterywolf mysterywolf removed the proposal proposal for future version label Jan 30, 2022
@thewon86
Copy link
Contributor

thewon86 commented Feb 9, 2022

https://club.rt-thread.org/ask/article/3204.html

各类函数的用法,重点在于告诉用户怎么用,而不是让使用者毫无头绪的去乱试,每次遇到 assert 才恍然大悟,哦,这个函数不能这么用。这种想法是错的。

assert 是在开发阶段,或者说,是对入门者的一种快速暴力提示;对于一个熟练使用者来讲还依靠 assert 来定位用法错误,会对产品引入隐患的。
一个产品,绝对不能带着 assert 运行。虽然 ipc 中添加的这些 assert 经过实验阶段验证没有问题了,也不能让他们留在程序里。

@mysterywolf 就像前不久在微信群里提到的那样,检测宏最好放到函数开头,醒目的地方,做“警示”“提示”作用。让使用者方便,快速,第一件需要了解到的事儿就是知道这个函数的使用环境。

这个 pr ,除了增加了代码大小,并不能引导使用者使用上的更多注意。

@thewon86
Copy link
Contributor

thewon86 commented Feb 9, 2022

顺便一提,我遇到的问题是,在 rtthread_startup() 函数中(线程调度初始化完成之前),调用 rt_mutex_take() 和 rt_mutex_release() 函数后,struct rt_mutex.value 的值会意外的变成2,导致后续可以有多个线程同时持有互斥锁。

rt_thread_startup 函数? 它里面需要用到锁了?

@jiladahe1997
Copy link
Contributor Author

jiladahe1997 commented Feb 9, 2022

assert 是在开发阶段,或者说,是对入门者的一种快速暴力提示;对于一个熟练使用者来讲还依靠 assert 来定位用法错误,会对产品引入隐患的。 一个产品,绝对不能带着 assert 运行。虽然 ipc 中添加的这些 assert 经过实验阶段验证没有问题了,也不能让他们留在程序里。

感谢回复。 我赞同您的这个观点,assert 过于暴力,不应该在这里使用。或许可以参考 linux kernel,强制打印一段提示:
Linux Kernel /mutex.c: 280行
https://github.com/torvalds/linux/blob/master/kernel/locking/mutex.c

各类函数的用法,重点在于告诉用户怎么用,而不是让使用者毫无头绪的去乱试,每次遇到 assert 才恍然大悟,哦,这个函数不能这么用。这种想法是错的。

而这个观点我认为有待商榷。 不应该说 README 里面写了,就把责任推给使用者。 受欢迎的产品常常是为用户着想,方便用户使用; 用户用着越方便,产品也就越受欢迎。

@jiladahe1997
Copy link
Contributor Author

顺便一提,我遇到的问题是,在 rtthread_startup() 函数中(线程调度初始化完成之前),调用 rt_mutex_take() 和 rt_mutex_release() 函数后,struct rt_mutex.value 的值会意外的变成2,导致后续可以有多个线程同时持有互斥锁。

rt_thread_startup 函数? 它里面需要用到锁了?

没有。 是我自己在 rt_thread_startup 增加了初始化步骤,在初始化步骤中误调用了锁。 但是由于没有任何错误相关提示,导致排查花了比较久的时间。

@mysterywolf
Copy link
Member

mysterywolf commented Feb 9, 2022

assert 是在开发阶段,或者说,是对入门者的一种快速暴力提示;对于一个熟练使用者来讲还依靠 assert 来定位用法错误,会对产品引入隐患的。

@thewon86 我同意,我之前也在想直接assert干掉是不是太危险了,这个assert在投入产品之后,即便出问题也不能直接卡死,否则,真正的bug可能没引发什么问题,反倒是assert直接把产品给卡死了。

@jg1uaa 先不用着急改这个pr,现存的一些安全类型的宏函数也有一些问题,等把现存的优化好,成熟的经验就可以放在这个pr上了。

src/ipc.c Outdated
@@ -515,7 +515,7 @@ rt_err_t rt_sem_take(rt_sem_t sem, rt_int32_t time)
else
{
/* current context checking */
RT_DEBUG_IN_THREAD_CONTEXT;
RT_CHECK_IN_THREAD_CONTEXT;
Copy link
Member

@Guozhanxin Guozhanxin Feb 9, 2022

Choose a reason for hiding this comment

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

之前这里用的RT_DEBUG是可以被关闭了,这样产品稳定之后就可以关闭这里的检查了。感觉后面的方向可以是让RT_DEBUG更细化,这里有单独的开关,最后关闭就好了。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

之前这里用的RT_DEBUG是可以被关闭了,这样产品稳定之后就可以关闭这里的检查了。感觉后面的方向可以是让RT_DEBUG更细化,这里有单独的开关,最后关闭就好了。

感谢回复。 我这个PR的主要想法是: 这里的上下文检查应该强制开启,不能关闭。 因为IPC通信的上下文“必须”是线程上下文,否则有可能会导致严重的后果,比如系统崩溃、死锁等,一定是错误的用法。

Copy link
Member

Choose a reason for hiding this comment

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

是的,在产品开发阶段这个是要开着的,但是如果产品已经开发完毕,稳定之后,这里应该是可以关闭的。这样运行起来更快,产品固件的大小也更小。

@mysterywolf mysterywolf added discussion This PR/issue needs to be discussed later and removed v4.1.0 labels Mar 28, 2022
@mysterywolf mysterywolf marked this pull request as draft September 29, 2022 05:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion This PR/issue needs to be discussed later
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants