lkml.org 
[lkml]   [2017]   [Dec]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: About the try to remove cross-release feature entirely by Ingo
From
Date
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

\
 
 \ /
  Last update: 2017-12-29 10:47    [W:0.096 / U:9.244 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site