Re: [PATCH] swap_state.c thinko

Hugh Dickins
2001-04-05T17:21:10Z

On Thu, 5 Apr 2001, Ben LaHaise wrote:
>
> Here's another one liner that closes an smp race that could corrupt
> things.
>
> diff -urN v2.4.3/mm/swap_state.c work-2.4.3/mm/swap_state.c
> --- v2.4.3/mm/swap_state.c Fri Dec 29 18:04:27 2000
> +++ work-2.4.3/mm/swap_state.c Thu Apr 5 11:55:00 2001
> @@ -140,7 +140,7 @@
> /*
> * If we are the only user, then try to free up the swap cache.
> */
> - if (PageSwapCache(page) && !TryLockPage(page)) {
> + if (!TryLockPage(page) && PageSwapCache(page)) {
> if (!is_page_shared(page)) {
> delete_from_swap_cache_nolock(page);
> }

I agree that PageSwapCache(page) needs to be retested when(if) the
page lock is acquired, but isn't it best to check PageSwapCache(page)
first as at present - won't it very often fail? won't the overhead of
TryLocking and Unlocking every page slow down a hot path?

And isn't this free_page_and_swap_cache(), precisely the area that's
currently subject to debate and patches, because swap pages are not
getting freed soon enough? I haven't been following that discussion
with full understanding, and haven't seen a full explanation of the
problem to be solved; but I'd rather _imagined_ it was that the page
would here be on an LRU list, raising its count and causing the
is_page_shared(page) test to succeed despite not really shared.
So I'd been expecting a patch to remove this code completely.

Forgive me if way off base...
Hugh