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=-12.9 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1, USER_IN_DEF_DKIM_WL autolearn=ham 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 05C0BC47255 for ; Mon, 11 May 2020 07:38:26 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id B6E8B2075E for ; Mon, 11 May 2020 07:38:25 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="ZYITXXk0" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B6E8B2075E Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 45AB490000F; Mon, 11 May 2020 03:38:25 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 40C73900006; Mon, 11 May 2020 03:38:25 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3220A90000F; Mon, 11 May 2020 03:38:25 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0079.hostedemail.com [216.40.44.79]) by kanga.kvack.org (Postfix) with ESMTP id 1BC02900006 for ; Mon, 11 May 2020 03:38:25 -0400 (EDT) Received: from smtpin29.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id C4B54824805A for ; Mon, 11 May 2020 07:38:24 +0000 (UTC) X-FDA: 76803635328.29.song64_66823a9d9f645 X-HE-Tag: song64_66823a9d9f645 X-Filterd-Recvd-Size: 8283 Received: from mail-ot1-f65.google.com (mail-ot1-f65.google.com [209.85.210.65]) by imf35.hostedemail.com (Postfix) with ESMTP for ; Mon, 11 May 2020 07:38:24 +0000 (UTC) Received: by mail-ot1-f65.google.com with SMTP id i27so6759873ota.7 for ; Mon, 11 May 2020 00:38:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:in-reply-to:message-id:references :user-agent:mime-version; bh=VP11kHxLwXDXaTzKOz+HMa/draWDszvpLP/hhYf3Fi4=; b=ZYITXXk0Sv8/5ieeoX241h0bIN3/+/flJRugchMMNKwGQJF62m7yiaYuZVI0Z2a/Ev 5jZTGQCU1hQuEquqBEqyiPENflPKm8A85MS4g6fDaKW/l90DVcVfJBVTzPsouTpG3lmj lMe15tgjv6BBowKWTrC+EpVWx46wLf1GQtjVgarN9qkIXaHNh4QkeXwgUekIi+xkfIRc eT5CVGfVYVNZCpwhTj9das9U5at2ho6q70YrjMSzr+uV3cPM57V5BF6x86vXrNELetjN fjLh0Yiub4SlJwlqEienK33bJ5iGStJmdf7jmuufStHuHi9hYYVXfVrWo/QHaGI1kUAY B9cA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:in-reply-to:message-id :references:user-agent:mime-version; bh=VP11kHxLwXDXaTzKOz+HMa/draWDszvpLP/hhYf3Fi4=; b=Q37wicSR4rqOgNQLBJNFDbmww5J7DuVvy61Fy/TR3Iw9ZFljawDCStUrBk4texbWzL JRmrBX22SwYVDWORk9fmcFi4GONxn5xIIaSLGc1vLyqIaYch1bb7V5FlpfaarBPkQ3+h 0z2ey5+mjzg66MfB4WlWWu+mXfnWatAsn4g32pPW6RFdCUS/xuwPqTvsNCyzZDrRqFw+ 9C9oIQRrYJjwQRRNrVKJAaTn8oeb4klRCFqMacjOaViLzu6ky6P7H7yuBp5NSTo07T6Q lR2GOvFsNQ2SQX4EVLRPR6jp8YcCwX1uxWSQmKoOxCQY9ASIIuntn90JJarakccPejXl pHfw== X-Gm-Message-State: AGi0PubqFzjOcfnPoTJV7jZGv2y6SuK2ayNMkBaQKW5b1n5kjzBWNlC2 k9yRETHBzRZMjZziQPQ7yIoHTQ== X-Google-Smtp-Source: APiQypISrLk02tj60/eS4sArwh2wB0XcKehRZlfYJebU0VMeRFve1zu672J8EbwdJy+PndQMB8GoXQ== X-Received: by 2002:a05:6830:19f8:: with SMTP id t24mr10838744ott.326.1589182703345; Mon, 11 May 2020 00:38:23 -0700 (PDT) Received: from eggly.attlocal.net (172-10-233-147.lightspeed.sntcca.sbcglobal.net. [172.10.233.147]) by smtp.gmail.com with ESMTPSA id q3sm2671555oom.12.2020.05.11.00.38.21 (version=TLS1 cipher=ECDHE-ECDSA-AES128-SHA bits=128/128); Mon, 11 May 2020 00:38:22 -0700 (PDT) Date: Mon, 11 May 2020 00:38:04 -0700 (PDT) From: Hugh Dickins X-X-Sender: hugh@eggly.anvils To: Johannes Weiner cc: Joonsoo Kim , Alex Shi , Shakeel Butt , Hugh Dickins , Michal Hocko , "Kirill A. Shutemov" , Roman Gushchin , linux-mm@kvack.org, cgroups@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-team@fb.com Subject: Re: [PATCH 05/18] mm: memcontrol: convert page cache to a new mem_cgroup_charge() API In-Reply-To: <20200508160122.GB181181@cmpxchg.org> Message-ID: References: <20200420221126.341272-1-hannes@cmpxchg.org> <20200420221126.341272-6-hannes@cmpxchg.org> <20200422064041.GE6780@js1304-desktop> <20200422120946.GA358439@cmpxchg.org> <20200423052450.GA12538@js1304-desktop> <20200508160122.GB181181@cmpxchg.org> User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII 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 Fri, 8 May 2020, Johannes Weiner wrote: > > I looked at this some more, as well as compared it to non-shmem > swapping. My conclusion is - and Hugh may correct me on this - that > the deletion looks mandatory but is actually an optimization. Page > reclaim will ultimately pick these pages up. > > When non-shmem pages are swapped in by readahead (locked until IO > completes) and their page tables are simultaneously unmapped, the > zap_pte_range() code calls free_swap_and_cache() and the locked pages > are stranded in the swap cache with no page table references. We rely > on page reclaim to pick them up later on. > > The same appears to be true for shmem. If the references to the swap > page are zapped while we're trying to swap in, we can strand the page > in the swap cache. But it's not up to swapin to detect this reliably, > it just frees the page more quickly than having to wait for reclaim. I think you've got all that exactly right, thanks for working it out. It originates from v3.7's 215c02bc33bb ("tmpfs: fix shmem_getpage_gfp() VM_BUG_ON") - in which I also had to thank you. I think I chose to do the delete_from_swap_cache() right there, partly because of following shmem_unuse_inode() code which already did that, partly on the basis that while we have to observe the case then it's better to clean it up, and partly out of guilt that our page lock here is what had prevented shmem_undo_range() from completing its job; but I believe you're right that unused swapcache reclaim would sort it out eventually. > > That being said, my patch introduces potentially undesirable behavior > (although AFAICS no correctness problem): We should only delete the > page from swapcache when we actually raced with undo_range - which we > see from the swap entry having been purged from the page cache > tree. If we delete the page from swapcache just because we failed to > charge it, the next fault has to read the still-valid page again from > the swap device. Yes. > > I'm going to include this: I haven't pulled down your V2 series into a tree yet (expecting perhaps a respin from Alex on top, when I hope to switch over to trying them both), so haven't looked into the context and may be wrong... > > diff --git a/mm/shmem.c b/mm/shmem.c > index e80167927dce..236642775f89 100644 > --- a/mm/shmem.c > +++ b/mm/shmem.c > @@ -640,7 +640,7 @@ static int shmem_add_to_page_cache(struct page *page, > xas_lock_irq(&xas); > entry = xas_find_conflict(&xas); > if (entry != expected) > - xas_set_err(&xas, -EEXIST); > + xas_set_err(&xas, expected ? -ENOENT : -EEXIST); Two things on this. Minor matter of taste, I'd prefer that as xas_set_err(&xas, entry ? -EEXIST : -ENOENT); which would be more general and more understandable - but what you have written should be fine for the actual callers. Except... I think returning -ENOENT there will not work correctly, in the case of a punched hole. Because (unless you've reworked it and I just haven't looked) shmem_getpage_gfp() knows to retry in the case of -EEXIST, but -ENOENT will percolate up to shmem_fault() and result in a SIGBUS, or a read/write error, when the hole should just get refilled instead. Not something that needs fixing in a hurry (it took trinity to generate this racy case in the first place), I'll take another look once I've pulled it into a tree (or collected next mmotm) - unless you've already have changed it around by then. Hugh > xas_create_range(&xas); > if (xas_error(&xas)) > goto unlock; > @@ -1683,17 +1683,18 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index, > error = shmem_add_to_page_cache(page, mapping, index, > swp_to_radix_entry(swap), gfp, > charge_mm); > - /* > - * We already confirmed swap under page lock, and make no > - * memory allocation here, so usually no possibility of error; > - * but free_swap_and_cache() only trylocks a page, so it is > - * just possible that the entry has been truncated or > - * holepunched since swap was confirmed. shmem_undo_range() > - * will have done some of the unaccounting, now > - * delete_from_swap_cache() will do the rest. > - */ > if (error) { > - delete_from_swap_cache(page); > + /* > + * We already confirmed swap under page lock, but > + * free_swap_and_cache() only trylocks a page, so it > + * is just possible that the entry has been truncated > + * or holepunched since swap was confirmed. > + * shmem_undo_range() will have done some of the > + * unaccounting, now delete_from_swap_cache() will do > + * the rest. > + */ > + if (error == -ENOENT) > + delete_from_swap_cache(page); > goto failed; > } > >