About the try to remove cross-release feature entirely by Ingo

About the try to remove cross-release feature entirely by Ingo

[Byungchul Park]

Lockdep works, based on the following:

   (1) Classifying locks properly
   (2) Checking relationship between the classes

If (1) is not good or (2) is not good, then we
might get false positives.

For (1), we don't have to classify locks 100%
properly but need as enough as lockdep works.

For (2), we should have a mechanism w/o
logical defects.

Cross-release added an additional capacity to
(2) and requires (1) to get more precisely classified.

Since the current classification level is too low for
cross-release to work, false positives are being
reported frequently with enabling cross-release.
Yes. It's a obvious problem. It needs to be off by
default until the classification is done by the level
that cross-release requires.

But, the logic (2) is valid and logically true. Please
keep the code, mechanism, and logic.

-- 
Thanks,
Byungchul

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Byungchul Park]

On Wed, Dec 13, 2017 at 3:24 PM, Byungchul Park
<max.byungchul.park@gmail.com> wrote:
> Lockdep works, based on the following:
>
>    (1) Classifying locks properly
>    (2) Checking relationship between the classes
>
> If (1) is not good or (2) is not good, then we
> might get false positives.
>
> For (1), we don't have to classify locks 100%
> properly but need as enough as lockdep works.
>
> For (2), we should have a mechanism w/o
> logical defects.
>
> Cross-release added an additional capacity to
> (2) and requires (1) to get more precisely classified.
>
> Since the current classification level is too low for
> cross-release to work, false positives are being
> reported frequently with enabling cross-release.
> Yes. It's a obvious problem. It needs to be off by
> default until the classification is done by the level
> that cross-release requires.
>
> But, the logic (2) is valid and logically true. Please
> keep the code, mechanism, and logic.

In addition, I want to say that the current level of
classification is much less than 100% but, since we
have annotated well to suppress wrong reports by
rough classifications, finally it does not come into
view by original lockdep for now.

But since cross-release makes the dependency
graph more fine-grained, it easily comes into view.

Even w/o cross-release, it can happen by adding
additional dependencies connecting two roughly
classified lock classes in the future.

Furthermore, I can see many places in kernel to
consider wait_for_completion() using manual
lock_acquire()/lock_release() and the rate using it
raises.

In other words, even without cross-release, the
more we add manual annotations for
wait_for_completion() the more we definitely
suffer same problems someday, we are facing now
through cross-release.

Therefore, I want to say the fundamental problem
comes from classification, not cross-release
specific. Of course, since cross-release accelerates
the condition, I agree with it to be off for now.

-- 
Thanks,
Byungchul

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Bart Van Assche]

On Wed, 2017-12-13 at 16:13 +0900, Byungchul Park wrote:
> In addition, I want to say that the current level of
> classification is much less than 100% but, since we
> have annotated well to suppress wrong reports by
> rough classifications, finally it does not come into
> view by original lockdep for now.

The Linux kernel is not a vehicle for experiments. The majority of false
positives should have been fixed before the crossrelease patches were sent
to Linus.

Bart.

Re: About the try to remove cross-release feature entirely by Ingo

[Theodore Ts'o]

On Wed, Dec 13, 2017 at 04:13:07PM +0900, Byungchul Park wrote:
> 
> Therefore, I want to say the fundamental problem
> comes from classification, not cross-release
> specific.

You keep saying that it is "just" a matter of classificaion.

However, it is not obvious how to do the classification in a sane
manner.  And this is why I keep pointing out that there is no
documentation on how to do this, and somehow you never respond to this
point....

In the case where you have multiple unrelated subsystems that can be
stacked in different ways, with potentially multiple instances stacked
on top of each other, it is not at all clear to me how this problem
should be solved.

It was said on one of these threads (perhaps by you, perhaps by
someone else), that we can't expect the lockdep maintainers to
understand all of the subsystems in the kernels, and so therefore it
must be up to the subsystem maintainers to classify the locks.  I
interpreted this as the lockdep maintainers saying, "hey, not my
fault, it's the subsystem maintainer's fault for not properly
classifying the locks" --- and thus dumping the responsibility in the
subsystem maintainers' laps.

I don't know if the situation is just that lockdep is insufficiently
documented, and with the proper tutorial, it would be obvious how to
solve the classification problem.

Or, if perhaps, there *is* no way to solve the classification problem,
at least not in a general form.

For example --- suppose we have a network block device on which there
is an btrfs file system, which is then exported via NFS.  Now all of
the TCP locks will be used twice for two different instances, once for
the TCP connection for the network block device, and then for the NFS
export.

How exactly are we supposed to classify the locks to make it all work?

Or the loop device built on top of an ext4 file system which on a
LVM/device mapper device.  And suppose the loop device is then layered
with a dm-error device for regression testing, and with another ext4
file system on top of that?

How exactly are we supposed to classify the locks in that situation?
Where's the documentation and tutorials which explain how to make this
work, if the responsibility is going to be dumped on the subsystem
maintainers' laps?  Or if the lockdep maintainers are expected to fix
and classify all of these locks, are you volunteering to do this?

How hard is it exactly to do all of this classification work, no
matter whose responsibility it will ultimately be?

And if the answer is that it is too hard, then let me gently suggest
to you that perhaps, if this is a case, that maybe this is a
fundamental and fatal flaw with the cross-release and completion
lockdep feature?

Best regards,

						- Ted

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Byungchul Park]

On Thu, Dec 14, 2017 at 12:07 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Wed, Dec 13, 2017 at 04:13:07PM +0900, Byungchul Park wrote:
>>
>> Therefore, I want to say the fundamental problem
>> comes from classification, not cross-release
>> specific.
>
> You keep saying that it is "just" a matter of classificaion.

But, it's a fact.

> However, it is not obvious how to do the classification in a sane
> manner.  And this is why I keep pointing out that there is no
> documentation on how to do this, and somehow you never respond to this
> point....

I can write a document explaining what lock class is but.. I
cannot explain how to assign it perfectly since there's no right
answer. It's something we need to improve more and more.

> In the case where you have multiple unrelated subsystems that can be
> stacked in different ways, with potentially multiple instances stacked
> on top of each other, it is not at all clear to me how this problem
> should be solved.

I cannot give you a perfect solution immediately. I know, and
as you know, it's a very difficult problem to solve.

> It was said on one of these threads (perhaps by you, perhaps by
> someone else), that we can't expect the lockdep maintainers to
> understand all of the subsystems in the kernels, and so therefore it
> must be up to the subsystem maintainers to classify the locks.  I
> interpreted this as the lockdep maintainers saying, "hey, not my
> fault, it's the subsystem maintainer's fault for not properly
> classifying the locks" --- and thus dumping the responsibility in the
> subsystem maintainers' laps.

Sorry to say, making you feel like that.

Precisely speaking, the responsibility for something caused by
cross-release is on me, and the responsibility for something caused
by lockdep itselt is on lockdep.

I meant, in the current way to assign lock class automatically, it's
inevitable for someone to annotate places manually, and it can be
done best by each expert. But, anyway fundamentally I think the
responsibility is on lockdep.

> I don't know if the situation is just that lockdep is insufficiently
> documented, and with the proper tutorial, it would be obvious how to
> solve the classification problem.
>
> Or, if perhaps, there *is* no way to solve the classification problem,
> at least not in a general form.

Agree. It's a very difficult one to solve.

> For example --- suppose we have a network block device on which there
> is an btrfs file system, which is then exported via NFS.  Now all of
> the TCP locks will be used twice for two different instances, once for
> the TCP connection for the network block device, and then for the NFS
> export.
>
> How exactly are we supposed to classify the locks to make it all work?
>
> Or the loop device built on top of an ext4 file system which on a
> LVM/device mapper device.  And suppose the loop device is then layered
> with a dm-error device for regression testing, and with another ext4
> file system on top of that?

Ditto.

> How exactly are we supposed to classify the locks in that situation?
> Where's the documentation and tutorials which explain how to make this
> work, if the responsibility is going to be dumped on the subsystem
> maintainers' laps?  Or if the lockdep maintainers are expected to fix
> and classify all of these locks, are you volunteering to do this?

I have the will. I will.

> How hard is it exactly to do all of this classification work, no
> matter whose responsibility it will ultimately be?
>
> And if the answer is that it is too hard, then let me gently suggest
> to you that perhaps, if this is a case, that maybe this is a
> fundamental and fatal flaw with the cross-release and completion
> lockdep feature?

I don't understand this.

> Best regards,
>
>                                                 - Ted



-- 
Thanks,
Byungchul

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Peter Zijlstra]

On Wed, Dec 13, 2017 at 10:07:11PM -0500, Theodore Ts'o wrote:
> interpreted this as the lockdep maintainers saying, "hey, not my
> fault, it's the subsystem maintainer's fault for not properly
> classifying the locks" --- and thus dumping the responsibility in the
> subsystem maintainers' laps.

Let me clarify that I (as lockdep maintainer) disagree with that
sentiment. I have spend a lot of time over the years staring at random
code trying to fix lockdep splats. Its awesome if corresponding
subsystem maintainers help out and many have, but I very much do not
agree its their problem and their problem alone.

This attitude is one of the biggest issues I have with the crossrelease
stuff and why I don't disagree with Ingo taking it out (for now).

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Byungchul Park]

On Thu, Dec 14, 2017 at 8:18 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Dec 13, 2017 at 10:07:11PM -0500, Theodore Ts'o wrote:
>> interpreted this as the lockdep maintainers saying, "hey, not my
>> fault, it's the subsystem maintainer's fault for not properly
>> classifying the locks" --- and thus dumping the responsibility in the
>> subsystem maintainers' laps.
>
> Let me clarify that I (as lockdep maintainer) disagree with that
> sentiment. I have spend a lot of time over the years staring at random
> code trying to fix lockdep splats. Its awesome if corresponding
> subsystem maintainers help out and many have, but I very much do not
> agree its their problem and their problem alone.

I apologize to all of you. That's really not what I intended to say.

I said that other folks can annotate it for the sub-system better
than lockdep developer, so suggested to invalidate locks making
trouble and wanting to avoid annotating it at the moment, and
validate those back when necessary with additional annotations.

It's my fault. I'm not sure how I should express what I want to say,
but, I didn't intend to charge the responsibility to other folks.

Ideally, I think it's best to solve it with co-work. I should've been
more careful to say that.

Again, I apologize for that, to lockdep and fs maintainers.

Of course, for cross-release, I have the will to annotate it or
find a better way to avoid false positives. And I think I have to.

-- 
Thanks,
Byungchul

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] locking/lockdep: Remove the cross-release locking checks

[Ingo Molnar]

* Byungchul Park <max.byungchul.park@gmail.com> wrote:

> Lockdep works, based on the following:
> 
>    (1) Classifying locks properly
>    (2) Checking relationship between the classes
> 
> If (1) is not good or (2) is not good, then we
> might get false positives.
> 
> For (1), we don't have to classify locks 100%
> properly but need as enough as lockdep works.
> 
> For (2), we should have a mechanism w/o
> logical defects.
> 
> Cross-release added an additional capacity to
> (2) and requires (1) to get more precisely classified.
> 
> Since the current classification level is too low for
> cross-release to work, false positives are being
> reported frequently with enabling cross-release.
> Yes. It's a obvious problem. It needs to be off by
> default until the classification is done by the level
> that cross-release requires.
> 
> But, the logic (2) is valid and logically true. Please
> keep the code, mechanism, and logic.

Just to give a full context to everyone: the patch that removes the cross-release 
locking checks was Cc:-ed to lkml, I've attached the patch below again.

In general, as described in the changelog, the cross-release checks were 
historically just too painful (first they were too slow, and they also had a lot 
of false positives), and today, 4 months after its introduction, the cross-release 
checks *still* produce numerous false positives, especially in the filesystem 
space, but the continuous-integration testing folks were still having trouble with 
kthread locking patterns causing false positives:

  https://bugs.freedesktop.org/show_bug.cgi?id=103950

which were resulting in two bad reactions:

 - turning off lockdep

 - writing patches that uglified unrelated subsystems

