Message-Id: <1674109388-6663-1-git-send-email-byungchul.park@lge.com>
Date:   Thu, 19 Jan 2023 15:23:08 +0900
From:   Byungchul Park <byungchul.park@....com>
To:     boqun.feng@...il.com
Cc:     linux-kernel@...r.kernel.org, torvalds@...ux-foundation.org,
        damien.lemoal@...nsource.wdc.com, linux-ide@...r.kernel.org,
        adilger.kernel@...ger.ca, linux-ext4@...r.kernel.org,
        mingo@...hat.com, peterz@...radead.org, will@...nel.org,
        tglx@...utronix.de, rostedt@...dmis.org, joel@...lfernandes.org,
        sashal@...nel.org, daniel.vetter@...ll.ch, duyuyang@...il.com,
        johannes.berg@...el.com, tj@...nel.org, tytso@....edu,
        willy@...radead.org, david@...morbit.com, amir73il@...il.com,
        gregkh@...uxfoundation.org, kernel-team@....com,
        linux-mm@...ck.org, akpm@...ux-foundation.org, mhocko@...nel.org,
        minchan@...nel.org, hannes@...xchg.org, vdavydov.dev@...il.com,
        sj@...nel.org, jglisse@...hat.com, dennis@...nel.org, cl@...ux.com,
        penberg@...nel.org, rientjes@...gle.com, vbabka@...e.cz,
        ngupta@...are.org, linux-block@...r.kernel.org,
        paolo.valente@...aro.org, josef@...icpanda.com,
        linux-fsdevel@...r.kernel.org, viro@...iv.linux.org.uk,
        jack@...e.cz, jlayton@...nel.org, dan.j.williams@...el.com,
        hch@...radead.org, djwong@...nel.org,
        dri-devel@...ts.freedesktop.org, rodrigosiqueiramelo@...il.com,
        melissa.srw@...il.com, hamohammed.sa@...il.com,
        42.hyeyoo@...il.com, chris.p.wilson@...el.com,
        gwan-gyeong.mun@...el.com, max.byungchul.park@...il.com,
        longman@...hat.com
Subject: Re: [PATCH RFC v7 00/23] DEPT(Dependency Tracker)

Boqun wrote:
> On Mon, Jan 16, 2023 at 10:00:52AM -0800, Linus Torvalds wrote:
> > [ Back from travel, so trying to make sense of this series..  ]
> > 
> > On Sun, Jan 8, 2023 at 7:33 PM Byungchul Park <byungchul.park@....com> wrote:
> > >
> > > I've been developing a tool for detecting deadlock possibilities by
> > > tracking wait/event rather than lock(?) acquisition order to try to
> > > cover all synchonization machanisms. It's done on v6.2-rc2.
> > 
> > Ugh. I hate how this adds random patterns like
> > 
> >         if (timeout == MAX_SCHEDULE_TIMEOUT)
> >                 sdt_might_sleep_strong(NULL);
> >         else
> >                 sdt_might_sleep_strong_timeout(NULL);
> >    ...
> >         sdt_might_sleep_finish();
> > 
> > to various places, it seems so very odd and unmaintainable.
> > 
> > I also recall this giving a fair amount of false positives, are they all fixed?
> > 
> 
> From the following part in the cover letter, I guess the answer is no?

I fixed what we found anyway.

> 	...
> 	6. Multiple reports are allowed.
> 	7. Deduplication control on multiple reports.
> 	8. Withstand false positives thanks to 6.
> 	...
> 
> seems to me that the logic is since DEPT allows multiple reports so that
> false positives are fitlerable by users?

At lease, it's needed until DEPT is considered stable because stronger
detection inevitably has more chance of false alarms unless we do manual
fix on each, which is the same as Lockdep.

> > Anyway, I'd really like the lockdep people to comment and be involved.
> 
> I never get Cced, so I'm unware of this for a long time...

Sorry I missed it. I will cc you from now on.

> A few comments after a quick look:
> 
> *	Looks like the DEPT dependency graph doesn't handle the
> 	fair/unfair readers as lockdep current does. Which bring the
> 	next question.

No. DEPT works better for unfair read. It works based on wait/event. So
read_lock() is considered a potential wait waiting on write_unlock()
while write_lock() is considered a potential wait waiting on either
write_unlock() or read_unlock(). DEPT is working perfect for it.

For fair read (maybe you meant queued read lock), I think the case
should be handled in the same way as normal lock. I might get it wrong.
Please let me know if I miss something.

> *	Can DEPT pass all the selftests of lockdep in
> 	lib/locking-selftests.c?
> 
> *	Instead of introducing a brand new detector/dependency tracker,
> 	could we first improve the lockdep's dependency tracker? I think

At the beginning of this work, of course I was thinking to improve
Lockdep but I decided to implement a new tool because:

	1. What we need to check for deadlock detection is no longer
	   lock dependency but more fundamental dependency by wait/event.
	   A better design would have a separate dependency engine for
	   that, not within Lockdep. Remind lock/unlock are also
	   wait/event after all.

	2. I was thinking to revert the revert of cross-release. But it
	   will start to report false alarms as Lockdep was at the
	   beginning, and require us to keep fixing things until being
	   able to see what we are interested in, maybe for ever. How
	   can we control that situation? I wouldn't use this extention.

	3. Okay. Let's think about modifying the current Lockdep to make
	   it work similar to DEPT. It'd require us to pay more effort
	   than developing a new simple tool from the scratch with the
	   basic requirement.

	4. Big change at once right away? No way. The new tool need to
	   be matured and there are ones who want to make use of DEPT at
	   the same time. The best approach would be I think to go along
	   together for a while.

Please don't look at each detail but the big picture, the architecture.
Plus, please consider I introduce a tool only focucing on fundamental
dependency itself that Lockdep can make use of. I wish great developers
like you would join improve the common engine togather.

> 	Byungchul also agrees that DEPT and lockdep should share the
> 	same dependency tracker and the benefit of improving the

I agree that both should share a single tracker.

> 	existing one is that we can always use the self test to catch
> 	any regression. Thoughts?

I imagine the follownig look for the final form:

     Lock correctness checker(LOCKDEP)
     +-----------------------------------------+
     | Lock usage correctness check            |
     |                                         |
     |                                         |
     |       (Request dependency check)        |
     |                           T             |
     +---------------------------|-------------+
                                 |
     Dependency tracker(DEPT)    V
     +-----------------------------------------+
     | Dependency check                        |
     | (by tracking wait and event context)    |
     +-----------------------------------------+

> Actually the above sugguest is just to revert revert cross-release
> without exposing any annotation, which I think is more practical to
> review and test.

Reverting the revert of cross-release is not bad. But I'd suggest a
nicer design for the reasons I explained above.

	Byungchul

> I'd sugguest we 1) first improve the lockdep dependency tracker with
> wait/event in mind and then 2) introduce wait related annotation so that
> users can use, and then 3) look for practical ways to resolve false
> positives/multi reports with the help of users, if all goes well,
> 4) make it all operation annotated.
> 
> Thoughts?
> 
> Regards,
> Boqun

Powered by blists - more mailing lists