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 28B0AC77B75 for ; Wed, 3 May 2023 20:58:03 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 6B65D6B0078; Wed, 3 May 2023 16:58:03 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 6676E6B007B; Wed, 3 May 2023 16:58:03 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 556506B007D; Wed, 3 May 2023 16:58:03 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from mail-ej1-f47.google.com (mail-ej1-f47.google.com [209.85.218.47]) by kanga.kvack.org (Postfix) with ESMTP id 022C66B0078 for ; Wed, 3 May 2023 16:58:03 -0400 (EDT) Received: by mail-ej1-f47.google.com with SMTP id a640c23a62f3a-953343581a4so873840866b.3 for ; Wed, 03 May 2023 13:58:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1683147482; x=1685739482; 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=fOKjucXS3bLNlM0zdkAZGUqfwQjiAxpZxAhc5U8CzHU=; b=M76iV/Cp1NdgRUq/sw7nrkkQZ1f5OQw/FLMsnaaaGZ9WDv4dmUFRbQJ7/aaMVURbXH jtw5XjZVNFYAGal1iu1GucmpcCJ0CgdZf5Dbshiuo8AvHjIdf0WWbQM1167FJbGBLOVb +m1nnTAklcYmPuroSxwm4K/URkI9jq+Kt+7F8TF3u4xM52Qw3iUGnjjoffhghD/7aHOY ZqstTs/T4J5k1rFcbF9bu5rwKRDcjvJp9ImpLerVMoag/moEHoqxkO/YcyexIo1OJITP UPEfOHgdeHnqeQ1WiBjWWHnrzmC9+Io4K3aoUpIz3h6fXx4MOAIJD5q5jVcoAjZcEg/c i7NA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1683147482; x=1685739482; 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=fOKjucXS3bLNlM0zdkAZGUqfwQjiAxpZxAhc5U8CzHU=; b=MobgIuBZ3Po1umHxovzb3TiIf+j4/XtsIYmeUYKUai6tf1hbvLc1wfKUyeZXgopi90 +XcqWGyMgktu0oA8hCTQgzS66aF3XSAuPIRjn7thiayHsmp8lQD9JrKl1B4OkUCwXh5u m59AetCKcsxcBfCJL1qSgKuq0FMc0F6wicwWy2rtGFnbaKuHojTsp2ZWmUaX2YD1ec2m ynZUSMxbtXluume0+Fr3Wmm4FlvweDbJBSY7xPcsgG/GMJ39r6uSep0lZKFtU92wEXZr 25jqEpVzLbhv6fmiAo+37MEWZoG2H19WSF/9yR0c+en/eDYaZOUzE1goRdDd5fSOaWS+ gKuQ== X-Gm-Message-State: AC+VfDw5e2IJQhVezV3mwApf8rmc4+GNDNTEfnueCJYoXEksbsvC+IK2 FHrpi1yH95dcALUqdjKDKdg9TB/BV1V7xZ0J/4OgFw== X-Google-Smtp-Source: ACHHUZ7bG8Xn1LjA65SfnlqTNN22QeCgktHev9DSXBWTCamKtyQdrW7/6BUqHXApnZNL4m4+BsdZL7Rw4nFGoBYOPAM= X-Received: by 2002:a17:906:6a14:b0:953:7be7:91de with SMTP id qw20-20020a1709066a1400b009537be791demr4660114ejc.20.1683147481570; Wed, 03 May 2023 13:58:01 -0700 (PDT) MIME-Version: 1.0 References: <20230501175025.36233-1-surenb@google.com> In-Reply-To: From: Yosry Ahmed Date: Wed, 3 May 2023 13:57:17 -0700 Message-ID: Subject: Re: [PATCH 1/3] mm: handle swap page faults under VMA lock if page is uncontended To: Suren Baghdasaryan , Ying 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 12:57=E2=80=AFPM Suren Baghdasaryan wrote: > > 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 wr= ote: > > > > > > > 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 Baghdasarya= n 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 Baghdas= aryan wrote: > > > > > > > > > > > +++ b/mm/memory.c > > > > > > > > > > > @@ -3711,11 +3711,6 @@ vm_fault_t do_swap_page(struct= vm_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) c= ase. > > > > > > > > > > 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 fo= r I/O with > > > > > > > > the VMA lock held? > > > > > > > > > > > > > > Well, that discussion was about waiting in folio_lock_or_retr= y() with > > > > > > > the lock being held. I argued against it because currently we= drop > > > > > > > mmap_lock lock before waiting, so if we don't drop VMA lock w= e would > > > > > > > be changing the current behavior which might introduce new > > > > > > > regressions. In the case of swap_readpage and swapin_readahea= d we > > > > > > > already wait with mmap_lock held, so waiting with VMA lock he= ld does > > > > > > > not introduce new problems (unless there is a need to hold mm= ap_lock). > > > > > > > > > > > > > > That said, you are absolutely correct that this situation can= be > > > > > > > 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= approach > > > > > > > implemented in https://lore.kernel.org/all/20230501175025.362= 33-3-surenb@google.com > > > > > > > for dropping the VMA lock before waiting, these cases can be = added > > > > > > > easier. Does that make sense? > > > > > > > > > > > > OK, I looked at this path some more, and I think we're fine. T= his > > > > > > patch is only called for SWP_SYNCHRONOUS_IO which is only set f= or > > > > > > QUEUE_FLAG_SYNCHRONOUS devices, which are brd, zram and nvdimms > > > > > > (both btt and pmem). So the answer is that we don't sleep in t= his > > > > > > path, and there's no need to drop the lock. > > > > > > > > > > Yes but swapin_readahead does sleep, so I'll have to handle that = case > > > > > too after this. > > > > > > > > Sleeping is OK, we do that in pXd_alloc()! Do we block on I/O anyw= here > > > > 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 th= e > > > 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! I am honestly not sure how common this is. +Ying who might have a better idea. > > > > > > > Does that mean swapin_readahead() might return a page which does not > > > have its content swapped-in yet? > > >