So while I appreciate the fixes that resulted from running cross-release, there's 
still numerous false positives, months after its interaction, which is 
unacceptable. For us to have this feature it has to have roughly similar qualities 
as compiler warnings:

 - there's a "zero false positive warnings" policy

 - plus any widespread changes to avoid warnings has to improve the code,
   not make it uglier.

Lockdep itself is a following that policy: the default state is that it produces 
no warnings upstream, and any annotations added to unrelated code documents the 
locking hierarchies.

While technically we could keep the cross-release checking code upstream and turn 
it off by default via the Kconfig switch, I'm not a big believer in such a policy 
for complex debugging code:

 - We already did that for v4.14, two months ago:

     b483cf3bc249: locking/lockdep: Disable cross-release features for now

   ... and re-enabled it for v4.15 - but the false positives are still not fixed.

 - either the cross-release checking code can be fixed and then having it off by
   default is just wrong, because we can apply the fixed code again once it's
   fixed.

 - or it cannot be fixed (or we don't have the manpower/interest to fix it),
   in which case having it off is only delaying the inevitable.

In any case, for v4.15 it's clear that the false positives are too numerous.

Thanks,

	Ingo


=============================>

Re: [PATCH] locking/lockdep: Remove the cross-release locking checks

[Byungchul Park]

On Wed, Dec 13, 2017 at 7:46 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Byungchul Park <max.byungchul.park@gmail.com> wrote:
>
>> Lockdep works, based on the following:
>>
>>    (1) Classifying locks properly
>>    (2) Checking relationship between the classes
>>
>> If (1) is not good or (2) is not good, then we
>> might get false positives.
>>
>> For (1), we don't have to classify locks 100%
>> properly but need as enough as lockdep works.
>>
>> For (2), we should have a mechanism w/o
>> logical defects.
>>
>> Cross-release added an additional capacity to
>> (2) and requires (1) to get more precisely classified.
>>
>> Since the current classification level is too low for
>> cross-release to work, false positives are being
>> reported frequently with enabling cross-release.
>> Yes. It's a obvious problem. It needs to be off by
>> default until the classification is done by the level
>> that cross-release requires.
>>
>> But, the logic (2) is valid and logically true. Please
>> keep the code, mechanism, and logic.
>
> Just to give a full context to everyone: the patch that removes the cross-release
> locking checks was Cc:-ed to lkml, I've attached the patch below again.
>
> In general, as described in the changelog, the cross-release checks were
> historically just too painful (first they were too slow, and they also had a lot
> of false positives), and today, 4 months after its introduction, the cross-release
> checks *still* produce numerous false positives, especially in the filesystem
> space, but the continuous-integration testing folks were still having trouble with
> kthread locking patterns causing false positives:

I admit false positives are the main problem, that should be solved.

I'm going willingly to try my best to solve that. However, as you may
know through introduction of lockdep, it's not something that I can
do easily and shortly on my own. It need take time to annotate
properly to avoid false positives.

>   https://bugs.freedesktop.org/show_bug.cgi?id=103950
>
> which were resulting in two bad reactions:
>
>  - turning off lockdep
>
>  - writing patches that uglified unrelated subsystems

I can't give you a solution at the moment but it's something we
think more so that we classify locks properly and not uglify them.

Even without cross-release, once we start to add lock_acquire() in
submit_bio_wait() in the ugly way to consider wait_for_completion()
someday, we would face this problem again. It's not an easy problem,
however, it's worth trying.

> So while I appreciate the fixes that resulted from running cross-release, there's
> still numerous false positives, months after its interaction, which is
> unacceptable. For us to have this feature it has to have roughly similar qualities
> as compiler warnings:
>
>  - there's a "zero false positive warnings" policy

It's almost impossible... but need time. I wonder if lockdep did at the
beginning. If I can, I want to fix false positive as many as possible by
myself. But, unluckily it does not happen in my machine. I want to get
informed from others, keeping it in mainline tree.

>  - plus any widespread changes to avoid warnings has to improve the code,
>    not make it uglier.

Agree.

> Lockdep itself is a following that policy: the default state is that it produces
> no warnings upstream, and any annotations added to unrelated code documents the
> locking hierarchies.
>
> While technically we could keep the cross-release checking code upstream and turn
> it off by default via the Kconfig switch, I'm not a big believer in such a policy
> for complex debugging code:
>
>  - We already did that for v4.14, two months ago:
>
>      b483cf3bc249: locking/lockdep: Disable cross-release features for now

The main reason disabling it was "performance regression".

>
>    ... and re-enabled it for v4.15 - but the false positives are still not fixed.

Right. But, all false positives cannot be fixed in a short period. We need
time to annotate them one by one.

>  - either the cross-release checking code can be fixed and then having it off by

It's not a problem of cross-release checking code. The way I have to fix it
should be to add additional annotation or change the way to assign lock
classes.

>    default is just wrong, because we can apply the fixed code again once it's
>    fixed.
>
>  - or it cannot be fixed (or we don't have the manpower/interest to fix it),
>    in which case having it off is only delaying the inevitable.

The more precisely assigning lock classes, the more false positives
would be getting fixed. It's not something messing it as time goes.

> In any case, for v4.15 it's clear that the false positives are too numerous.
>
> Thanks,
>
>         Ingo
>
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] locking/lockdep: Remove the cross-release locking checks

[Byungchul Park]

On Thu, Dec 14, 2017 at 2:01 PM, Byungchul Park
<max.byungchul.park@gmail.com> wrote:
> On Wed, Dec 13, 2017 at 7:46 PM, Ingo Molnar <mingo@kernel.org> wrote:
>>
>> * Byungchul Park <max.byungchul.park@gmail.com> wrote:
>>
>>> Lockdep works, based on the following:
>>>
>>>    (1) Classifying locks properly
>>>    (2) Checking relationship between the classes
>>>
>>> If (1) is not good or (2) is not good, then we
>>> might get false positives.
>>>
>>> For (1), we don't have to classify locks 100%
>>> properly but need as enough as lockdep works.
>>>
>>> For (2), we should have a mechanism w/o
>>> logical defects.
>>>
>>> Cross-release added an additional capacity to
>>> (2) and requires (1) to get more precisely classified.
>>>
>>> Since the current classification level is too low for
>>> cross-release to work, false positives are being
>>> reported frequently with enabling cross-release.
>>> Yes. It's a obvious problem. It needs to be off by
>>> default until the classification is done by the level
>>> that cross-release requires.
>>>
>>> But, the logic (2) is valid and logically true. Please
>>> keep the code, mechanism, and logic.
>>
>> Just to give a full context to everyone: the patch that removes the cross-release
>> locking checks was Cc:-ed to lkml, I've attached the patch below again.
>>
>> In general, as described in the changelog, the cross-release checks were
>> historically just too painful (first they were too slow, and they also had a lot
>> of false positives), and today, 4 months after its introduction, the cross-release
>> checks *still* produce numerous false positives, especially in the filesystem
>> space, but the continuous-integration testing folks were still having trouble with
>> kthread locking patterns causing false positives:
>
> I admit false positives are the main problem, that should be solved.
>
> I'm going willingly to try my best to solve that. However, as you may
> know through introduction of lockdep, it's not something that I can
> do easily and shortly on my own. It need take time to annotate
> properly to avoid false positives.
>
>>   https://bugs.freedesktop.org/show_bug.cgi?id=103950
>>
>> which were resulting in two bad reactions:
>>
>>  - turning off lockdep
>>
>>  - writing patches that uglified unrelated subsystems
>
> I can't give you a solution at the moment but it's something we
> think more so that we classify locks properly and not uglify them.
>
> Even without cross-release, once we start to add lock_acquire() in
> submit_bio_wait() in the ugly way to consider wait_for_completion()
> someday, we would face this problem again. It's not an easy problem,
> however, it's worth trying.
>
>> So while I appreciate the fixes that resulted from running cross-release, there's
>> still numerous false positives, months after its interaction, which is
>> unacceptable. For us to have this feature it has to have roughly similar qualities
>> as compiler warnings:
>>
>>  - there's a "zero false positive warnings" policy
>
> It's almost impossible... but need time. I wonder if lockdep did at the
> beginning. If I can, I want to fix false positive as many as possible by
> myself. But, unluckily it does not happen in my machine. I want to get
> informed from others, keeping it in mainline tree.
>
>>  - plus any widespread changes to avoid warnings has to improve the code,
>>    not make it uglier.
>
> Agree.
>
>> Lockdep itself is a following that policy: the default state is that it produces
>> no warnings upstream, and any annotations added to unrelated code documents the
>> locking hierarchies.
>>
>> While technically we could keep the cross-release checking code upstream and turn
>> it off by default via the Kconfig switch, I'm not a big believer in such a policy
>> for complex debugging code:
>>
>>  - We already did that for v4.14, two months ago:
>>
>>      b483cf3bc249: locking/lockdep: Disable cross-release features for now
>
> The main reason disabling it was "performance regression".
>
>>
>>    ... and re-enabled it for v4.15 - but the false positives are still not fixed.
>
> Right. But, all false positives cannot be fixed in a short period. We need
> time to annotate them one by one.
>
>>  - either the cross-release checking code can be fixed and then having it off by
>
> It's not a problem of cross-release checking code. The way I have to fix it
> should be to add additional annotation or change the way to assign lock
> classes.
>
>>    default is just wrong, because we can apply the fixed code again once it's
>>    fixed.
>>
>>  - or it cannot be fixed (or we don't have the manpower/interest to fix it),
>>    in which case having it off is only delaying the inevitable.
>
> The more precisely assigning lock classes, the more false positives
> would be getting fixed. It's not something messing it as time goes.
>
>> In any case, for v4.15 it's clear that the false positives are too numerous.
>>
>> Thanks,
>>
>>         Ingo
>>
>>

Hello all,

I'm against the removing, not only because it's my work.

We've already kept adding lock_acquire() manually to
consider wait_for_completion() or general waiters one by
one until now. It's certainly what we need.

Cross-release does it, but it makes trouble because it tries
to consider all waiters *at one time* instead of one by one.

Yes, it's a obvious problem. Doesn't we consider an option,
that is, to invalidate locks making trouble quickly and validate
it back one by one, so that almost other waiters can still get
benefit from cross-release?

Of course, it's not the ultimate solution. But, that would be
much better than stopping all the benefit at once.

For example, in the case of fs issues, for now we can
invalidate wait_for_completion() in submit_bio_wait() and
re-validate it later, of course, I really want to find more
fundamental solution though.

Is it possible?

-- 
Thanks,
Byungchul

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] locking/lockdep: Remove the cross-release locking checks

[Theodore Ts'o]

On Fri, Dec 15, 2017 at 01:05:43PM +0900, Byungchul Park wrote:
> For example, in the case of fs issues, for now we can
> invalidate wait_for_completion() in submit_bio_wait()....

And this will spawn a checkpatch.pl ERROR:

    	      	    ERROR("LOCKDEP",
		    "lockdep_no_validate class is reserved for device->mutex.\n" . $herecurr);

This mention in checkpatch.pl is the only documentation I've been able
to find about lockdep_set_novalidate_class(), by the way. 

> ... and re-validate it later, of course, I really want to find more
> fundamental solution though.

Oh, and I was finally able to find the quote that the *only* people
who are likely to be able to deal with lock annotations are the
subsystem maintainers.  But if the ways of dealing with lock
annotations are not documented, such that subsystem maintainers are
going to have a very hard time figuring out *how* to deal with it, it
seems that lock classification as a solution to cross-release false
positives seems.... unlikely:
   
   From: Byungchul Park <byungchul.park@lge.com>
   Date: Fri, 8 Dec 2017 18:27:45 +0900
   Subject: Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

   1) Firstly, it's hard to assign lock classes *properly*. By
   default, it relies on the caller site of lockdep_init_map(),
   but we need to assign another class manually, where ordering
   rules are complicated so cannot rely on the caller site. That
   *only* can be done by experts of the subsystem.

   	      	      	 	    	- Ted

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] locking/lockdep: Remove the cross-release locking checks

[Byungchul Park]

