From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id CB845C77B75 for ; Wed, 3 May 2023 19:57:21 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 63214900003; Wed, 3 May 2023 15:57:21 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 5E236900002; Wed, 3 May 2023 15:57:21 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4D160900003; Wed, 3 May 2023 15:57:21 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from mail-yb1-f179.google.com (mail-yb1-f179.google.com [209.85.219.179]) by kanga.kvack.org (Postfix) with ESMTP id 2346B900002 for ; Wed, 3 May 2023 15:57:21 -0400 (EDT) Received: by mail-yb1-f179.google.com with SMTP id 3f1490d57ef6-b9d881ad689so8091813276.2 for ; Wed, 03 May 2023 12:57:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1683143840; x=1685735840; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=1Ox1/516ZvGWiy6XQzvdYM7/gH/XZHZIVHoP8inhzxM=; b=tRO5/vsXmgz8bVUiKFCXTV8aovuqnpc72ZftAjAmKaHKUf97DDZm/88QvfymfWreye 9t68UwHLaPnIKrub9j6Uh3xDEzv32OfM52MVZU5n2roAEMLmNGL5OgnkuwpX473CFnhE Q224qoZ9m0ei5qlLmUFmbz4acl3XMEWtwAyAq6pPi1lB4p9Mrd7Baw78EtRNJdYVkcvG D5WtsIugC6jkpwf9NZ1EZKUfSJyaLOnAJx5TgAasi0iBHbse8rj94z/kDqfv+sqddCPJ AWbxexAHZRDHXnRqbhJpejXPxipykhlLNVvttdrlypKH19n9HWGhOj23xTTvOI2T4CP9 L2zg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1683143840; x=1685735840; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=1Ox1/516ZvGWiy6XQzvdYM7/gH/XZHZIVHoP8inhzxM=; b=IyzfRCUJk1vEMN3BTUJAKDeW/X4ovz4MQV16n0lujYq4S3xWt3mNvKmt7xu2mjCSBG oC03SR5maYqmo8Ni1OsqrE6wH6PA/evYPW1yXZ3IU0atcNQJrDq1SCaS8ecP7Gt/rIbL Qxln/Hdoz2ZQLxfUIROj5m1sQOpZf2VGpsD9vAqlSP19WB6DBLl8web9bBcbMx+dv0rn Ocn9LcXmWJigMxtT7aAdzcWUy/BAMNgzKkO+yVjEyFiGg2XO0BYPSJ6m3aVLao583+Qn MD9CDmZVeWE5QkHQX23f4bPi59EOe4rQWnddR90S6wG3dQTYMqkG7Aq47FkyqD1ikdk4 2Xfw== X-Gm-Message-State: AC+VfDybvhCgeeNmAu0tuoagbgTFlAHV7NghFr/KZTvHfaInwfWGfnN1 vbHZCXSc3vqtf7JwEhxJMwBk9rHc6IbQZSxfHm4qgA== X-Google-Smtp-Source: ACHHUZ7ABw610g4pAAup3sgwroFeEko5dVILAFx3nQjtdd9Aki/WRsMXwrfgPFiTJCmml2TfA6W46q0XZ5s6cGLvQ/g= X-Received: by 2002:a25:f46:0:b0:b9d:f4df:b0ef with SMTP id 67-20020a250f46000000b00b9df4dfb0efmr13111149ybp.42.1683143840443; Wed, 03 May 2023 12:57:20 -0700 (PDT) MIME-Version: 1.0 References: <20230501175025.36233-1-surenb@google.com> In-Reply-To: From: Suren Baghdasaryan Date: Wed, 3 May 2023 12:57:09 -0700 Message-ID: Subject: Re: [PATCH 1/3] mm: handle swap page faults under VMA lock if page is uncontended To: Yosry Ahmed Cc: Matthew Wilcox , akpm@linux-foundation.org, hannes@cmpxchg.org, mhocko@suse.com, josef@toxicpanda.com, jack@suse.cz, ldufour@linux.ibm.com, laurent.dufour@fr.ibm.com, michel@lespinasse.org, liam.howlett@oracle.com, jglisse@google.com, vbabka@suse.cz, minchan@google.com, dave@stgolabs.net, punit.agrawal@bytedance.com, lstoakes@gmail.com, hdanton@sina.com, apopple@nvidia.com, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-team@android.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Wed, May 3, 2023 at 1:34=E2=80=AFAM Yosry Ahmed = wrote: > > On Tue, May 2, 2023 at 4:05=E2=80=AFPM Suren Baghdasaryan wrote: > > > > On Tue, May 2, 2023 at 3:31=E2=80=AFPM Matthew Wilcox wrote: > > > > > > On Tue, May 02, 2023 at 09:36:03AM -0700, Suren Baghdasaryan wrote: > > > > On Tue, May 2, 2023 at 8:03=E2=80=AFAM Matthew Wilcox wrote: > > > > > > > > > > On Mon, May 01, 2023 at 10:04:56PM -0700, Suren Baghdasaryan wrot= e: > > > > > > On Mon, May 1, 2023 at 8:22=E2=80=AFPM Matthew Wilcox wrote: > > > > > > > > > > > > > > On Mon, May 01, 2023 at 07:30:13PM -0700, Suren Baghdasaryan = wrote: > > > > > > > > On Mon, May 1, 2023 at 7:02=E2=80=AFPM Matthew Wilcox wrote: > > > > > > > > > > > > > > > > > > On Mon, May 01, 2023 at 10:50:23AM -0700, Suren Baghdasar= yan wrote: > > > > > > > > > > +++ b/mm/memory.c > > > > > > > > > > @@ -3711,11 +3711,6 @@ vm_fault_t do_swap_page(struct v= m_fault *vmf) > > > > > > > > > > if (!pte_unmap_same(vmf)) > > > > > > > > > > goto out; > > > > > > > > > > > > > > > > > > > > - if (vmf->flags & FAULT_FLAG_VMA_LOCK) { > > > > > > > > > > - ret =3D VM_FAULT_RETRY; > > > > > > > > > > - goto out; > > > > > > > > > > - } > > > > > > > > > > - > > > > > > > > > > entry =3D pte_to_swp_entry(vmf->orig_pte); > > > > > > > > > > if (unlikely(non_swap_entry(entry))) { > > > > > > > > > > if (is_migration_entry(entry)) { > > > > > > > > > > > > > > > > > > You're missing the necessary fallback in the (!folio) cas= e. > > > > > > > > > swap_readpage() is synchronous and will sleep. > > > > > > > > > > > > > > > > True, but is it unsafe to do that under VMA lock and has to= be done > > > > > > > > under mmap_lock? > > > > > > > > > > > > > > ... you were the one arguing that we didn't want to wait for = I/O with > > > > > > > the VMA lock held? > > > > > > > > > > > > Well, that discussion was about waiting in folio_lock_or_retry(= ) with > > > > > > the lock being held. I argued against it because currently we d= rop > > > > > > mmap_lock lock before waiting, so if we don't drop VMA lock we = would > > > > > > be changing the current behavior which might introduce new > > > > > > regressions. In the case of swap_readpage and swapin_readahead = we > > > > > > already wait with mmap_lock held, so waiting with VMA lock held= does > > > > > > not introduce new problems (unless there is a need to hold mmap= _lock). > > > > > > > > > > > > That said, you are absolutely correct that this situation can b= e > > > > > > improved by dropping the lock in these cases too. I just didn't= want > > > > > > to attack everything at once. I believe after we agree on the a= pproach > > > > > > implemented in https://lore.kernel.org/all/20230501175025.36233= -3-surenb@google.com > > > > > > for dropping the VMA lock before waiting, these cases can be ad= ded > > > > > > easier. Does that make sense? > > > > > > > > > > OK, I looked at this path some more, and I think we're fine. Thi= s > > > > > patch is only called for SWP_SYNCHRONOUS_IO which is only set for > > > > > QUEUE_FLAG_SYNCHRONOUS devices, which are brd, zram and nvdimms > > > > > (both btt and pmem). So the answer is that we don't sleep in thi= s > > > > > path, and there's no need to drop the lock. > > > > > > > > Yes but swapin_readahead does sleep, so I'll have to handle that ca= se > > > > too after this. > > > > > > Sleeping is OK, we do that in pXd_alloc()! Do we block on I/O anywhe= re > > > in swapin_readahead()? It all looks like async I/O to me. > > > > Hmm. I thought that we have synchronous I/O in the following paths: > > swapin_readahead()->swap_cluster_readahead()->swap_readpage() > > swapin_readahead()->swap_vma_readahead()->swap_readpage() > > but just noticed that in both cases swap_readpage() is called with the > > synchronous parameter being false. So you are probably right here... > > In both swap_cluster_readahead() and swap_vma_readahead() it looks > like if the readahead window is 1 (aka we are not reading ahead), then > we jump to directly calling read_swap_cache_async() passing do_poll =3D > true, which means we may end up calling swap_readpage() passing > synchronous =3D true. > > I am not familiar with readahead heuristics, so I am not sure how > common this is, but it's something to think about. Uh, you are correct. If this branch is common, we could use the same "drop the lock and retry" pattern inside read_swap_cache_async(). That would be quite easy to implement. Thanks for checking on it! > > > Does that mean swapin_readahead() might return a page which does not > > have its content swapped-in yet? > >