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

[Changbin Du]

On Thu, Feb 02, 2023 at 08:01:44AM +0000, Conor Dooley wrote:
> On Fri, Feb 03, 2023 at 07:00:43AM +0800, Changbin Du wrote:
> 
> btw, something is wrong with your mail client or host machine.
> Everything that you are sending is timestamped in the future,
> as it is currently 15:57 on the 2nd in UTC+8.
>
hmm, my machine goes 12 hours ahead. Thanks for your remainding.

> > 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@xxxxxxxxxxxx/
> 
> Oh cool, thanks for that.
> Why not resend that approach, with your suggested fixup for
> ftrace_init_nop() then?
> It looks more complex, but is less worrisome & has an R-b from Steven
> already.
> 
Personally I don't like the complex change, because the clients of text patching
api are very limited. So I think it's not hard to take care of it. (The arm code
also doesn't have the assertion)

So I would leave the decision making to maintainers :) Anyway I will send V4 as
a candidate.

> Thanks,
> Conor.
> 



-- 
Cheers,
Changbin Du