On Fri, Dec 15, 2017 at 3:24 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Fri, Dec 15, 2017 at 01:05:43PM +0900, Byungchul Park wrote:
>> For example, in the case of fs issues, for now we can
>> invalidate wait_for_completion() in submit_bio_wait()....
>
> And this will spawn a checkpatch.pl ERROR:
>
>                     ERROR("LOCKDEP",
>                     "lockdep_no_validate class is reserved for device->mutex.\n" . $herecurr);
>
> This mention in checkpatch.pl is the only documentation I've been able
> to find about lockdep_set_novalidate_class(), by the way.
>
>> ... and re-validate it later, of course, I really want to find more
>> fundamental solution though.
>
> Oh, and I was finally able to find the quote that the *only* people
> who are likely to be able to deal with lock annotations are the

Right. Using the word, "only", is one that I should not have done
and I apologize for.

They are just "only" people who solve and classify locks quickly,
assuming all of kernel guys are familiar with lockdep annotations.
Thus, even this statement is bad as well, since no good
document for that exists, as you pointed out. I agree.

> subsystem maintainers.  But if the ways of dealing with lock
> annotations are not documented, such that subsystem maintainers are
> going to have a very hard time figuring out *how* to deal with it, it

Right. I've agreed with this, since you pointed out it's problem not
to be documented well.

> seems that lock classification as a solution to cross-release false
> positives seems.... unlikely:
>
>    From: Byungchul Park <byungchul.park@lge.com>
>    Date: Fri, 8 Dec 2017 18:27:45 +0900
>    Subject: Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray
>
>    1) Firstly, it's hard to assign lock classes *properly*. By
>    default, it relies on the caller site of lockdep_init_map(),
>    but we need to assign another class manually, where ordering
>    rules are complicated so cannot rely on the caller site. That
>    *only* can be done by experts of the subsystem.
>
>                                         - Ted



-- 
Thanks,
Byungchul

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] locking/lockdep: Remove the cross-release locking checks

[Byungchul Park]

On Fri, Dec 15, 2017 at 3:24 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> seems that lock classification as a solution to cross-release false
> positives seems.... unlikely:

For this, let me explain more.

For example, either to use cross-release or to consider
wait_for_completion() in submit_bio_wait() manually using
lock_acquire() someday, classifying locks or waiters precisely
is needed.

All locks should belong to one class if each path of acquisition
can be switchable each other within the class at any time.
Otherwise, they should belong to a different class.

Even though they are different classes but belong to one class
roughly, no problem comes into view unless they are connected
each other via extra dependency chains. But, once they get
connected, we can see problems by the wrong classification.
That can happen even w/o cross-release.

Of course, as you pointed out, cross-release generates many
chains between classes, assuming all classes are well-
classified. But, practically well-classifying is not an easy work.

So that's why I suggested the way since anyway that's better
than removing all. If that's allowed, I can invalidate those waiters,
using e.g. completion_init_nomap().

>    From: Byungchul Park <byungchul.park@lge.com>
>    Date: Fri, 8 Dec 2017 18:27:45 +0900
>    Subject: Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray
>
>    1) Firstly, it's hard to assign lock classes *properly*. By
>    default, it relies on the caller site of lockdep_init_map(),
>    but we need to assign another class manually, where ordering
>    rules are complicated so cannot rely on the caller site. That
>    *only* can be done by experts of the subsystem.
>
>                                         - Ted



-- 
Thanks,
Byungchul

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] locking/lockdep: Remove the cross-release locking checks

[Theodore Ts'o]

On Fri, Dec 15, 2017 at 05:39:25PM +0900, Byungchul Park wrote:
> 
> All locks should belong to one class if each path of acquisition
> can be switchable each other within the class at any time.
> Otherwise, they should belong to a different class.

OK, so let's go back to my case of a Network Block Device with a local
file system mounted on it, which is then exported via NFS.

So an incoming TCP packet can go into the NFS server subsystem, then
be processed by local disk file system, which then does an I/O
operation to the NBD device, which results in an TCP packet being sent
out.  Then the response will come back over TCP, into the NBD block
layer, then into the local disk file system, and that will result in
an outgoing response to the TCP connection for the NFS protocol.

In order to avoid cross release problems, all locks associated with
the incoming TCP connection will need to be classified as belonging to
a different class as the outgoing TCP connection.  Correct?  One
solution might be to put every single TCP connection into a separate
class --- but that will explode the number of lock classes which
Lockdep will need to track, and there is a limited number of lock
classes (set at compile time) that Lockdep can track.  So if that
doesn't work, we will have to put something ugly which manually makes
certain TCP connections "magic" and require them to be put into a
separate class than all other TCP connections, which will get
collapsed into a single class.  Basically, any TCP connection which is
either originated by the kernel, or passed in from userspace into the
kernel and used by some kernel subsystem, will have to be assigned its
own lockdep class.

If the TCP connection gets closed, we don't need to track that lockdep
class any more.  (Or if a device mapper device is torn down, we
similarly don't need any unique lockdep classes created for that
device mapper device.)  Is there a way to tell lockdep that a set of
lockdep classes can be released so we can recover the kernel memory to
be used to track some new TCP connection or some new device mapper
device?

						- Ted

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] locking/lockdep: Remove the cross-release locking checks

[Byungchul Park]

On Sat, Dec 16, 2017 at 6:15 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Fri, Dec 15, 2017 at 05:39:25PM +0900, Byungchul Park wrote:
>>
>> All locks should belong to one class if each path of acquisition
>> can be switchable each other within the class at any time.
>> Otherwise, they should belong to a different class.
>
> OK, so let's go back to my case of a Network Block Device with a local
> file system mounted on it, which is then exported via NFS.
>
> So an incoming TCP packet can go into the NFS server subsystem, then
> be processed by local disk file system, which then does an I/O
> operation to the NBD device, which results in an TCP packet being sent
> out.  Then the response will come back over TCP, into the NBD block
> layer, then into the local disk file system, and that will result in
> an outgoing response to the TCP connection for the NFS protocol.
>
> In order to avoid cross release problems, all locks associated with
> the incoming TCP connection will need to be classified as belonging to
> a different class as the outgoing TCP connection.  Correct?  One
> solution might be to put every single TCP connection into a separate
> class --- but that will explode the number of lock classes which
> Lockdep will need to track, and there is a limited number of lock
> classes (set at compile time) that Lockdep can track.  So if that
> doesn't work, we will have to put something ugly which manually makes
> certain TCP connections "magic" and require them to be put into a
> separate class than all other TCP connections, which will get
> collapsed into a single class.  Basically, any TCP connection which is
> either originated by the kernel, or passed in from userspace into the
> kernel and used by some kernel subsystem, will have to be assigned its
> own lockdep class.
>
> If the TCP connection gets closed, we don't need to track that lockdep
> class any more.  (Or if a device mapper device is torn down, we
> similarly don't need any unique lockdep classes created for that
> device mapper device.)  Is there a way to tell lockdep that a set of
> lockdep classes can be released so we can recover the kernel memory to
> be used to track some new TCP connection or some new device mapper
> device?

Right. I also think lockdep should be able to reflect that
kind of dynamic situations to do a better job.

The fact that kernel works well w/o that work doesn't
mean current status is perfect, in my opinion.

As you know, lockdep is running within very limited
environment so it's very hard to achieve it.

However, anyway, I think that's a problem and should
be solved by modifying lockdep core. Actually, that had
been one on my to-dos, if allowed.

For some waiters, for which this is only solution to play
with cross-release, I think we can invalidate those
waiters for now, while all others still get benefit.

We have added acquire annotations manually to
consider waiters one by one, and I am sure it's going
to continue in the future.

IMO, considering all waiters at once and fixing false
positives in a right way or invalidating one by one is
better than considering waiters one by one as is, of
course, while keeping off by default.

-- 
Thanks,
Byungchul

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Byungchul Park]

On Wed, Dec 13, 2017 at 03:24:29PM +0900, Byungchul Park wrote:
> Lockdep works, based on the following:
> 
>    (1) Classifying locks properly
>    (2) Checking relationship between the classes
> 
> If (1) is not good or (2) is not good, then we
> might get false positives.
> 
> For (1), we don't have to classify locks 100%
> properly but need as enough as lockdep works.
> 
> For (2), we should have a mechanism w/o
> logical defects.
> 
> Cross-release added an additional capacity to
> (2) and requires (1) to get more precisely classified.
> 
> Since the current classification level is too low for
> cross-release to work, false positives are being
> reported frequently with enabling cross-release.
> Yes. It's a obvious problem. It needs to be off by
> default until the classification is done by the level
> that cross-release requires.
> 
> But, the logic (2) is valid and logically true. Please
> keep the code, mechanism, and logic.

I admit the cross-release feature had introduced several false positives
about 4 times(?), maybe. And I suggested roughly 3 ways to solve it. I
should have explained each in more detail. The lack might have led some
to misunderstand.

   (1) The best way: To classify all waiters correctly.

      Ultimately the problems should be solved in this way. But it
      takes a lot of time so it's not easy to use the way right away.
      And I need helps from experts of other sub-systems.

      While talking about this way, I made a trouble.. I still believe
      that each sub-system expert knows how to solve dependency problems
      most, since each has own dependency rule, but it was not about
      responsibility. I've never wanted to charge someone else it but me.

   (2) The 2nd way: To make cross-release off by default.

      At the beginning, I proposed cross-release being off by default.
      Honestly, I was happy and did it when Ingo suggested it on by
      default once lockdep on. But I shouldn't have done that but kept
      it off by default. Cross-release can make some happy but some
      unhappy until problems go away through (1) or (2).

   (3) The 3rd way: To invalidate waiters making trouble.

      Of course, this is not the best. Now that you have already spent
      a lot of time to fix original lockdep's problems since lockdep was
      introduced in 2006, we don't need to use this way for typical
      locks except a few special cases. Lockdep is fairly robust by now.

      And I understand you don't want to spend more time to fix
      additional problems again. Now that the situation is different
      from the time, 2006, it's not too bad to use this way to handle
      the issues.

IMO, the ways can be considered together at a time, which perhaps would
be even better.

Talking about what Ingo said in the commit msg.. I want to ask him back,
if he did it with no false positives at the moment merging it in 2006,
without using (2) or (3) method. I bet he know what it means.. And
classifying locks/waiters correctly is not something uglifying code but
a way to document code better. I've felt ill at ease because of the
unnatural and forced explanation.

--
Thanks,
Byungchul

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Byungchul Park]

On Fri, Dec 29, 2017 at 10:47:36AM +0900, Byungchul Park wrote:
> On Wed, Dec 13, 2017 at 03:24:29PM +0900, Byungchul Park wrote:
> > Lockdep works, based on the following:
> > 
> >    (1) Classifying locks properly
> >    (2) Checking relationship between the classes
> > 
> > If (1) is not good or (2) is not good, then we
> > might get false positives.
> > 
> > For (1), we don't have to classify locks 100%
> > properly but need as enough as lockdep works.
> > 
> > For (2), we should have a mechanism w/o
> > logical defects.
> > 
> > Cross-release added an additional capacity to
> > (2) and requires (1) to get more precisely classified.
> > 
> > Since the current classification level is too low for
> > cross-release to work, false positives are being
> > reported frequently with enabling cross-release.
> > Yes. It's a obvious problem. It needs to be off by
> > default until the classification is done by the level
> > that cross-release requires.
> > 
> > But, the logic (2) is valid and logically true. Please
> > keep the code, mechanism, and logic.
> 
> I admit the cross-release feature had introduced several false positives
> about 4 times(?), maybe. And I suggested roughly 3 ways to solve it. I
> should have explained each in more detail. The lack might have led some
> to misunderstand.
> 
>    (1) The best way: To classify all waiters correctly.
> 
>       Ultimately the problems should be solved in this way. But it
>       takes a lot of time so it's not easy to use the way right away.
>       And I need helps from experts of other sub-systems.
> 
>       While talking about this way, I made a trouble.. I still believe
>       that each sub-system expert knows how to solve dependency problems
>       most, since each has own dependency rule, but it was not about
>       responsibility. I've never wanted to charge someone else it but me.
> 
>    (2) The 2nd way: To make cross-release off by default.
> 
>       At the beginning, I proposed cross-release being off by default.
>       Honestly, I was happy and did it when Ingo suggested it on by
>       default once lockdep on. But I shouldn't have done that but kept
>       it off by default. Cross-release can make some happy but some
>       unhappy until problems go away through (1) or (2).
> 
>    (3) The 3rd way: To invalidate waiters making trouble.
> 
>       Of course, this is not the best. Now that you have already spent
>       a lot of time to fix original lockdep's problems since lockdep was
>       introduced in 2006, we don't need to use this way for typical
>       locks except a few special cases. Lockdep is fairly robust by now.
> 
>       And I understand you don't want to spend more time to fix
>       additional problems again. Now that the situation is different
>       from the time, 2006, it's not too bad to use this way to handle
>       the issues.
> 
> IMO, the ways can be considered together at a time, which perhaps would
> be even better.

