Messages in this thread | | | Date | Thu, 16 Nov 2017 13:02:16 +0100 | From | Michal Hocko <> | Subject | Re: [PATCH 1/3] lockdep: Apply crossrelease to PG_locked locks |
| |
[I have only briefly looked at patches so I might have missed some details.]
On Thu 16-11-17 12:14:25, Byungchul Park wrote: > Although lock_page() and its family can cause deadlock, lockdep have not > worked with them, because unlock_page() might be called in a different > context from the acquire context, which violated lockdep's assumption. > > Now CONFIG_LOCKDEP_CROSSRELEASE has been introduced, lockdep can work > with page locks.
I definitely agree that debugging page_lock deadlocks is a major PITA but your implementation seems prohibitively too expensive.
[...] > @@ -218,6 +222,10 @@ struct page { > #ifdef LAST_CPUPID_NOT_IN_PAGE_FLAGS > int _last_cpupid; > #endif > + > +#ifdef CONFIG_LOCKDEP_PAGELOCK > + struct lockdep_map_cross map; > +#endif > }
now you are adding struct lockdep_map_cross { struct lockdep_map map; /* 0 40 */ struct cross_lock xlock; /* 40 56 */ /* --- cacheline 1 boundary (64 bytes) was 32 bytes ago --- */
/* size: 96, cachelines: 2, members: 2 */ /* last cacheline: 32 bytes */ };
for each struct page. So you are doubling the size. Who is going to enable this config option? You are moving this to page_ext in a later patch which is a good step but it doesn't go far enough because this still consumes those resources. Is there any problem to make this kernel command line controllable? Something we do for page_owner for example?
Also it would be really great if you could give us some measures about the runtime overhead. I do not expect it to be very large but this is something people are usually interested in when enabling debugging features. -- Michal Hocko SUSE Labs
| |