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 X-Spam-Level: X-Spam-Status: No, score=-0.6 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 483A8C83000 for ; Tue, 28 Apr 2020 06:49:56 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id E94BD206B9 for ; Tue, 28 Apr 2020 06:49:55 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="NXZIXjwp" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E94BD206B9 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 860178E0005; Tue, 28 Apr 2020 02:49:55 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 7E90B8E0001; Tue, 28 Apr 2020 02:49:55 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6B0CF8E0005; Tue, 28 Apr 2020 02:49:55 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0086.hostedemail.com [216.40.44.86]) by kanga.kvack.org (Postfix) with ESMTP id 5056B8E0001 for ; Tue, 28 Apr 2020 02:49:55 -0400 (EDT) Received: from smtpin19.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 064832C98 for ; Tue, 28 Apr 2020 06:49:55 +0000 (UTC) X-FDA: 76756338750.19.spark91_568a5579a542a X-HE-Tag: spark91_568a5579a542a X-Filterd-Recvd-Size: 6988 Received: from mail-qv1-f66.google.com (mail-qv1-f66.google.com [209.85.219.66]) by imf24.hostedemail.com (Postfix) with ESMTP for ; Tue, 28 Apr 2020 06:49:54 +0000 (UTC) Received: by mail-qv1-f66.google.com with SMTP id t8so9886651qvw.5 for ; Mon, 27 Apr 2020 23:49:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=LDAfY6IMuAEGrWvee7RhilnIfuGLqiwe5VYND28O0Zw=; b=NXZIXjwpYXLULNObRahj5gByNuIc0r19/uWT7O/oda8walfipdmJpBY1rPc1D8E4XV Mcp5AxJAEhso0Yggwa9MT5aH7XfYpfIGWcpEN73Q5c8eVUJkXLKNkOzv8kS+d6p2/+bi MSqgWc2wMs+ad4oOVM/k6Bb3hzv8ZjKuqoWeMfOjn79WRW7GuQhUQKpJkY8MnrC4ADZ7 XHVb+d9PA9QiMKEayiG9jqLeZr2bvDrNDlMsleqnn0mOFmuTZQFSTCQ+oVp0+Xd2Cs48 lCHRe4JRhamssskAVpjbzRGiOlB88EYJQcZMmijyfAHcHsHVxYsBwL9LzXPXPeA9aVE1 euxQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=LDAfY6IMuAEGrWvee7RhilnIfuGLqiwe5VYND28O0Zw=; b=X50Fxwz2itUuDaknXFNtFSH2KqJgWBZBRGZAGBamdfJBe3mXLDdbZQH7f8WOVVLBZ4 bSKD/2SfTKiDLZ3TIF1/apNlWPA21i/dWhAM6agyeRJgLYG3lZJm2727TW/2KmE4+LwO 6D/kqJ5XbPpX089DSLiupyMbVT5Sqmokeir1EFkF85l6zRxCxYlXVdVrdz70VSPqFolh vBLXwqc0pxO7/hAcDyvOM/ZAYMpZIRgzaPvQyS4MJ6at+/BMj/yjENScAVkcwMFFkgW9 qiZ2ODDx/R3Wm9Dgm71LKtrff1p1iF2O5rTTMX7Fen91upxWntKCd0X7kU67iJK/Qq4i pbiw== X-Gm-Message-State: AGi0PuafoIKANkeak3CSuytI8DL039lYGpW5+C8VLj4vkcm/dMsxVuzF dUjOUN9ZJ5k3JaeFqhmCwc6ICMV2+KCFzmAK79I= X-Google-Smtp-Source: APiQypImVaEzsNMUlWQJDm0dBB+htx0aMNuXLzgzzwXjbioyFBt5+O7nAMOdkcmCqRMHEFU2omiDgliGDxdZnfqx9HA= X-Received: by 2002:ad4:45ac:: with SMTP id y12mr26016776qvu.227.1588056592811; Mon, 27 Apr 2020 23:49:52 -0700 (PDT) MIME-Version: 1.0 References: <20200420221126.341272-1-hannes@cmpxchg.org> <20200420221126.341272-17-hannes@cmpxchg.org> <20200424004441.GF13929@js1304-desktop> <20200424025135.GB464082@cmpxchg.org> In-Reply-To: <20200424025135.GB464082@cmpxchg.org> From: Joonsoo Kim Date: Tue, 28 Apr 2020 15:49:41 +0900 Message-ID: Subject: Re: [PATCH 16/18] mm: memcontrol: charge swapin pages on instantiation To: Johannes Weiner Cc: Alex Shi , Shakeel Butt , Hugh Dickins , Michal Hocko , "Kirill A. Shutemov" , Roman Gushchin , Linux Memory Management List , cgroups@vger.kernel.org, LKML , kernel-team@fb.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: 2020=EB=85=84 4=EC=9B=94 24=EC=9D=BC (=EA=B8=88) =EC=98=A4=EC=A0=84 11:51, = Johannes Weiner =EB=8B=98=EC=9D=B4 =EC=9E=91=EC=84=B1: > > On Fri, Apr 24, 2020 at 09:44:42AM +0900, Joonsoo Kim wrote: > > On Mon, Apr 20, 2020 at 06:11:24PM -0400, Johannes Weiner wrote: > > > @@ -412,31 +407,43 @@ struct page *__read_swap_cache_async(swp_entry_= t entry, gfp_t gfp_mask, > > > */ > > > cond_resched(); > > > continue; > > > - } else if (err) /* swp entry is obsolete ? */ > > > - break; > > > - > > > - /* May fail (-ENOMEM) if XArray node allocation failed. *= / > > > - __SetPageLocked(new_page); > > > - __SetPageSwapBacked(new_page); > > > - err =3D add_to_swap_cache(new_page, entry, gfp_mask & GFP= _KERNEL); > > > - if (likely(!err)) { > > > - /* Initiate read into locked page */ > > > - SetPageWorkingset(new_page); > > > - lru_cache_add_anon(new_page); > > > - *new_page_allocated =3D true; > > > - return new_page; > > > } > > > - __ClearPageLocked(new_page); > > > - /* > > > - * add_to_swap_cache() doesn't return -EEXIST, so we can = safely > > > - * clear SWAP_HAS_CACHE flag. > > > - */ > > > - put_swap_page(new_page, entry); > > > - } while (err !=3D -ENOMEM); > > > + if (err) /* swp entry is obsolete ? */ > > > + return NULL; > > > > "if (err)" is not needed since "!err" is already exiting the loop. > > But we don't want to leave the loop, we want to leave the > function. For example, if swapcache_prepare() says the entry is gone > (-ENOENT), we don't want to exit the loop and allocate a page for it. Yes, so I said "if (err)" is not needed. Just "return NULL;" would be enough. > > > + > > > + /* > > > + * The swap entry is ours to swap in. Prepare a new page. > > > + */ > > > + > > > + page =3D alloc_page_vma(gfp_mask, vma, addr); > > > + if (!page) > > > + goto fail_free; > > > + > > > + __SetPageLocked(page); > > > + __SetPageSwapBacked(page); > > > + > > > + /* May fail (-ENOMEM) if XArray node allocation failed. */ > > > + if (add_to_swap_cache(page, entry, gfp_mask & GFP_KERNEL)) > > > + goto fail_unlock; > > > + > > > + if (mem_cgroup_charge(page, NULL, gfp_mask & GFP_KERNEL, false)) > > > + goto fail_delete; > > > + > > > > I think that following order of operations is better than yours. > > > > 1. page alloc > > 2. memcg charge > > 3. swapcache_prepare > > 4. add_to_swap_cache > > > > Reason is that page allocation and memcg charging could take for a > > long time due to reclaim and other tasks waiting this swapcache page > > could be blocked inbetween swapcache_prepare() and add_to_swap_cache(). > > I see how that would be preferable, but memcg charging actually needs > the swap(cache) information to figure out the cgroup that owned it at > swapout, then uncharge the swapcache and drop the swap cgroup record. > > Maybe it could be done, but I'm not sure that level of surgery would > be worth the benefits? Whoever else would be trying to swap the page > in at the same time is likely in the same memory situation, and would > not necessarily be able to allocate pages any faster. Hmm, at least, some modifications are needed since waiting task would do busy waiting in the loop and it wastes overall system cpu time. I still think that changing operation order is better since it's possible t= hat later task allocates a page faster though it's not usual case. However, I also agree your reasoning so will not insist more. Thanks.