Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 14 Feb 2019 16:22:36 -0500
From:   Andrea Arcangeli <aarcange@redhat.com>
To:     Andrew Morton <akpm@linux-foundation.org>
Cc:     Michal Hocko <mhocko@kernel.org>,
        "Huang, Ying" <ying.huang@intel.com>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, Hugh Dickins <hughd@google.com>,
        "Paul E . McKenney" <paulmck@linux.vnet.ibm.com>,
        Minchan Kim <minchan@kernel.org>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Tim Chen <tim.c.chen@linux.intel.com>,
        Mel Gorman <mgorman@techsingularity.net>,
        =?iso-8859-1?B?Suly9G1l?= Glisse <jglisse@redhat.com>,
        David Rientjes <rientjes@google.com>,
        Rik van Riel <riel@redhat.com>, Jan Kara <jack@suse.cz>,
        Dave Jiang <dave.jiang@intel.com>,
        Daniel Jordan <daniel.m.jordan@oracle.com>,
        Andrea Parri <andrea.parri@amarulasolutions.com>
Subject: Re: [PATCH -mm -V7] mm, swap: fix race between swapoff and some swap
 operations
Message-ID: <20190214212236.GA10698@redhat.com>
References: <20190211083846.18888-1-ying.huang@intel.com>
 <20190214143318.GJ4525@dhcp22.suse.cz>
 <20190214123002.b921b680fea07bf5f798df79@linux-foundation.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190214123002.b921b680fea07bf5f798df79@linux-foundation.org>
User-Agent: Mutt/1.11.3 (2019-02-01)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hello,

On Thu, Feb 14, 2019 at 12:30:02PM -0800, Andrew Morton wrote:
> This was discussed to death and I think the changelog explains the
> conclusions adequately.  swapoff is super-rare so a stop_machine() in
> that path is appropriate if its use permits more efficiency in the
> regular swap code paths.  

The problem is precisely that the way the stop_machine callback is
implemented right now (a dummy noop), makes the stop_machine()
solution fully equivalent to RCU from the fast path point of view. It
does not permit more efficiency in the fast path which is all we care
about.

For the slow path point of view the only difference is possibly that
stop_machine will reach the quiescent state faster (i.e. swapoff may
return a few dozen milliseconds faster), but nobody cares about the
latency of swapoff and it's actually nicer if swapoff doesn't stop all
CPUs on large systems and it uses less CPU overall.

This is why I suggested if we keep using stop_machine() we should not
use a dummy function whose only purpose is to reach a queiscent state
(which is something that is more efficiently achieved with the
syncronize_rcu/sched/kernel RCU API of the day) but we should instead
try to leverage the UP-like serialization to remove more spinlocks
from the fast path and convert them to preempt_disable(). However the
current dummy callback cannot achieve that higher efficiency in the
fast paths, the code would need to be reshuffled to try to remove at
least the swap_lock.

If no spinlock is converted to preempt_disable() RCU I don't see the
point of stop_machine().

On a side note, the cmpxchge machinery I posted to run the function
simultaneously on all CPUs I think may actually be superflous if using
cpus=NULL like Hing suggested. Implementation details aside, still the
idea of stop_machine would be to do those p->swap_map = NULL and
everything protected by the swap_lock, should be executed inside the
callback that runs like in a UP system to speedup the fast path
further.

Thanks,
Andrea