+cc daniel@ffwll.ch

> Talking about what Ingo said in the commit msg.. I want to ask him back,

I'm sorry for missing specifying the commit I'm talking about.

   e966eaeeb locking/lockdep: Remove the cross-release locking checks

> if he did it with no false positives at the moment merging it in 2006,
> without using (2) or (3) method. I bet he know what it means.. And
> classifying locks/waiters correctly is not something uglifying code but
> a way to document code better. I've felt ill at ease because of the
> unnatural and forced explanation.
> 
> --
> Thanks,
> Byungchul

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Theodore Ts'o]

On Fri, Dec 29, 2017 at 10:47:36AM +0900, Byungchul Park wrote:
> 
>    (1) The best way: To classify all waiters correctly.

It's really not all waiters, but all *locks*, no?

>       Ultimately the problems should be solved in this way. But it
>       takes a lot of time so it's not easy to use the way right away.
>       And I need helps from experts of other sub-systems.
> 
>       While talking about this way, I made a trouble.. I still believe
>       that each sub-system expert knows how to solve dependency problems
>       most, since each has own dependency rule, but it was not about
>       responsibility. I've never wanted to charge someone else it but me.

The problem is that it's not one subsystem, but *many*.  And it's the
interactions between the subsystems.

Consider the example I gave of a network block device, on which a
local disk file system is mounted, which is then exported over NFS.
So we have the Networking (TCP) stack involved, the NBD device driver,
the local disk file system, the NFS file system, and the networking
stack a second time.  That is *many* subsystem maintainers who have to
get involved.

In addition, the lock classification system is not documented at all,
so now you also need someone who understands the lockdep code.  And
since some of these classifications involve transient objects, and
lockdep doesn't have a way of dealing with transient locks, and has a
hard compile time limit of the number of locks that it supports, to
expect a subsystem maintainer to figure out all of the interactions,
plus figure out lockdep, and work around lockdep's limitations
seems.... not realistic.

(By the way, I've tried reading the crosslock and crossrelease
documentation --- and I'm lost.  Sorry, I'm just not smart enough to
understand how it works, at least not from reading the documentation
that was in the patch series.  And honestly, I don't care.  All I do
need is some practical instructions for how to "classify locks
properly", and how this interacts with lockdep's limitations.)

>    (2) The 2nd way: To make cross-release off by default.
> 
>       At the beginning, I proposed cross-release being off by default.
>       Honestly, I was happy and did it when Ingo suggested it on by
>       default once lockdep on. But I shouldn't have done that but kept
>       it off by default. Cross-release can make some happy but some
>       unhappy until problems go away through (1) or (2).

Ingo's argument is that (1) is not going to be happening any time
soon, and in the meantime, code which is turned off will bitrot.

Given that once Lockdep reports a locking violation, it doesn't report
any more lockdep violations, if there are a large number of false
positives, people will not want to turn on cross-release, since it
will report the false positive and then turn itself off, so it won't
report anything useful.  So if no one turns it on because of the false
positives, how does the bitrot problem get resolved?

And if the answer is that some small number of lockdep experts will be
trying to figure out how to do (1) in a tractable way, then Ingo has
argued it can be handled via an out-of-tree patch.

>    (3) The 3rd way: To invalidate waiters making trouble.

Hmm, can we make cross-release and cross-lock off by default on a per
lock basis?  With a well documented to enable it?  I'm still not sure
how this works given the cross-subsystem problem, though.

So if networking enables it because there are no problems with their
TCP-only test, and then it blows up when someone is doing NBD or NFS
testing, what's the recourse?  The file system developer submitting a
patch against the networking subsystem to turn off the lockdep
tracking on that particular lock because it's causing pain for the
file system developer?  I can see that potentially causing all sorts
of inter-subsystem conflicts.

> Talking about what Ingo said in the commit msg.. I want to ask him back,
> if he did it with no false positives at the moment merging it in 2006,
> without using (2) or (3) method. I bet he know what it means.. And
> classifying locks/waiters correctly is not something uglifying code but
> a way to document code better. I've felt ill at ease because of the
> unnatural and forced explanation.

So I think this is the big difference is that potential for
cross-subsystem false positives is dramatically higher than with
cross-release compared with the traditional lockdep.  And I'm not sure
there is a clean solution --- how do you "cleanly classify" locks when
in some cases each object's locks needs to be considered individual
locks, and when that must not be done lest there is an explosion of
the number of locks which lockdep needs to track (which is strictly
limited due to memory and CPU overhead, as I understand things)?  I
haven't seen an explanation for how to solve this in a clean, general
way --- and I strongly suspect it doesn't exist.

Regards,

						- Ted

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Byungchul Park]

On Thu, Dec 28, 2017 at 10:51:46PM -0500, Theodore Ts'o wrote:
> On Fri, Dec 29, 2017 at 10:47:36AM +0900, Byungchul Park wrote:
> > 
> >    (1) The best way: To classify all waiters correctly.
> 
> It's really not all waiters, but all *locks*, no?

Thanks for your opinion. I will add my opinion on you.

I meant *waiters*. Locks are only a sub set of potential waiters, which
actually cause deadlocks. Cross-release was designed to consider the
super set including all general waiters such as typical locks,
wait_for_completion(), and lock_page() and so on..

> >       Ultimately the problems should be solved in this way. But it
> >       takes a lot of time so it's not easy to use the way right away.
> >       And I need helps from experts of other sub-systems.
> > 
> >       While talking about this way, I made a trouble.. I still believe
> >       that each sub-system expert knows how to solve dependency problems
> >       most, since each has own dependency rule, but it was not about
> >       responsibility. I've never wanted to charge someone else it but me.
> 
> The problem is that it's not one subsystem, but *many*.  And it's the
> interactions between the subsystems.
> 
> Consider the example I gave of a network block device, on which a
> local disk file system is mounted, which is then exported over NFS.
> So we have the Networking (TCP) stack involved, the NBD device driver,
> the local disk file system, the NFS file system, and the networking
> stack a second time.  That is *many* subsystem maintainers who have to
> get involved.

I admit that it's not simple one to solve..

> In addition, the lock classification system is not documented at all,
> so now you also need someone who understands the lockdep code.  And
> since some of these classifications involve transient objects, and
> lockdep doesn't have a way of dealing with transient locks, and has a
> hard compile time limit of the number of locks that it supports, to
> expect a subsystem maintainer to figure out all of the interactions,
> plus figure out lockdep, and work around lockdep's limitations
> seems.... not realistic.

I have to think it more to find out how to solve it simply enough to be
acceptable. The only solution I come up with for now is too complex.

> (By the way, I've tried reading the crosslock and crossrelease
> documentation --- and I'm lost.  Sorry, I'm just not smart enough to
> understand how it works, at least not from reading the documentation
> that was in the patch series.  And honestly, I don't care.  All I do

I am sorry for that. My english is too bad.. I can explain whatever you
wonder if you ask me.

> need is some practical instructions for how to "classify locks
> properly", and how this interacts with lockdep's limitations.)

I see what you consider. As you know, it's not something that I can
solve right away. That's why I suggested (2) or (3)..

> >    (2) The 2nd way: To make cross-release off by default.
> > 
> >       At the beginning, I proposed cross-release being off by default.
> >       Honestly, I was happy and did it when Ingo suggested it on by
> >       default once lockdep on. But I shouldn't have done that but kept
> >       it off by default. Cross-release can make some happy but some
> >       unhappy until problems go away through (1) or (2).
                                                         ^
                                                 should be (3)

> Ingo's argument is that (1) is not going to be happening any time
> soon, and in the meantime, code which is turned off will bitrot.

The root cause of the problem is that locks, generally speaking, waiters
are roughly classified. IOW, having the new code with a better
classification is worth, even it would be done later.

> Given that once Lockdep reports a locking violation, it doesn't report
> any more lockdep violations, if there are a large number of false
> positives, people will not want to turn on cross-release, since it
> will report the false positive and then turn itself off, so it won't
> report anything useful.  So if no one turns it on because of the false
> positives, how does the bitrot problem get resolved?

The problems come from wrong classification. Waiters either classfied
well or invalidated properly won't bitrot.

> And if the answer is that some small number of lockdep experts will be
> trying to figure out how to do (1) in a tractable way, then Ingo has
> argued it can be handled via an out-of-tree patch.
> 
> >    (3) The 3rd way: To invalidate waiters making trouble.
> 
> Hmm, can we make cross-release and cross-lock off by default on a per
> lock basis?  With a well documented to enable it?  I'm still not sure

Yes. More precisely speaking, we can make cross-release check off on a
per waiter basis, for example, by using init_completion_nomap() or its
family which I can provide if needed, leaving other traditional lockdep
checking *unchanged*. For that issue we talked about, we could use it in
submit_bio_wait() to invalidate the checking for the waiter.

> how this works given the cross-subsystem problem, though.

It works because the invalidation make lockdep not generate the link
between a set of fs locks on a layer and another set on another layer.

> So if networking enables it because there are no problems with their
> TCP-only test, and then it blows up when someone is doing NBD or NFS
> testing, what's the recourse?  The file system developer submitting a
> patch against the networking subsystem to turn off the lockdep
> tracking on that particular lock because it's causing pain for the
> file system developer?  I can see that potentially causing all sorts
> of inter-subsystem conflicts.

If it can never be solved anyway, we can invalidate the waiter. What I
want to say is that it's better than nothing, since cross-release would
work and give the benefit in most cases, except that complicated case.

> > Talking about what Ingo said in the commit msg.. I want to ask him back,
> > if he did it with no false positives at the moment merging it in 2006,
> > without using (2) or (3) method. I bet he know what it means.. And
> > classifying locks/waiters correctly is not something uglifying code but
> > a way to document code better. I've felt ill at ease because of the
> > unnatural and forced explanation.
> 
> So I think this is the big difference is that potential for
> cross-subsystem false positives is dramatically higher than with
> cross-release compared with the traditional lockdep.  And I'm not sure
> there is a clean solution --- how do you "cleanly classify" locks when
> in some cases each object's locks needs to be considered individual
> locks, and when that must not be done lest there is an explosion of
> the number of locks which lockdep needs to track (which is strictly
> limited due to memory and CPU overhead, as I understand things)?  I
> haven't seen an explanation for how to solve this in a clean, general
> way --- and I strongly suspect it doesn't exist.

I think this is the main point you want to point out anyway. Couldn't we
why, if we try in one way or another?

For example, we can introduce the concept of group so classes in each
group can be distinguished from another, of course, there might also be
many things to discuss though.

--
Thanks,
Byungchul

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Matthew Wilcox]

On Fri, Dec 29, 2017 at 04:28:51PM +0900, Byungchul Park wrote:
> On Thu, Dec 28, 2017 at 10:51:46PM -0500, Theodore Ts'o wrote:
> > On Fri, Dec 29, 2017 at 10:47:36AM +0900, Byungchul Park wrote:
> > > 
> > >    (1) The best way: To classify all waiters correctly.
> > 
> > It's really not all waiters, but all *locks*, no?
> 
> Thanks for your opinion. I will add my opinion on you.
> 
> I meant *waiters*. Locks are only a sub set of potential waiters, which
> actually cause deadlocks. Cross-release was designed to consider the
> super set including all general waiters such as typical locks,
> wait_for_completion(), and lock_page() and so on..

