On Sat, Mar 05, 2022 at 11:15:38PM +0900, Byungchul Park wrote:
> On Fri, Mar 04, 2022 at 10:26:23PM -0500, Theodore Ts'o wrote:
> > On Fri, Mar 04, 2022 at 09:42:37AM +0900, Byungchul Park wrote:
> > >
> > > All contexts waiting for any of the events in the circular dependency
> > > chain will be definitely stuck if there is a circular dependency as I
> > > explained. So we need another wakeup source to break the circle. In
> > > ext4 code, you might have the wakeup source for breaking the circle.
> > >
> > > What I agreed with is:
> > >
> > > The case that 1) the circular dependency is unevitable 2) there are
> > > another wakeup source for breadking the circle and 3) the duration
> > > in sleep is short enough, should be acceptable.
> > >
> > > Sounds good?
> >
> > These dependencies are part of every single ext4 metadata update,
> > and if there were any unnecessary sleeps, this would be a major
> > performance gap, and this is a very well studied part of ext4.
> >
> > There are some places where we sleep, sure. In some case
> > start_this_handle() needs to wait for a commit to complete, and the
> > commit thread might need to sleep for I/O to complete. But the moment
> > the thing that we're waiting for is complete, we wake up all of the
> > processes on the wait queue. But in the case where we wait for I/O
> > complete, that wakeupis coming from the device driver, when it
> > receives the the I/O completion interrupt from the hard drive. Is
> > that considered an "external source"? Maybe DEPT doesn't recognize
> > that this is certain to happen just as day follows the night? (Well,
> > maybe the I/O completion interrupt might not happen if the disk drive
> > bursts into flames --- but then, you've got bigger problems. :-)
>
> Almost all you've been blaming at Dept are totally non-sense. Based on
> what you're saying, I'm conviced that you don't understand how Dept
> works even 1%. You don't even try to understand it before blame.
>
> You don't have to understand and support it. But I can't response to you
> if you keep saying silly things that way.
Byungchul, other than ext4 have there been any DEPT reports that other
subsystem maintainers' agree were valid usecases?
Regarding false-positives, just to note lockdep is not without its share of
false-positives. Just that (as you know), the signal-to-noise ratio should be
high for it to be useful. I've put up with lockdep's false positives just
because it occasionally saves me from catastrophe.
> > In any case, if DEPT is going to report these "circular dependencies
> > as bugs that MUST be fixed", it's going to be pure noise and I will
> > ignore all DEPT reports, and will push back on having Lockdep replaced
>
> Dept is going to be improved so that what you are concerning about won't
> be reported.
Yeah I am looking forward to learning more about it however I was wondering
about the following: lockdep can already be used for modeling "resource
acquire/release" and "resource wait" semantics that are unrelated to locks,
like we do in mm reclaim. I am wondering why we cannot just use those existing
lockdep mechanisms for the wait/wake usecases (Assuming that we can agree
that circular dependencies on related to wait/wake is a bad thing. Or perhaps
there's a reason why Peter Zijlstra did not use lockdep for wait/wake
dependencies (such as multiple wake sources) considering he wrote a lot of
that code.
Keep kicking ass brother, you're doing great.
Thanks,
Joel
On Sat, Mar 05, 2022 at 03:05:23PM +0000, Joel Fernandes wrote:
> On Sat, Mar 05, 2022 at 11:15:38PM +0900, Byungchul Park wrote:
> > Almost all you've been blaming at Dept are totally non-sense. Based on
> > what you're saying, I'm conviced that you don't understand how Dept
> > works even 1%. You don't even try to understand it before blame.
> >
> > You don't have to understand and support it. But I can't response to you
> > if you keep saying silly things that way.
>
> Byungchul, other than ext4 have there been any DEPT reports that other
> subsystem maintainers' agree were valid usecases?
Not yet.
> Regarding false-positives, just to note lockdep is not without its share of
> false-positives. Just that (as you know), the signal-to-noise ratio should be
> high for it to be useful. I've put up with lockdep's false positives just
> because it occasionally saves me from catastrophe.
I love your insight. Agree. A tool would be useful only when it's
*actually* helpful. I hope Dept would be so.
> > > In any case, if DEPT is going to report these "circular dependencies
> > > as bugs that MUST be fixed", it's going to be pure noise and I will
> > > ignore all DEPT reports, and will push back on having Lockdep replaced
> >
> > Dept is going to be improved so that what you are concerning about won't
> > be reported.
>
> Yeah I am looking forward to learning more about it however I was wondering
> about the following: lockdep can already be used for modeling "resource
> acquire/release" and "resource wait" semantics that are unrelated to locks,
> like we do in mm reclaim. I am wondering why we cannot just use those existing
> lockdep mechanisms for the wait/wake usecases (Assuming that we can agree
1. Lockdep can't work with general waits/events happening across
contexts basically. To get over this, manual tagging of
acquire/release can be used at each section that we suspect. But
unfortunately, we cannot use the method if we cannot simply identify
the sections. Furthermore, it's inevitable to miss sections that
shouldn't get missed.
2. Some cases should be correctly tracked via wait/event model, not
acquisition order model. For example, read-lock in rwlock should be
defined as a waiter waiting for write-unlock, write-lock in rwlock
as a waiter waiting for either read-unlock or write-unlock.
Otherwise, if we try to track those cases using acquisition order,
it cannot completely work. Don't you think it looks werid?
3. Tracking what we didn't track before means both stronger detection
and new emergence of false positives, exactly same as Lockdep at its
beginning when it started to track what we hadn't tracked before.
Even though the emergence was allowed at that time, now that Locdkep
got stable enough, folks would be more strict on new emergences. It's
gonna get even worse if valid reports are getting prevented by false
positives.
For that reason, multi reporting functionality is essential. I was
thinking to improve Lockdep to allow multi reporting. But it might be
needed to change more than developing a new tool from scratch. Plus
it might be even more difficult cuz Lockdep already works not badly.
So even for Lockdep, I thought the new thing should be developed
independently leaving Lockdep as it is.
4. (minor reason) The concept and name of acquisition and release is not
for general wait/event. The design and implementation are not,
either. I wanted to address the issue as soon as possible before we
squeeze out Lockdep to use for general wait/event more and the kernel
code gets weird. Of course, it doesn't mean Dept is more stable than
Lockdep. However, I can tell Dept works what a dependency tool should
do and we need to make the code go right.
> that circular dependencies on related to wait/wake is a bad thing. Or perhaps
> there's a reason why Peter Zijlstra did not use lockdep for wait/wake
> dependencies (such as multiple wake sources) considering he wrote a lot of
> that code.
>
> Keep kicking ass brother, you're doing great.
Thank you! I'll go through this in a right way so as not to disappoint
you!
Thanks,
Byungchul