Re: [PATCH v3] riscv: patch: Fixup lockdep warning in stop_machine

[Changbin Du]

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?)

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?
> 
> Thanks,
> Conor.



-- 
Cheers,
Changbin Du