Re: [PATCH] lockdep: lock_set_subclass - reset a held lock's subclass

[Jeremy Fitzhardinge <jeremy@goop.org>]

Linus Torvalds wrote:
> Hmm. 
>
> With hashed locks, that is _not_ safe in general. Now, it may well be safe 
> in your case, so I'm not going to say you have a bug, but even if you do 
> them in vaddr order, the thing is, we don't guarantee that the _locks_ are 
> ordered in virtual address order.
>   

Yes, it's current simplicity definitely relies on a simple relationship 
between pte pages and locks.

> Right now, I think the pte locks are either a single one per mm (in which 
> case I assume you don't take any lock at all), or it's a lock that is 
> embedded in the pmd page iirc.
>   

Actually the locks in the pte page, so it's already fairly 
fine-grained.  If it were made coarser - at the pmd level - then I'd 
simply move my lock-taking out a level in the pagetable traversal.

But making it finer - by hashing individual pte entries to their own 
locks - would work badly for me.  I'm taking the lock specifically to 
protect against updates to any pte within the pte page, so I'd have to 
manually take all the locks that the ptes could possibly hash to.  
Presumably in that eventuality we could define correct order for taking 
the hashed pte locks, independent of how the ptes actually map to them 
(for example, always take locks in low->high order of *lock* address).

> What if you have pmd sharing through some shared area being mapped at two 
> different processes at different addresses? Yeah, I don't think we share 
> pmd's at all (except if you use hugetables and for the kernel), but it's 
> one of those things where subtle changes in how the pte lock allocation 
> could cause problems.
>   

In this particular case it isn't an issue.  The traversal is performing 
first/last use init/deinit, so any shared parts of the pagetable can be 
skipped with no action because they're already done.  Presumably there'd 
be separate lifetime management for whatever parts of the pagetable can 
be shared, so I'd need to hook in there, rather than the wholesale 
pagetable create/destroy as I do now.

> Eg, I could easily see somebody doing the pte lock as a hash over not just 
> the address, but the "struct mm" pointer too. At which point different 
> parts of the address space might even share the PTE lock, and you'd get 
> deadlocks even without any ABBA behavior, just because the pte lock might 
> be A B C A or something inside the same process.
>   

Yep.  That would be awkward.  A function which says "here's a pte page, 
return the set of all pte locks these ptes map to in correct locking 
order" would be useful in that case.  Ugh, but still unpleasant.

    J