I think this is a terminology problem.  To me (and, I suspect Ted), a
waiter is a subject of a verb while a lock is an object.  So Ted is asking
whether we have to classify the users, while I think you're saying we
have extra objects to classify.

I'd be comfortable continuing to refer to completions as locks.  We could
try to come up with a new object name like waitpoints though?

> > In addition, the lock classification system is not documented at all,
> > so now you also need someone who understands the lockdep code.  And
> > since some of these classifications involve transient objects, and
> > lockdep doesn't have a way of dealing with transient locks, and has a
> > hard compile time limit of the number of locks that it supports, to
> > expect a subsystem maintainer to figure out all of the interactions,
> > plus figure out lockdep, and work around lockdep's limitations
> > seems.... not realistic.
> 
> I have to think it more to find out how to solve it simply enough to be
> acceptable. The only solution I come up with for now is too complex.

I want to amplify Ted's point here.  How to use the existing lockdep
functionality is undocumented.  And that's not your fault.  We have
Documentation/locking/lockdep-design.txt which I'm sure is great for
someone who's willing to invest a week understanding it, but we need a
"here's how to use it" guide.

> > Given that once Lockdep reports a locking violation, it doesn't report
> > any more lockdep violations, if there are a large number of false
> > positives, people will not want to turn on cross-release, since it
> > will report the false positive and then turn itself off, so it won't
> > report anything useful.  So if no one turns it on because of the false
> > positives, how does the bitrot problem get resolved?
> 
> The problems come from wrong classification. Waiters either classfied
> well or invalidated properly won't bitrot.

I disagree here.  As Ted says, it's the interactions between the
subsystems that leads to problems.  Everything's goig to work great
until somebody does something in a way that's never been tried before.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Theodore Ts'o]

On Fri, Dec 29, 2017 at 10:16:24PM -0800, Matthew Wilcox wrote:
> 
> I think this is a terminology problem.  To me (and, I suspect Ted), a
> waiter is a subject of a verb while a lock is an object.  So Ted is asking
> whether we have to classify the users, while I think you're saying we
> have extra objects to classify.

Exactly, the classification is applied when the {lock, mutex,
completion} object is initialized.  Not currently at the individual
call points to mutex_lock(), wait_for_completion(), down_write(), etc.


> > The problems come from wrong classification. Waiters either classfied
> > well or invalidated properly won't bitrot.
> 
> I disagree here.  As Ted says, it's the interactions between the
> subsystems that leads to problems.  Everything's goig to work great
> until somebody does something in a way that's never been tried before.

The question what is classified *well* mean?  At the extreme, we could
put the locks for every single TCP connection into their own lockdep
class.  But that would blow the limits in terms of the number of locks
out of the water super-quickly --- and it would destroy the ability
for lockdep to learn what the proper locking order should be.  Yet
given Lockdep's current implementation, the only way to guarantee that
there won't be any interactions between subsystems that cause false
positives would be to categorizes locks for each TCP connection into
their own class.

So this is why I get a little annoyed when you say, "it's just a
matter of classification".  NO IT IS NOT.  We can not possibly
classify things "correctly" to completely limit false positives
without completely destroying lockdep's scalability as it is currently
designed.  Byungchul, you don't acknowledge this, and it makes the
"just classify everything" argument completely suspect as a result.

As far as the "just invalidate the waiter", the problem is that it
requires source level changes to invalidate the waiter, and for
different use cases, we will need to validate different waiters.  For
example, in the example I gave, we would have to invalidate *all* TCP
waiters/locks in order to prevent false positives.  But that makes the
lockdep useless for all TCP locks.  What's the solution?  I claim that
until lockdep is fundamentally fixed, there is no way to eliminate
*all* false positives without invalidating *all*
cross-release/cross-locks --- in which case you might as well leave
the cross-release patches as an out of tree patch.

So to claim that we can somehow fix the problem by making source-level
changes outside of lockdep, by "properly classifying" or "properly
invalidating" all locks, just doesn't make sense. 

The only way it can work is to either dump it on the reposibility of
the people debugging lockdep reports to make source level changes to
other subsystems which they aren't the maintainers of to suppress
false positives that arise due to how the subsystems are being used
together in their particular configuration ---- or you can try to
claim that there is an "acceptable level" of false positives with
which we can live with forever, and which can not be fixed by "proper
classifying" the locks.

Or you can try to make lockdep scalable enough that if we could put
every single lock for every single object into its own lock class
(e.g., each lock for every single TCP connection gets its own lock
class) which is after all the only way we can "properly classify
everything") and still let lockdep be useful.

If you think that is doable, why don't you work on that, and once that
is done, maybe cross-locks lockdep will be considered more acceptable
for mainstream?

					- Ted

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Matthew Wilcox]

On Sat, Dec 30, 2017 at 10:40:41AM -0500, Theodore Ts'o wrote:
> On Fri, Dec 29, 2017 at 10:16:24PM -0800, Matthew Wilcox wrote:
> > > The problems come from wrong classification. Waiters either classfied
> > > well or invalidated properly won't bitrot.
> > 
> > I disagree here.  As Ted says, it's the interactions between the
> > subsystems that leads to problems.  Everything's goig to work great
> > until somebody does something in a way that's never been tried before.
> 
> The question what is classified *well* mean?  At the extreme, we could
> put the locks for every single TCP connection into their own lockdep
> class.  But that would blow the limits in terms of the number of locks
> out of the water super-quickly --- and it would destroy the ability
> for lockdep to learn what the proper locking order should be.  Yet
> given Lockdep's current implementation, the only way to guarantee that
> there won't be any interactions between subsystems that cause false
> positives would be to categorizes locks for each TCP connection into
> their own class.

I'm not sure I agree with this part.  What if we add a new TCP lock class
for connections which are used for filesystems/network block devices/...?
Yes, it'll be up to each user to set the lockdep classification correctly,
but that's a relatively small number of places to add annotations,
and I don't see why it wouldn't work.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Theodore Ts'o]

On Sat, Dec 30, 2017 at 12:44:17PM -0800, Matthew Wilcox wrote:
> 
> I'm not sure I agree with this part.  What if we add a new TCP lock class
> for connections which are used for filesystems/network block devices/...?
> Yes, it'll be up to each user to set the lockdep classification correctly,
> but that's a relatively small number of places to add annotations,
> and I don't see why it wouldn't work.

I was exagerrating a bit for effect, I admit.  (but only a bit).

It can probably be for all TCP connections that are used by kernel
code (as opposed to userspace-only TCP connections).  But it would
probably have to be each and every device-mapper instance, each and
every block device, each and every mounted file system, each and every
bdi object, etc.

The point I was trying to drive home is that "all we have to do is
just classify everything well or just invalidate the right lock
objects" is a massive understatement of the complexity level of what
would be required, or the number of locks/completion handlers that
would have to be blacklisted.

						- Ted

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Theodore Ts'o]

On Sat, Dec 30, 2017 at 05:40:28PM -0500, Theodore Ts'o wrote:
> On Sat, Dec 30, 2017 at 12:44:17PM -0800, Matthew Wilcox wrote:
> > 
> > I'm not sure I agree with this part.  What if we add a new TCP lock class
> > for connections which are used for filesystems/network block devices/...?
> > Yes, it'll be up to each user to set the lockdep classification correctly,
> > but that's a relatively small number of places to add annotations,
> > and I don't see why it wouldn't work.
> 
> I was exagerrating a bit for effect, I admit.  (but only a bit).
> 
> It can probably be for all TCP connections that are used by kernel
> code (as opposed to userspace-only TCP connections).  But it would
> probably have to be each and every device-mapper instance, each and
> every block device, each and every mounted file system, each and every
> bdi object, etc.

Clarification: all TCP connections that are used by kernel code would
need to be in their own separate lock class.  All TCP connections used
only by userspace could be in their own shared lock class.  You can't
use a one lock class for all kernel-used TCP connections, because of
the Network Block Device mounted on a local file system which is then
exported via NFS and squirted out yet another TCP connection problem.

Also, what to do with TCP connections which are created in userspace
(with some authentication exchanges happening in userspace), and then
passed into kernel space for use in kernel space, is an interesting
question.

So "all you have to do is classify the locks 'properly'" is much like
the apocrophal, "all you have to do is bell the cat"[1].  Or like the
saying, "colonizing the stars is *easy*; all you have to do is figure
out faster than light travel."

[1] https://en.wikipedia.org/wiki/Belling_the_cat

							- Ted

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Matthew Wilcox]

On Sat, Dec 30, 2017 at 06:00:57PM -0500, Theodore Ts'o wrote:
> On Sat, Dec 30, 2017 at 05:40:28PM -0500, Theodore Ts'o wrote:
> > On Sat, Dec 30, 2017 at 12:44:17PM -0800, Matthew Wilcox wrote:
> > > 
> > > I'm not sure I agree with this part.  What if we add a new TCP lock class
> > > for connections which are used for filesystems/network block devices/...?
> > > Yes, it'll be up to each user to set the lockdep classification correctly,
> > > but that's a relatively small number of places to add annotations,
> > > and I don't see why it wouldn't work.
> > 
> > I was exagerrating a bit for effect, I admit.  (but only a bit).

I feel like there's been rather too much of that recently.  Can we stick
to facts as far as possible, please?

> > It can probably be for all TCP connections that are used by kernel
> > code (as opposed to userspace-only TCP connections).  But it would
> > probably have to be each and every device-mapper instance, each and
> > every block device, each and every mounted file system, each and every
> > bdi object, etc.
> 
> Clarification: all TCP connections that are used by kernel code would
> need to be in their own separate lock class.  All TCP connections used
> only by userspace could be in their own shared lock class.  You can't
> use a one lock class for all kernel-used TCP connections, because of
> the Network Block Device mounted on a local file system which is then
> exported via NFS and squirted out yet another TCP connection problem.

So the false positive you're concerned about is write-comes-in-over-NFS
(with socket lock held), NFS sends a write request to local filesystem,
local filesystem sends write to block device, block device sends a
packet to a socket which takes that socket lock.

I don't think we need to be as drastic as giving each socket its own lock
class to solve this.  All NFS sockets can be in lock class A; all NBD
sockets can be in lock class B; all user sockets can be in lock class
C; etc.

> Also, what to do with TCP connections which are created in userspace
> (with some authentication exchanges happening in userspace), and then
> passed into kernel space for use in kernel space, is an interesting
> question.

Yes!  I'd love to have a lockdep expert weigh in here.  I believe it's
legitimate to change a lock's class after it's been used, essentially
destroying it and reinitialising it.  If not, it should be because it's
a reasonable design for an object to need different lock classes for
different phases of its existance.

> So "all you have to do is classify the locks 'properly'" is much like
> the apocrophal, "all you have to do is bell the cat"[1].  Or like the
> saying, "colonizing the stars is *easy*; all you have to do is figure
> out faster than light travel."

This is only computer programming, not rocket surgery :-)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Theodore Ts'o]

On Mon, Jan 01, 2018 at 02:18:55AM -0800, Matthew Wilcox wrote:
> > Clarification: all TCP connections that are used by kernel code would
> > need to be in their own separate lock class.  All TCP connections used
> > only by userspace could be in their own shared lock class.  You can't
> > use a one lock class for all kernel-used TCP connections, because of
> > the Network Block Device mounted on a local file system which is then
> > exported via NFS and squirted out yet another TCP connection problem.
> 
> So the false positive you're concerned about is write-comes-in-over-NFS
> (with socket lock held), NFS sends a write request to local filesystem,
> local filesystem sends write to block device, block device sends a
> packet to a socket which takes that socket lock.

It's not just the socket lock, but any of the locks/mutexes/"waiters"
that might be taken in the TCP code path and below, including in the
NIC driver.

> I don't think we need to be as drastic as giving each socket its own lock
> class to solve this.  All NFS sockets can be in lock class A; all NBD
> sockets can be in lock class B; all user sockets can be in lock class
> C; etc.

But how do you know which of the locks taken in the networking stack
are for the NBD versus the NFS sockets?  What manner of horrific
abstraction violation is going to pass that information all the way
down to all of the locks that might be taken at the socket layer and
below?

