From: Joonsoo Kim <js1304@gmail.com> To: Johannes Weiner <hannes@cmpxchg.org> Cc: Alex Shi <alex.shi@linux.alibaba.com>, Shakeel Butt <shakeelb@google.com>, Hugh Dickins <hughd@google.com>, Michal Hocko <mhocko@suse.com>, "Kirill A. Shutemov" <kirill@shutemov.name>, Roman Gushchin <guro@fb.com>, Linux Memory Management List <linux-mm@kvack.org>, cgroups@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>, kernel-team@fb.com Subject: Re: [PATCH 16/18] mm: memcontrol: charge swapin pages on instantiation Date: Tue, 28 Apr 2020 15:49:41 +0900 [thread overview] Message-ID: <CAAmzW4PQ=Bs=GcCWkORx6YDF-35TaeCYULprVqnVpCBSP9S0Kg@mail.gmail.com> (raw) In-Reply-To: <20200424025135.GB464082@cmpxchg.org> 2020년 4월 24일 (금) 오전 11:51, Johannes Weiner <hannes@cmpxchg.org>님이 작성: > > 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 = 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 = 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 != -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 = 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 that later task allocates a page faster though it's not usual case. However, I also agree your reasoning so will not insist more. Thanks.
WARNING: multiple messages have this Message-ID (diff)
From: Joonsoo Kim <js1304-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> To: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org> Cc: Alex Shi <alex.shi-KPsoFbNs7GizrGE5bRqYAgC/G2K4zDHf@public.gmane.org>, Shakeel Butt <shakeelb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>, Hugh Dickins <hughd-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>, Michal Hocko <mhocko-IBi9RG/b67k@public.gmane.org>, "Kirill A. Shutemov" <kirill-oKw7cIdHH8eLwutG50LtGA@public.gmane.org>, Roman Gushchin <guro-b10kYP2dOMg@public.gmane.org>, Linux Memory Management List <linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org>, cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, LKML <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, kernel-team-b10kYP2dOMg@public.gmane.org Subject: Re: [PATCH 16/18] mm: memcontrol: charge swapin pages on instantiation Date: Tue, 28 Apr 2020 15:49:41 +0900 [thread overview] Message-ID: <CAAmzW4PQ=Bs=GcCWkORx6YDF-35TaeCYULprVqnVpCBSP9S0Kg@mail.gmail.com> (raw) In-Reply-To: <20200424025135.GB464082-druUgvl0LCNAfugRpC6u6w@public.gmane.org> 2020년 4월 24일 (금) 오전 11:51, Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>님이 작성: > > 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 = 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 = 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 != -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 = 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 that later task allocates a page faster though it's not usual case. However, I also agree your reasoning so will not insist more. Thanks.
next prev parent reply other threads:[~2020-04-28 6:49 UTC|newest] Thread overview: 140+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-04-20 22:11 [PATCH 00/18] mm: memcontrol: charge swapin pages on instantiation Johannes Weiner 2020-04-20 22:11 ` Johannes Weiner 2020-04-20 22:11 ` [PATCH 01/18] mm: fix NUMA node file count error in replace_page_cache() Johannes Weiner 2020-04-20 22:11 ` Johannes Weiner 2020-04-21 8:28 ` Alex Shi 2020-04-21 8:28 ` Alex Shi 2020-04-21 19:13 ` Shakeel Butt 2020-04-21 19:13 ` Shakeel Butt 2020-04-21 19:13 ` Shakeel Butt 2020-04-22 6:34 ` Joonsoo Kim 2020-04-22 6:34 ` Joonsoo Kim 2020-04-20 22:11 ` [PATCH 02/18] mm: memcontrol: fix theoretical race in charge moving Johannes Weiner 2020-04-20 22:11 ` Johannes Weiner 2020-04-22 6:36 ` Joonsoo Kim 2020-04-22 6:36 ` Joonsoo Kim 2020-04-22 16:51 ` Shakeel Butt 2020-04-22 16:51 ` Shakeel Butt 2020-04-22 16:51 ` Shakeel Butt 2020-04-22 17:42 ` Johannes Weiner 2020-04-22 17:42 ` Johannes Weiner 2020-04-22 18:01 ` Shakeel Butt 2020-04-22 18:01 ` Shakeel Butt 2020-04-22 18:01 ` Shakeel Butt 2020-04-22 18:02 ` Shakeel Butt 2020-04-22 18:02 ` Shakeel Butt 2020-04-20 22:11 ` [PATCH 03/18] mm: memcontrol: drop @compound parameter from memcg charging API Johannes Weiner 2020-04-20 22:11 ` Johannes Weiner 2020-04-21 9:11 ` Alex Shi 2020-04-21 9:11 ` Alex Shi 2020-04-22 6:37 ` Joonsoo Kim 2020-04-22 6:37 ` Joonsoo Kim 2020-04-22 17:30 ` Shakeel Butt 2020-04-22 17:30 ` Shakeel Butt 2020-04-22 17:30 ` Shakeel Butt 2020-04-20 22:11 ` [PATCH 04/18] mm: memcontrol: move out cgroup swaprate throttling Johannes Weiner 2020-04-21 9:11 ` Alex Shi 2020-04-21 9:11 ` Alex Shi 2020-04-22 6:37 ` Joonsoo Kim 2020-04-22 6:37 ` Joonsoo Kim 2020-04-22 22:20 ` Shakeel Butt 2020-04-22 22:20 ` Shakeel Butt 2020-04-22 22:20 ` Shakeel Butt 2020-04-20 22:11 ` [PATCH 05/18] mm: memcontrol: convert page cache to a new mem_cgroup_charge() API Johannes Weiner 2020-04-20 22:11 ` Johannes Weiner 2020-04-21 9:12 ` Alex Shi 2020-04-21 9:12 ` Alex Shi 2020-04-22 6:40 ` Joonsoo Kim 2020-04-22 6:40 ` Joonsoo Kim 2020-04-22 12:09 ` Johannes Weiner 2020-04-22 12:09 ` Johannes Weiner 2020-04-23 5:25 ` Joonsoo Kim 2020-05-08 16:01 ` Johannes Weiner 2020-05-11 1:57 ` Joonsoo Kim 2020-05-11 7:38 ` Hugh Dickins 2020-05-11 7:38 ` Hugh Dickins 2020-05-11 7:38 ` Hugh Dickins 2020-05-11 15:06 ` Johannes Weiner 2020-05-11 16:32 ` Hugh Dickins 2020-05-11 16:32 ` Hugh Dickins 2020-05-11 16:32 ` Hugh Dickins 2020-05-11 18:10 ` Johannes Weiner 2020-05-11 18:10 ` Johannes Weiner 2020-05-11 18:12 ` Johannes Weiner 2020-05-11 18:44 ` Hugh Dickins 2020-05-11 18:44 ` Hugh Dickins 2020-05-11 18:44 ` Hugh Dickins 2020-04-20 22:11 ` [PATCH 06/18] mm: memcontrol: prepare uncharging for removal of private page type counters Johannes Weiner 2020-04-21 9:12 ` Alex Shi 2020-04-21 9:12 ` Alex Shi 2020-04-22 6:41 ` Joonsoo Kim 2020-04-22 6:41 ` Joonsoo Kim 2020-04-20 22:11 ` [PATCH 07/18] mm: memcontrol: prepare move_account " Johannes Weiner 2020-04-21 9:13 ` Alex Shi 2020-04-21 9:13 ` Alex Shi 2020-04-22 6:41 ` Joonsoo Kim 2020-04-22 6:41 ` Joonsoo Kim 2020-04-20 22:11 ` [PATCH 08/18] mm: memcontrol: prepare cgroup vmstat infrastructure for native anon counters Johannes Weiner 2020-04-22 6:42 ` Joonsoo Kim 2020-04-22 6:42 ` Joonsoo Kim 2020-04-20 22:11 ` [PATCH 09/18] mm: memcontrol: switch to native NR_FILE_PAGES and NR_SHMEM counters Johannes Weiner 2020-04-22 6:42 ` Joonsoo Kim 2020-04-22 6:42 ` Joonsoo Kim 2020-04-20 22:11 ` [PATCH 10/18] mm: memcontrol: switch to native NR_ANON_MAPPED counter Johannes Weiner 2020-04-22 6:51 ` Joonsoo Kim 2020-04-22 12:28 ` Johannes Weiner 2020-04-23 5:27 ` Joonsoo Kim 2020-04-20 22:11 ` [PATCH 11/18] mm: memcontrol: switch to native NR_ANON_THPS counter Johannes Weiner 2020-04-20 22:11 ` Johannes Weiner 2020-04-24 0:29 ` Joonsoo Kim 2020-04-24 0:29 ` Joonsoo Kim 2020-04-20 22:11 ` [PATCH 12/18] mm: memcontrol: convert anon and file-thp to new mem_cgroup_charge() API Johannes Weiner 2020-04-20 22:11 ` Johannes Weiner 2020-04-24 0:29 ` Joonsoo Kim 2020-04-24 0:29 ` Joonsoo Kim 2020-04-20 22:11 ` [PATCH 13/18] mm: memcontrol: drop unused try/commit/cancel charge API Johannes Weiner 2020-04-24 0:30 ` Joonsoo Kim 2020-04-24 0:30 ` Joonsoo Kim 2020-04-20 22:11 ` [PATCH 14/18] mm: memcontrol: prepare swap controller setup for integration Johannes Weiner 2020-04-20 22:11 ` Johannes Weiner 2020-04-24 0:30 ` Joonsoo Kim 2020-04-24 0:30 ` Joonsoo Kim 2020-04-20 22:11 ` [PATCH 15/18] mm: memcontrol: make swap tracking an integral part of memory control Johannes Weiner 2020-04-20 22:11 ` Johannes Weiner 2020-04-21 9:27 ` Alex Shi 2020-04-21 9:27 ` Alex Shi 2020-04-21 14:39 ` Johannes Weiner 2020-04-21 14:39 ` Johannes Weiner 2020-04-22 3:14 ` Alex Shi 2020-04-22 3:14 ` Alex Shi 2020-04-22 13:30 ` Johannes Weiner 2020-04-22 13:30 ` Johannes Weiner 2020-04-22 13:40 ` Alex Shi 2020-04-22 13:43 ` Alex Shi 2020-04-24 0:30 ` Joonsoo Kim 2020-04-24 0:30 ` Joonsoo Kim 2020-04-24 3:01 ` Johannes Weiner 2020-04-20 22:11 ` [PATCH 16/18] mm: memcontrol: charge swapin pages on instantiation Johannes Weiner 2020-04-20 22:11 ` Johannes Weiner 2020-04-21 9:21 ` Alex Shi 2020-04-21 9:21 ` Alex Shi 2020-04-24 0:44 ` Joonsoo Kim 2020-04-24 2:51 ` Johannes Weiner 2020-04-24 2:51 ` Johannes Weiner 2020-04-28 6:49 ` Joonsoo Kim [this message] 2020-04-28 6:49 ` Joonsoo Kim 2020-04-28 6:49 ` Joonsoo Kim 2020-04-20 22:11 ` [PATCH 17/18] mm: memcontrol: delete unused lrucare handling Johannes Weiner 2020-04-20 22:11 ` Johannes Weiner 2020-04-24 0:46 ` Joonsoo Kim 2020-04-24 0:46 ` Joonsoo Kim 2020-04-20 22:11 ` [PATCH 18/18] mm: memcontrol: update page->mem_cgroup stability rules Johannes Weiner 2020-04-21 9:20 ` Alex Shi 2020-04-21 9:20 ` Alex Shi 2020-04-24 0:48 ` Joonsoo Kim 2020-04-24 0:48 ` Joonsoo Kim 2020-04-21 9:10 ` Hillf Danton 2020-04-21 14:34 ` Johannes Weiner 2020-04-21 14:34 ` Johannes Weiner 2020-04-21 9:32 ` [PATCH 00/18] mm: memcontrol: charge swapin pages on instantiation Alex Shi 2020-04-21 9:32 ` Alex Shi
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAAmzW4PQ=Bs=GcCWkORx6YDF-35TaeCYULprVqnVpCBSP9S0Kg@mail.gmail.com' \ --to=js1304@gmail.com \ --cc=alex.shi@linux.alibaba.com \ --cc=cgroups@vger.kernel.org \ --cc=guro@fb.com \ --cc=hannes@cmpxchg.org \ --cc=hughd@google.com \ --cc=kernel-team@fb.com \ --cc=kirill@shutemov.name \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mhocko@suse.com \ --cc=shakeelb@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.