Messages in this thread | | | Date | Fri, 3 Feb 2023 07:00:43 +0800 | From | Changbin Du <> | Subject | Re: [PATCH v3] riscv: patch: Fixup lockdep warning in stop_machine |
| |
On Wed, Feb 01, 2023 at 02:01:07PM +0000, Conor Dooley wrote: > On Thu, Feb 02, 2023 at 05:00:31AM +0800, Changbin Du wrote: > > On Tue, Jan 31, 2023 at 07:50:20AM +0000, Conor Dooley wrote: > > > On Tue, Jan 31, 2023 at 03:26:33PM +0800, Guo Ren wrote: > > [snip] > > > > > > > > > > > > - /* > > > > > > - * Before reaching here, it was expected to lock the text_mutex > > > > > > - * already, so we don't need to give another lock here and could > > > > > > - * ensure that it was safe between each cores. > > > > > > - */ > > > > > > - lockdep_assert_held(&text_mutex); > > > > > > > > > > I must admit, patches like this do concern me a little, as a someone > > > > > unfamiliar with the world of probing and tracing. > > > > > Seeing an explicit check that the lock was held, leads me to believe > > > > > that the original author (Zong Li I think) thought that the text_mutex > > > > > lock was insufficient. > > > > > Do you think that their fear is unfounded? Explaining why it is safe to > > > > > remove this assertion in the commit message would go a long way towards > > > > > easing my anxiety! > > > > > > > > > > Also, why delete the comment altogether? The comment provides some > > > > > information that doesn't appear to become invalid, even with the > > > > > assertion removed? > > > > Stop_machine separated the mutex context and made a lockdep warning. > > > > So text_mutex can't be used here. We need to find another check > > > > solution. I agree with the patch. > > > > > > Whether or not you agree with the change is not the point (with your SoB > > > I'd hope you agree with it). > > > I understand that you two are trying to fix a false positive lockdep > > > warning, but what I am asking for an explanation as to why the original > > > author's fear is unfounded. > > > Surely, having added the assertion, they were not thinking of the same > > > code path that you guys are hitting the false positive on? > > > > > The assertion is reasonable since the fixmap entry is shared. The text_mutex > > does should be held before entering that function. But the false positive cases > > make some functions (ftrace for example) difficult to use due to warning log > > storm. > > > > Either the lockdep should be fixed for stop_machine, or remove the assertion > > simply now (we can keep the comments). (or do the assertion conditionly?) > > How would you suggest checking it conditionally? > Please refer to a early patch from Palmer Dabbelt. https://lore.kernel.org/all/20220322022331.32136-1-palmer@rivosinc.com/
> Also, if you persist in removing the assertion, there is a comment in > arch/riscv/kernel/ftrace.c that would need to be updated. (L129-ish) > No problem.
> The comment you removed in this patch seems valid both before and after > though, so I don't see a compelling reason for its removal. We all agreed. The key is to get rid of false positive case.
> > > And this is not a riscv only problem but common for architectures which use > > stop_machine to patch text. (arm for example) > > > > > Perhaps Zong themselves can tell us what the original fear was? >
-- Cheers, Changbin Du
| |