How is this "proper clasification" supposed to happen?  It's the
repeated handwaving which claims this is easy which is rather
frustrating.  The simple thing is to use a unique ID which is bumped
for each struct sock, each struct super, struct block_device, struct
request_queue, struct bdi, etc, but that runs into lockdep scalability
issues.

Anything else means that you have to somehow pass down through the
layers so that, in the general case, the socket knows that it is "an
NFS socket" versus "an NBD socket" --- and remember, if there is any
kind of completion handling done in the NIC driver, it's going to have
to passed down well below the TCP layer all the way down to the
network device drivers.  Or is the plan to do this add a bit ad hoc of
plumbing for each false positive which cross-release lockdep failures
are reported?

> > Also, what to do with TCP connections which are created in userspace
> > (with some authentication exchanges happening in userspace), and then
> > passed into kernel space for use in kernel space, is an interesting
> > question.
> 
> Yes!  I'd love to have a lockdep expert weigh in here.  I believe it's
> legitimate to change a lock's class after it's been used, essentially
> destroying it and reinitialising it.  If not, it should be because it's
> a reasonable design for an object to need different lock classes for
> different phases of its existance.

We just also need to be destroy a lock class after the transient
object has been deleted.  This is especially true for file system
testing, since we are constantly mounting and unmounting file systems,
and creating and destroying loop devices, potentially hundreds or
thousands of times during a test run.  So if we have to create a
unique lock class for "proper classification" each time a file system
is mounted, or loop device or device-mapper device (dm-error, etc.) is
created, we'll run into lockdep scalability issues really quickly.

So this is yet another example where the handwaving, "all you have to
do is proper classification" just doesn't work.

> > So "all you have to do is classify the locks 'properly'" is much like
> > the apocrophal, "all you have to do is bell the cat"[1].  Or like the
> > saying, "colonizing the stars is *easy*; all you have to do is figure
> > out faster than light travel."
> 
> This is only computer programming, not rocket surgery :-)

Given the current state of the lockdep technology, merging cross-lock
certainly feels like requiring the use of sledgehammers to do rocket
surgery in order to avoid false positives --- sorry, "proper
classification".

					- Ted

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Byungchul Park]

On 1/2/2018 1:00 AM, Theodore Ts'o wrote:
> On Mon, Jan 01, 2018 at 02:18:55AM -0800, Matthew Wilcox wrote:
>>> Clarification: all TCP connections that are used by kernel code would
>>> need to be in their own separate lock class.  All TCP connections used
>>> only by userspace could be in their own shared lock class.  You can't
>>> use a one lock class for all kernel-used TCP connections, because of
>>> the Network Block Device mounted on a local file system which is then
>>> exported via NFS and squirted out yet another TCP connection problem.
>>
>> So the false positive you're concerned about is write-comes-in-over-NFS
>> (with socket lock held), NFS sends a write request to local filesystem,
>> local filesystem sends write to block device, block device sends a
>> packet to a socket which takes that socket lock.
> 
> It's not just the socket lock, but any of the locks/mutexes/"waiters"
> that might be taken in the TCP code path and below, including in the
> NIC driver.
> 
>> I don't think we need to be as drastic as giving each socket its own lock
>> class to solve this.  All NFS sockets can be in lock class A; all NBD
>> sockets can be in lock class B; all user sockets can be in lock class
>> C; etc.
> 
> But how do you know which of the locks taken in the networking stack
> are for the NBD versus the NFS sockets?  What manner of horrific
> abstraction violation is going to pass that information all the way
> down to all of the locks that might be taken at the socket layer and
> below?
> 
> How is this "proper clasification" supposed to happen?  It's the
> repeated handwaving which claims this is easy which is rather
> frustrating.  The simple thing is to use a unique ID which is bumped
> for each struct sock, each struct super, struct block_device, struct
> request_queue, struct bdi, etc, but that runs into lockdep scalability
> issues.

This is what I mentioned with group ID in an example for you before.
To do that, the most important thing is to prevent running into
lockdep scalability.

-- 
Thanks,
Byungchul

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Byungchul Park]

On 1/1/2018 7:18 PM, Matthew Wilcox wrote:
> On Sat, Dec 30, 2017 at 06:00:57PM -0500, Theodore Ts'o wrote:
>> On Sat, Dec 30, 2017 at 05:40:28PM -0500, Theodore Ts'o wrote:
>>> On Sat, Dec 30, 2017 at 12:44:17PM -0800, Matthew Wilcox wrote:
>>>>
>>>> I'm not sure I agree with this part.  What if we add a new TCP lock class
>>>> for connections which are used for filesystems/network block devices/...?
>>>> Yes, it'll be up to each user to set the lockdep classification correctly,
>>>> but that's a relatively small number of places to add annotations,
>>>> and I don't see why it wouldn't work.
>>>
>>> I was exagerrating a bit for effect, I admit.  (but only a bit).
> 
> I feel like there's been rather too much of that recently.  Can we stick
> to facts as far as possible, please?
> 
>>> It can probably be for all TCP connections that are used by kernel
>>> code (as opposed to userspace-only TCP connections).  But it would
>>> probably have to be each and every device-mapper instance, each and
>>> every block device, each and every mounted file system, each and every
>>> bdi object, etc.
>>
>> Clarification: all TCP connections that are used by kernel code would
>> need to be in their own separate lock class.  All TCP connections used
>> only by userspace could be in their own shared lock class.  You can't
>> use a one lock class for all kernel-used TCP connections, because of
>> the Network Block Device mounted on a local file system which is then
>> exported via NFS and squirted out yet another TCP connection problem.
> 
> So the false positive you're concerned about is write-comes-in-over-NFS
> (with socket lock held), NFS sends a write request to local filesystem,
> local filesystem sends write to block device, block device sends a
> packet to a socket which takes that socket lock.
> 
> I don't think we need to be as drastic as giving each socket its own lock
> class to solve this.  All NFS sockets can be in lock class A; all NBD
> sockets can be in lock class B; all user sockets can be in lock class
> C; etc.
> 
>> Also, what to do with TCP connections which are created in userspace
>> (with some authentication exchanges happening in userspace), and then
>> passed into kernel space for use in kernel space, is an interesting
>> question.
> 
> Yes!  I'd love to have a lockdep expert weigh in here.  I believe it's
> legitimate to change a lock's class after it's been used, essentially
> destroying it and reinitialising it.  If not, it should be because it's
> a reasonable design for an object to need different lock classes for
> different phases of its existance.

I also think it should be done ultimately. And I think it's very much
hard since it requires to change the dependency graph of lockdep but
anyway possible. It's up to lockdep maintainer's will though..

-- 
Thanks,
Byungchul

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Dave Chinner]

On Wed, Jan 03, 2018 at 11:28:44AM +0900, Byungchul Park wrote:
> On 1/1/2018 7:18 PM, Matthew Wilcox wrote:
> >On Sat, Dec 30, 2017 at 06:00:57PM -0500, Theodore Ts'o wrote:
> >>Also, what to do with TCP connections which are created in userspace
> >>(with some authentication exchanges happening in userspace), and then
> >>passed into kernel space for use in kernel space, is an interesting
> >>question.
> >
> >Yes!  I'd love to have a lockdep expert weigh in here.  I believe it's
> >legitimate to change a lock's class after it's been used, essentially
> >destroying it and reinitialising it.  If not, it should be because it's
> >a reasonable design for an object to need different lock classes for
> >different phases of its existance.
> 
> I also think it should be done ultimately. And I think it's very much
> hard since it requires to change the dependency graph of lockdep but
> anyway possible. It's up to lockdep maintainer's will though..

We used to do this in XFS to work around the fact that the memory
reclaim context "locks" were too stupid to understand that an object
referenced and locked above memory allocation could not be
accessed below in memory reclaim because memory reclaim only accesses
/unreferenced objects/. We played whack-a-mole with lockdep for
years to get most of the false positives sorted out.

Hence for a long time we had to re-initialise the lock context for
the XFS inode iolock in ->evict_inode() so we could lock it for
reclaim processing.  Eventually we ended up completely reworking the
inode reclaim locking in XFS primarily to get rid of all the nasty
lockdep hacks we had strewn throughout the code. It was ~2012 we
got rid of the last inode re-init code, IIRC. Yeah:

commit 4f59af758f9092bc7b266ca919ce6067170e5172
Author: Christoph Hellwig <hch@infradead.org>
Date:   Wed Jul 4 11:13:33 2012 -0400

    xfs: remove iolock lock classes
    
    Now that we never take the iolock during inode reclaim we don't need
    to play games with lock classes.
    
    Signed-off-by: Christoph Hellwig <hch@lst.de>
    Reviewed-by: Rich Johnston <rjohnston@sgi.com>
    Signed-off-by: Ben Myers <bpm@sgi.com>



We still have problems with lockdep false positives w.r.t. memory
allocation contexts, mainly with code that can be called from
both above and below memory allocation contexts. We've finally
got __GFP_NOLOCKDEP to be able to annotate memory allocation points
within such code paths, but that doesn't help with locks....

Byungchul, lockdep has a long, long history of having sharp edges
and being very unfriendly to developers. We've all been scarred by
lockdep at one time or another and so there's a fair bit of
resistance to repeating past mistakes and allowing lockdep to
inflict more scars on us....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Byungchul Park]

On 1/3/2018 11:58 AM, Dave Chinner wrote:
> On Wed, Jan 03, 2018 at 11:28:44AM +0900, Byungchul Park wrote:
>> On 1/1/2018 7:18 PM, Matthew Wilcox wrote:
>>> On Sat, Dec 30, 2017 at 06:00:57PM -0500, Theodore Ts'o wrote:
>>>> Also, what to do with TCP connections which are created in userspace
>>>> (with some authentication exchanges happening in userspace), and then
>>>> passed into kernel space for use in kernel space, is an interesting
>>>> question.
>>>
>>> Yes!  I'd love to have a lockdep expert weigh in here.  I believe it's
>>> legitimate to change a lock's class after it's been used, essentially
>>> destroying it and reinitialising it.  If not, it should be because it's
>>> a reasonable design for an object to need different lock classes for
>>> different phases of its existance.
>>
>> I also think it should be done ultimately. And I think it's very much
>> hard since it requires to change the dependency graph of lockdep but
>> anyway possible. It's up to lockdep maintainer's will though..
> 
> We used to do this in XFS to work around the fact that the memory
> reclaim context "locks" were too stupid to understand that an object
> referenced and locked above memory allocation could not be
> accessed below in memory reclaim because memory reclaim only accesses
> /unreferenced objects/. We played whack-a-mole with lockdep for
> years to get most of the false positives sorted out.
> 
> Hence for a long time we had to re-initialise the lock context for
> the XFS inode iolock in ->evict_inode() so we could lock it for
> reclaim processing.  Eventually we ended up completely reworking the
> inode reclaim locking in XFS primarily to get rid of all the nasty
> lockdep hacks we had strewn throughout the code. It was ~2012 we
> got rid of the last inode re-init code, IIRC. Yeah:
> 
> commit 4f59af758f9092bc7b266ca919ce6067170e5172
> Author: Christoph Hellwig <hch@infradead.org>
> Date:   Wed Jul 4 11:13:33 2012 -0400
> 
>      xfs: remove iolock lock classes
>      
>      Now that we never take the iolock during inode reclaim we don't need
>      to play games with lock classes.
>      
>      Signed-off-by: Christoph Hellwig <hch@lst.de>
>      Reviewed-by: Rich Johnston <rjohnston@sgi.com>
>      Signed-off-by: Ben Myers <bpm@sgi.com>
> 
> We still have problems with lockdep false positives w.r.t. memory
> allocation contexts, mainly with code that can be called from
> both above and below memory allocation contexts. We've finally
> got __GFP_NOLOCKDEP to be able to annotate memory allocation points
> within such code paths, but that doesn't help with locks....
> 
> Byungchul, lockdep has a long, long history of having sharp edges
> and being very unfriendly to developers. We've all been scarred by
> lockdep at one time or another and so there's a fair bit of
> resistance to repeating past mistakes and allowing lockdep to
> inflict more scars on us....

