Re: Re: [PATCH] [PATCH] mm: disable preemption before swapcache_free

[Hugh Dickins <hughd@google.com>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1700 bytes --]

On Tue, 7 Aug 2018, zhaowuyun@wingtech.com 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