On Tue, 7 Aug 2018, zhaowuyun@xxxxxxxxxxxx wrote: > > Thanks for affirming the modification of disabling preemption and > pointing out the incompleteness, delete_from_swap_cache() needs the same protection. > I'm curious about that why don't put swapcache_free(swap) under protection of mapping->tree_lock ?? That would violate the long-established lock ordering (see not-always- kept-up-to-date comments at the head of mm/rmap.c). In particular, swap_lock (and its more recent descendants, such as swap_info->lock) can be held with interrupts enabled, whereas taking tree_lock (later called i_pages lock) involves disabling interrupts. So: there would be quite a lot of modifications required to do swapcache_free(swap) under mapping->tree_lock. Generally easier would be to take tree_lock under swap lock: that fits the establishd lock ordering, and is already done in just a few places - or am I thinking of free_swap_and_cache() in the old days before find_get_page() did lockless lookup? But you didn't suggest that way, because it's more awkward in the __remove_mapping() case: I expect that could be worked around with an initial PageSwapCache check, taking swap locks there first (not inside swapcache_free()) - __remove_mapping()'s BUG_ON(!PageLocked) implies that won't be racy. But either way round, why? What would be the advantage in doing so? A more conventional nesting of locks, easier to describe and understand, yes. But from a performance point of view, thinking of lock contention, nothing but disadvantage. And don't forget the get_swap_page() end: there it would be harder to deal with both locks together (at least in the shmem case). Hugh