As I understand what you suffered from.. I don't really want to
force it forward strongly.

So far, all problems have been handled by myself including the
final one e.i. the completion in submit_bio_wait() with the
invalidation if it's allowed. But yes, who knows the future? In
the future, that terrible thing you mentioned might or might
not happen because of cross-release.

I just felt like someone was misunderstanding what the problem
came from, what the problem was, how we could avoid it, why
cross-release should be removed and so on..

I believe the 3 ways I suggested can help, but I don't want to
strongly insist if all of you don't think so.

Thanks a lot anyway for your opinion.

-- 
Thanks,
Byungchul

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[J. Bruce Fields]

On Mon, Jan 01, 2018 at 02:18:55AM -0800, Matthew Wilcox wrote:
> On Sat, Dec 30, 2017 at 06:00:57PM -0500, Theodore Ts'o wrote:
> > On Sat, Dec 30, 2017 at 05:40:28PM -0500, Theodore Ts'o wrote:
> > > On Sat, Dec 30, 2017 at 12:44:17PM -0800, Matthew Wilcox wrote:
> > > > 
> > > > I'm not sure I agree with this part.  What if we add a new TCP lock class
> > > > for connections which are used for filesystems/network block devices/...?
> > > > Yes, it'll be up to each user to set the lockdep classification correctly,
> > > > but that's a relatively small number of places to add annotations,
> > > > and I don't see why it wouldn't work.
> > > 
> > > I was exagerrating a bit for effect, I admit.  (but only a bit).
> 
> I feel like there's been rather too much of that recently.  Can we stick
> to facts as far as possible, please?
> 
> > > It can probably be for all TCP connections that are used by kernel
> > > code (as opposed to userspace-only TCP connections).  But it would
> > > probably have to be each and every device-mapper instance, each and
> > > every block device, each and every mounted file system, each and every
> > > bdi object, etc.
> > 
> > Clarification: all TCP connections that are used by kernel code would
> > need to be in their own separate lock class.  All TCP connections used
> > only by userspace could be in their own shared lock class.  You can't
> > use a one lock class for all kernel-used TCP connections, because of
> > the Network Block Device mounted on a local file system which is then
> > exported via NFS and squirted out yet another TCP connection problem.
> 
> So the false positive you're concerned about is write-comes-in-over-NFS
> (with socket lock held), NFS sends a write request to local filesystem,

I'm confused, what lock does Ted think the NFS server is holding over
NFS processing?

--b.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[J. Bruce Fields]

On Fri, Jan 05, 2018 at 11:49:41AM -0500, bfields wrote:
> On Mon, Jan 01, 2018 at 02:18:55AM -0800, Matthew Wilcox wrote:
> > On Sat, Dec 30, 2017 at 06:00:57PM -0500, Theodore Ts'o wrote:
> > > On Sat, Dec 30, 2017 at 05:40:28PM -0500, Theodore Ts'o wrote:
> > > > On Sat, Dec 30, 2017 at 12:44:17PM -0800, Matthew Wilcox wrote:
> > > > > 
> > > > > I'm not sure I agree with this part.  What if we add a new TCP lock class
> > > > > for connections which are used for filesystems/network block devices/...?
> > > > > Yes, it'll be up to each user to set the lockdep classification correctly,
> > > > > but that's a relatively small number of places to add annotations,
> > > > > and I don't see why it wouldn't work.
> > > > 
> > > > I was exagerrating a bit for effect, I admit.  (but only a bit).
> > 
> > I feel like there's been rather too much of that recently.  Can we stick
> > to facts as far as possible, please?
> > 
> > > > It can probably be for all TCP connections that are used by kernel
> > > > code (as opposed to userspace-only TCP connections).  But it would
> > > > probably have to be each and every device-mapper instance, each and
> > > > every block device, each and every mounted file system, each and every
> > > > bdi object, etc.
> > > 
> > > Clarification: all TCP connections that are used by kernel code would
> > > need to be in their own separate lock class.  All TCP connections used
> > > only by userspace could be in their own shared lock class.  You can't
> > > use a one lock class for all kernel-used TCP connections, because of
> > > the Network Block Device mounted on a local file system which is then
> > > exported via NFS and squirted out yet another TCP connection problem.
> > 
> > So the false positive you're concerned about is write-comes-in-over-NFS
> > (with socket lock held), NFS sends a write request to local filesystem,
> 
> I'm confused, what lock does Ted think the NFS server is holding over
> NFS processing?

Sorry, I meant "over RPC processing".

I'll confess to no understanding of socket locking.  The server RPC code
doesn't take any itself except in a couple places on setup and tear
down of a connection.  We wouldn't actually want any exclusive
per-connection lock held across RPC processing because we want to be
able to handle multiple concurrent RPCs per connection.

We do need a little locking just to make sure multiple server threads
replying to the same client don't accidentally corrupt their replies by
interleaving.  But even there we're using our own lock, held only while
transmitting the reply (after all the work's done and reply encoded).

--b.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Byungchul Park]

On 12/31/2017 7:40 AM, Theodore Ts'o wrote:
> On Sat, Dec 30, 2017 at 12:44:17PM -0800, Matthew Wilcox wrote:
>>
>> I'm not sure I agree with this part.  What if we add a new TCP lock class
>> for connections which are used for filesystems/network block devices/...?
>> Yes, it'll be up to each user to set the lockdep classification correctly,
>> but that's a relatively small number of places to add annotations,
>> and I don't see why it wouldn't work.
> 
> I was exagerrating a bit for effect, I admit.  (but only a bit).
> 
> It can probably be for all TCP connections that are used by kernel
> code (as opposed to userspace-only TCP connections).  But it would
> probably have to be each and every device-mapper instance, each and
> every block device, each and every mounted file system, each and every
> bdi object, etc.
> 
> The point I was trying to drive home is that "all we have to do is
> just classify everything well or just invalidate the right lock

Just to be sure, we don't have to invalidate lock objects at all but
a problematic waiter only.

> objects" is a massive understatement of the complexity level of what
> would be required, or the number of locks/completion handlers that
> would have to be blacklisted.
> 
> 						- Ted
> 

-- 
Thanks,
Byungchul

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Theodore Ts'o]

On Wed, Jan 03, 2018 at 11:10:37AM +0900, Byungchul Park wrote:
> > The point I was trying to drive home is that "all we have to do is
> > just classify everything well or just invalidate the right lock
> 
> Just to be sure, we don't have to invalidate lock objects at all but
> a problematic waiter only.

So essentially you are proposing that we have to play "whack-a-mole"
as we find false positives, and where we may have to put in ad-hoc
plumbing to only invalidate "a problematic waiter" when it's
problematic --- or to entirely suppress the problematic waiter
altogether.  And in that case, a file system developer might be forced
to invalidate a lock/"waiter"/"completion" in another subsystem.

I will also remind you that doing this will trigger a checkpatch.pl
*error*:

ERROR("LOCKDEP", "lockdep_no_validate class is reserved for device->mutex.\n" . $herecurr);

	 		      	       		- Ted

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Byungchul Park]

On 1/3/2018 4:05 PM, Theodore Ts'o wrote:
> On Wed, Jan 03, 2018 at 11:10:37AM +0900, Byungchul Park wrote:
>>> The point I was trying to drive home is that "all we have to do is
>>> just classify everything well or just invalidate the right lock
>>
>> Just to be sure, we don't have to invalidate lock objects at all but
>> a problematic waiter only.
> 
> So essentially you are proposing that we have to play "whack-a-mole"
> as we find false positives, and where we may have to put in ad-hoc
> plumbing to only invalidate "a problematic waiter" when it's
> problematic --- or to entirely suppress the problematic waiter

If we have too many problematic completions(waiters) to handle it,
then I agree with you. But so far, only one exits and it seems able
to be handled even in the future on my own.

Or if you believe that we have a lot of those kind of completions
making trouble so we cannot handle it, the (4) by Amir would work,
no? I'm asking because I'm really curious about your opinion..

> altogether.  And in that case, a file system developer might be forced
> to invalidate a lock/"waiter"/"completion" in another subsystem.

As I said, with regard to the invalidation, we don't have to
consider locks at all. It's enough to invalidate the waiter only.

> I will also remind you that doing this will trigger a checkpatch.pl
> *error*:

This is what we decided. And I think the decision is reasonable for
original lockdep. But I wonder if we should apply the same decision
on waiters. I don't insist but just wonder.

> ERROR("LOCKDEP", "lockdep_no_validate class is reserved for device->mutex.\n" . $herecurr);
> 
> 	 		      	       		- Ted
> 

-- 
Thanks,
Byungchul

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Byungchul Park]

On 1/3/2018 5:10 PM, Byungchul Park wrote:
> On 1/3/2018 4:05 PM, Theodore Ts'o wrote:
>> On Wed, Jan 03, 2018 at 11:10:37AM +0900, Byungchul Park wrote:
>>>> The point I was trying to drive home is that "all we have to do is
>>>> just classify everything well or just invalidate the right lock
>>>
>>> Just to be sure, we don't have to invalidate lock objects at all but
>>> a problematic waiter only.
>>
>> So essentially you are proposing that we have to play "whack-a-mole"
>> as we find false positives, and where we may have to put in ad-hoc
>> plumbing to only invalidate "a problematic waiter" when it's
>> problematic --- or to entirely suppress the problematic waiter
> 
> If we have too many problematic completions(waiters) to handle it,
> then I agree with you. But so far, only one exits and it seems able
> to be handled even in the future on my own.
> 
> Or if you believe that we have a lot of those kind of completions
> making trouble so we cannot handle it, the (4) by Amir would work,
> no? I'm asking because I'm really curious about your opinion..
> 
>> altogether.A  And in that case, a file system developer might be forced
>> to invalidate a lock/"waiter"/"completion" in another subsystem.
> 
> As I said, with regard to the invalidation, we don't have to
> consider locks at all. It's enough to invalidate the waiter only.
> 
>> I will also remind you that doing this will trigger a checkpatch.pl
>> *error*:
> 
> This is what we decided. And I think the decision is reasonable for
> original lockdep. But I wonder if we should apply the same decision
> on waiters. I don't insist but just wonder.

What if we adopt the (4) in which waiters are validated one by one
and no explicit invalidation is involved?

>> ERROR("LOCKDEP", "lockdep_no_validate class is reserved for 
>> device->mutex.\n" . $herecurr);
>>
>> A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A  - Ted
>>
> 

-- 
Thanks,
Byungchul

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Byungchul Park]

On 12/31/2017 12:40 AM, Theodore Ts'o wrote:
> On Fri, Dec 29, 2017 at 10:16:24PM -0800, Matthew Wilcox wrote:
>>
>> I disagree here.  As Ted says, it's the interactions between the
>> subsystems that leads to problems.  Everything's goig to work great
>> until somebody does something in a way that's never been tried before.
> 
> The question what is classified *well* mean?  At the extreme, we could
> put the locks for every single TCP connection into their own lockdep
> class.  But that would blow the limits in terms of the number of locks
> out of the water super-quickly --- and it would destroy the ability
> for lockdep to learn what the proper locking order should be.  Yet
> given Lockdep's current implementation, the only way to guarantee that
> there won't be any interactions between subsystems that cause false
> positives would be to categorizes locks for each TCP connection into
> their own class.
> 
> So this is why I get a little annoyed when you say, "it's just a
> matter of classification".  NO IT IS NOT.  We can not possibly
> classify things "correctly" to completely limit false positives
> without completely destroying lockdep's scalability as it is currently

You seem to admit that it can be solved by proper classification but
say that it's *not realistic* because of the limitation of lockdep.

Right?

I've agreed with you for that point. I also think it's very hard to
do it because of the lockdep design and the only way might be to fix
lockdep fundamentally, that may be the one we should do ultimately.

Is it the best decision to keep it removed until lockdep get fixed
fundamentally? If I believe it were, I would have kept quiet. But, I
don't think so. Almost other users had already gotten benifit from
it except the special case.

And it would be appriciated if you remind that I suggested 3 methods
+ 1 (by Amir) before for that reason.

I don't want to force it forward but just want the facts to be shared.
I felt like I failed it because of the lack of explanation.

> As far as the "just invalidate the waiter", the problem is that it
> requires source level changes to invalidate the waiter, and for

Or, no change is needed if we adopt the (4)th option (by Amir), in
which we keep waiters invalidated by default and validate waiters
explicitly only when it needs.

> different use cases, we will need to validate different waiters.  For
> example, in the example I gave, we would have to invalidate *all* TCP
> waiters/locks in order to prevent false positives.  But that makes the

No. Only invalidating waiters is enough. For now, the waiter in
submit_bio_wait() is the only one to invalidate.

> lockdep useless for all TCP locks.  What's the solution?  I claim that

Even if we invalidate waiters, TCP locks can still work with lockdep.
Invalidating waiters *never* affect lockdep checking for typical locks
at all.

> The only way it can work is to either dump it on the reposibility of
> the people debugging lockdep reports to make source level changes to
> other subsystems which they aren't the maintainers of to suppress
> false positives that arise due to how the subsystems are being used
> together in their particular configuration ---- or you can try to

You seem to misunderstand it a lot.. The only thing we have to is to
use init_completion_nomap() instead of init_completion() for the
problematic completion object. So far, the completion in
submit_bio_wait() has been the only one.

If you belive that we have a lot of problematic completions(waiters)
so that we cannot handle it, the (4) by Amir can be an option.

Just to be sure, there were several false positives by cross-release.
Something was due to confliction between manual acquire()s added
before and automatic cross-release, both of which are for detecting
deadlocks by a specific completion(waiter). Or, something was solved
by classifying locks properly simply. And this case of
submit_bio_wait() is the first case that we cannot classify locks
simply and need to consider other options.

-- 
Thanks,
Byungchul

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Byungchul Park]

On 12/30/2017 3:16 PM, Matthew Wilcox wrote:
> On Fri, Dec 29, 2017 at 04:28:51PM +0900, Byungchul Park wrote:
>> On Thu, Dec 28, 2017 at 10:51:46PM -0500, Theodore Ts'o wrote:
>>> On Fri, Dec 29, 2017 at 10:47:36AM +0900, Byungchul Park wrote:
>>>>
>>>>     (1) The best way: To classify all waiters correctly.
>>>
>>> It's really not all waiters, but all *locks*, no?
>>
>> Thanks for your opinion. I will add my opinion on you.
>>
>> I meant *waiters*. Locks are only a sub set of potential waiters, which
>> actually cause deadlocks. Cross-release was designed to consider the
>> super set including all general waiters such as typical locks,
>> wait_for_completion(), and lock_page() and so on..
> 
> I think this is a terminology problem.  To me (and, I suspect Ted), a
> waiter is a subject of a verb while a lock is an object.  So Ted is asking
> whether we have to classify the users, while I think you're saying we
> have extra objects to classify.
> 
> I'd be comfortable continuing to refer to completions as locks.  We could
> try to come up with a new object name like waitpoints though?

Right. Then "event" should be used as an object name than "waiter".

>> The problems come from wrong classification. Waiters either classfied
>> well or invalidated properly won't bitrot.
> 
> I disagree here.  As Ted says, it's the interactions between the

As you know, the classification is something already considering
the interactions between the subsystems and classified. But, yes.
That is just what we have to do untimately but not what we can do
right away. That's why I suggested all 3 ways + 1 way (by Amir).

> subsystems that leads to problems.  Everything's goig to work great
> until somebody does something in a way that's never been tried before.

Yes. Everything has worked great so far, since the classification
by now is done well enough at least to avoid such problems, not
perfect though. IMO, the classification does not have to be perfect
but needs to be good enough to work.

--
Thanks,
Byungchul

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Amir Goldstein]

On Fri, Dec 29, 2017 at 3:47 AM, Byungchul Park <byungchul.park@lge.com> wrote:
> On Wed, Dec 13, 2017 at 03:24:29PM +0900, Byungchul Park wrote:
>> Lockdep works, based on the following:
>>
>>    (1) Classifying locks properly
>>    (2) Checking relationship between the classes
>>
>> If (1) is not good or (2) is not good, then we
>> might get false positives.
>>
>> For (1), we don't have to classify locks 100%
>> properly but need as enough as lockdep works.
>>
>> For (2), we should have a mechanism w/o
>> logical defects.
>>
>> Cross-release added an additional capacity to
>> (2) and requires (1) to get more precisely classified.
>>
>> Since the current classification level is too low for
>> cross-release to work, false positives are being
>> reported frequently with enabling cross-release.
>> Yes. It's a obvious problem. It needs to be off by
>> default until the classification is done by the level
>> that cross-release requires.
>>
>> But, the logic (2) is valid and logically true. Please
>> keep the code, mechanism, and logic.
>
> I admit the cross-release feature had introduced several false positives
> about 4 times(?), maybe. And I suggested roughly 3 ways to solve it. I
> should have explained each in more detail. The lack might have led some
> to misunderstand.
>
>    (1) The best way: To classify all waiters correctly.
>
>       Ultimately the problems should be solved in this way. But it
>       takes a lot of time so it's not easy to use the way right away.
>       And I need helps from experts of other sub-systems.
>
>       While talking about this way, I made a trouble.. I still believe
>       that each sub-system expert knows how to solve dependency problems
>       most, since each has own dependency rule, but it was not about
>       responsibility. I've never wanted to charge someone else it but me.
>
>    (2) The 2nd way: To make cross-release off by default.
>
>       At the beginning, I proposed cross-release being off by default.
>       Honestly, I was happy and did it when Ingo suggested it on by
>       default once lockdep on. But I shouldn't have done that but kept
>       it off by default. Cross-release can make some happy but some
>       unhappy until problems go away through (1) or (2).
>
>    (3) The 3rd way: To invalidate waiters making trouble.
>
>       Of course, this is not the best. Now that you have already spent
>       a lot of time to fix original lockdep's problems since lockdep was
>       introduced in 2006, we don't need to use this way for typical
>       locks except a few special cases. Lockdep is fairly robust by now.
>
>       And I understand you don't want to spend more time to fix
>       additional problems again. Now that the situation is different
>       from the time, 2006, it's not too bad to use this way to handle
>       the issues.
>

Purely logically, aren't you missing a 4th option:

    (4) The 4th way: To validate specific waiters.

Is it not an option for a subsystem to opt-in for cross-release validation
of specific locks/waiters? This may be a much preferred route for cross-
release. I remember seeing a post from a graphic driver developer that
found cross-release useful for finding bugs in his code.

For example, many waiters in kernel can be waiting for userspace code,
so does that mean the cross-release is going to free the world from
userspace deadlocks as well?? Possibly I am missing something.

In any way, it seem logical to me that some waiters should particpate
in lock chain dependencies, while other waiters should break the chain
to avoid false positives and to avoid protecting against user configurable
deadlocks (like loop mount over file inside the loop mounted fs).
And if you agree that this logic claim is correct, than surely, an inclusive
approach is the best way forward.

Cheers,
Amir.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: About the try to remove cross-release feature entirely by Ingo

[Byungchul Park]

On 12/29/2017 5:09 PM, Amir Goldstein wrote:
> On Fri, Dec 29, 2017 at 3:47 AM, Byungchul Park <byungchul.park@lge.com> wrote:
>> On Wed, Dec 13, 2017 at 03:24:29PM +0900, Byungchul Park wrote:
>>> Lockdep works, based on the following:
>>>
>>>     (1) Classifying locks properly
>>>     (2) Checking relationship between the classes
>>>
>>> If (1) is not good or (2) is not good, then we
>>> might get false positives.
>>>
>>> For (1), we don't have to classify locks 100%
>>> properly but need as enough as lockdep works.
>>>
>>> For (2), we should have a mechanism w/o
>>> logical defects.
>>>
>>> Cross-release added an additional capacity to
>>> (2) and requires (1) to get more precisely classified.
>>>
>>> Since the current classification level is too low for
>>> cross-release to work, false positives are being
>>> reported frequently with enabling cross-release.
>>> Yes. It's a obvious problem. It needs to be off by
>>> default until the classification is done by the level
>>> that cross-release requires.
>>>
>>> But, the logic (2) is valid and logically true. Please
>>> keep the code, mechanism, and logic.
>>
>> I admit the cross-release feature had introduced several false positives
>> about 4 times(?), maybe. And I suggested roughly 3 ways to solve it. I
>> should have explained each in more detail. The lack might have led some
>> to misunderstand.
>>
>>     (1) The best way: To classify all waiters correctly.
>>
>>        Ultimately the problems should be solved in this way. But it
>>        takes a lot of time so it's not easy to use the way right away.
>>        And I need helps from experts of other sub-systems.
>>
>>        While talking about this way, I made a trouble.. I still believe
>>        that each sub-system expert knows how to solve dependency problems
>>        most, since each has own dependency rule, but it was not about
>>        responsibility. I've never wanted to charge someone else it but me.
>>
>>     (2) The 2nd way: To make cross-release off by default.
>>
>>        At the beginning, I proposed cross-release being off by default.
>>        Honestly, I was happy and did it when Ingo suggested it on by
>>        default once lockdep on. But I shouldn't have done that but kept
>>        it off by default. Cross-release can make some happy but some
>>        unhappy until problems go away through (1) or (2).
>>
>>     (3) The 3rd way: To invalidate waiters making trouble.
>>
>>        Of course, this is not the best. Now that you have already spent
>>        a lot of time to fix original lockdep's problems since lockdep was
>>        introduced in 2006, we don't need to use this way for typical
>>        locks except a few special cases. Lockdep is fairly robust by now.
>>
>>        And I understand you don't want to spend more time to fix
>>        additional problems again. Now that the situation is different
>>        from the time, 2006, it's not too bad to use this way to handle
>>        the issues.
>>
> 
> Purely logically, aren't you missing a 4th option:
> 
>      (4) The 4th way: To validate specific waiters.
> 

Hello,

Thanks for your opinion. I will add my opinion on you.

> Is it not an option for a subsystem to opt-in for cross-release validation
> of specific locks/waiters? This may be a much preferred route for cross-

Yes. I think it can be a good option.

I think we have to choose a better one between (3) and (4) depending
on the following:

    In case that there are few waiters making trouble, it would be
    better to choose (3).

    In case that there are a lot of waiter making trouble, it would be
    better to chosse (4).

I think (3) is better for now because there's only one or two cases
making us hard to handle it. However, if you don't agree, I also
think (4) can be an available option.

> release. I remember seeing a post from a graphic driver developer that
> found cross-release useful for finding bugs in his code.
> 
> For example, many waiters in kernel can be waiting for userspace code,
> so does that mean the cross-release is going to free the world from
> userspace deadlocks as well?? Possibly I am missing something.

I don't see what you are saying exactly.. but cross-release can be
used if we know (a) the spot waiting for an event and (3) the other
spot triggering the event. Please explain it more if I miss something.

> In any way, it seem logical to me that some waiters should particpate
> in lock chain dependencies, while other waiters should break the chain
> to avoid false positives and to avoid protecting against user configurable
> deadlocks (like loop mount over file inside the loop mounted fs).

For example, when we had cross-release enabled, the following chain
was built and false positives were produced:

    link 1: ext4 spin lock class A (in a lower fs) ->
            waiter class B (in submit_bio_wait())

    link 2: waiter class B (in submit_bio_wait()) ->
            ext4 spin lock class A (in an upper fs)

Even though conceptually it should have been "class A in lower fs
!= class A in upper fs", current code registers these two as class A.

So we need to correct the chain like, using (1):

    link 1: ext4 spin lock class A (in a lower fs) ->
            waiter class B (in submit_bio_wait())

    link 2: waiter class B (in submit_bio_wait()) ->
            ext4 spin lock class *C* (in an upper fs)

Or using (3) or (4):

    no link (because waiter class B does not exist anymore)

> And if you agree that this logic claim is correct, than surely, an inclusive
> approach is the best way forward.

I'm also curious about other opinions..

> Cheers,
2> Amir.
> 

-- 
Thanks,
Byungchul

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>