From: Laurent Dufour <ldufour@linux.vnet.ibm.com>
To: Daniel Jordan <daniel.m.jordan@oracle.com>,
linux-mm@kvack.org, linux-kernel@vger.kernel.org,
cgroups@vger.kernel.org
Cc: aaron.lu@intel.com, ak@linux.intel.com,
akpm@linux-foundation.org, dave.dice@oracle.com,
dave.hansen@linux.intel.com, hannes@cmpxchg.org,
levyossi@icloud.com, mgorman@techsingularity.net,
mhocko@kernel.org, Pavel.Tatashin@microsoft.com,
steven.sistare@oracle.com, tim.c.chen@intel.com,
vdavydov.dev@gmail.com, ying.huang@intel.com
Subject: Re: [RFC PATCH v2 1/8] mm, memcontrol.c: make memcg lru stats thread-safe without lru_lock
Date: Tue, 11 Sep 2018 18:32:17 +0200 [thread overview]
Message-ID: <e62ef1a0-9518-5a16-df5b-86977b4e8881@linux.vnet.ibm.com> (raw)
In-Reply-To: <20180911004240.4758-2-daniel.m.jordan@oracle.com>
On 11/09/2018 02:42, Daniel Jordan wrote:
> lru_lock needs to be held to update memcg LRU statistics. This
> requirement arises fairly naturally based on when the stats are updated
> because callers are holding lru_lock already.
>
> In preparation for allowing concurrent adds and removes from the LRU,
> however, make concurrent updates to these statistics safe without
> lru_lock. The lock continues to be held until later in the series, when
> it is replaced with a rwlock that also disables preemption, maintaining
> the assumption of __mod_lru_zone_size, which is introduced here.
>
> Follow the existing pattern for statistics in memcontrol.h by using a
> combination of per-cpu counters and atomics.
>
> Remove the negative statistics warning from ca707239e8a7 ("mm:
> update_lru_size warn and reset bad lru_size"). Although an earlier
> version of this patch updated the warning to account for the error
> introduced by the per-cpu counters, Hugh says this warning has not been
> seen in the wild and that for simplicity's sake it should probably just
> be removed.
>
> Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
> ---
> include/linux/memcontrol.h | 43 +++++++++++++++++++++++++++++---------
> mm/memcontrol.c | 29 +++++++------------------
> 2 files changed, 40 insertions(+), 32 deletions(-)
>
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index d99b71bc2c66..6377dc76dc41 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -99,7 +99,8 @@ struct mem_cgroup_reclaim_iter {
> };
>
> struct lruvec_stat {
> - long count[NR_VM_NODE_STAT_ITEMS];
> + long node[NR_VM_NODE_STAT_ITEMS];
> + long lru_zone_size[MAX_NR_ZONES][NR_LRU_LISTS];
It might be better to use different name for the lru_zone_size field to
distinguish it from the one in the mem_cgroup_per_node structure.
> };
>
> /*
> @@ -109,9 +110,8 @@ struct mem_cgroup_per_node {
> struct lruvec lruvec;
>
> struct lruvec_stat __percpu *lruvec_stat_cpu;
> - atomic_long_t lruvec_stat[NR_VM_NODE_STAT_ITEMS];
> -
> - unsigned long lru_zone_size[MAX_NR_ZONES][NR_LRU_LISTS];
> + atomic_long_t node_stat[NR_VM_NODE_STAT_ITEMS];
> + atomic_long_t lru_zone_size[MAX_NR_ZONES][NR_LRU_LISTS];
>
> struct mem_cgroup_reclaim_iter iter[DEF_PRIORITY + 1];
>
> @@ -446,7 +446,7 @@ unsigned long mem_cgroup_get_lru_size(struct lruvec *lruvec, enum lru_list lru)
>
> mz = container_of(lruvec, struct mem_cgroup_per_node, lruvec);
> for (zid = 0; zid < MAX_NR_ZONES; zid++)
> - nr_pages += mz->lru_zone_size[zid][lru];
> + nr_pages += atomic64_read(&mz->lru_zone_size[zid][lru]);
> return nr_pages;
> }
>
> @@ -457,7 +457,7 @@ unsigned long mem_cgroup_get_zone_lru_size(struct lruvec *lruvec,
> struct mem_cgroup_per_node *mz;
>
> mz = container_of(lruvec, struct mem_cgroup_per_node, lruvec);
> - return mz->lru_zone_size[zone_idx][lru];
> + return atomic64_read(&mz->lru_zone_size[zone_idx][lru]);
> }
>
> void mem_cgroup_handle_over_high(void);
> @@ -575,7 +575,7 @@ static inline unsigned long lruvec_page_state(struct lruvec *lruvec,
> return node_page_state(lruvec_pgdat(lruvec), idx);
>
> pn = container_of(lruvec, struct mem_cgroup_per_node, lruvec);
> - x = atomic_long_read(&pn->lruvec_stat[idx]);
> + x = atomic_long_read(&pn->node_stat[idx]);
> #ifdef CONFIG_SMP
> if (x < 0)
> x = 0;
> @@ -601,12 +601,12 @@ static inline void __mod_lruvec_state(struct lruvec *lruvec,
> __mod_memcg_state(pn->memcg, idx, val);
>
> /* Update lruvec */
> - x = val + __this_cpu_read(pn->lruvec_stat_cpu->count[idx]);
> + x = val + __this_cpu_read(pn->lruvec_stat_cpu->node[idx]);
> if (unlikely(abs(x) > MEMCG_CHARGE_BATCH)) {
> - atomic_long_add(x, &pn->lruvec_stat[idx]);
> + atomic_long_add(x, &pn->node_stat[idx]);
> x = 0;
> }
> - __this_cpu_write(pn->lruvec_stat_cpu->count[idx], x);
> + __this_cpu_write(pn->lruvec_stat_cpu->node[idx], x);
> }
>
> static inline void mod_lruvec_state(struct lruvec *lruvec,
> @@ -619,6 +619,29 @@ static inline void mod_lruvec_state(struct lruvec *lruvec,
> local_irq_restore(flags);
> }
>
> +/**
> + * __mod_lru_zone_size - update memcg lru statistics in batches
> + *
> + * Updates memcg lru statistics using per-cpu counters that spill into atomics
> + * above a threshold.
> + *
> + * Assumes that the caller has disabled preemption. IRQs may be enabled
> + * because this function is not called from irq context.
> + */
> +static inline void __mod_lru_zone_size(struct mem_cgroup_per_node *pn,
> + enum lru_list lru, int zid, int val)
> +{
> + long x;
> + struct lruvec_stat __percpu *lruvec_stat_cpu = pn->lruvec_stat_cpu;
> +
> + x = val + __this_cpu_read(lruvec_stat_cpu->lru_zone_size[zid][lru]);
> + if (unlikely(abs(x) > MEMCG_CHARGE_BATCH)) {
> + atomic_long_add(x, &pn->lru_zone_size[zid][lru]);
> + x = 0;
> + }
> + __this_cpu_write(lruvec_stat_cpu->lru_zone_size[zid][lru], x);
> +}
> +
> static inline void __mod_lruvec_page_state(struct page *page,
> enum node_stat_item idx, int val)
> {
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 2bd3df3d101a..5463ad160e10 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -962,36 +962,20 @@ struct lruvec *mem_cgroup_page_lruvec(struct page *page, struct pglist_data *pgd
> * @zid: zone id of the accounted pages
> * @nr_pages: positive when adding or negative when removing
> *
> - * This function must be called under lru_lock, just before a page is added
> - * to or just after a page is removed from an lru list (that ordering being
> - * so as to allow it to check that lru_size 0 is consistent with list_empty).
> + * This function must be called just before a page is added to, or just after a
> + * page is removed from, an lru list. Callers aren't required to hold lru_lock
> + * because these statistics use per-cpu counters and atomics.
> */
> void mem_cgroup_update_lru_size(struct lruvec *lruvec, enum lru_list lru,
> int zid, int nr_pages)
> {
> struct mem_cgroup_per_node *mz;
> - unsigned long *lru_size;
> - long size;
>
> if (mem_cgroup_disabled())
> return;
>
> mz = container_of(lruvec, struct mem_cgroup_per_node, lruvec);
> - lru_size = &mz->lru_zone_size[zid][lru];
> -
> - if (nr_pages < 0)
> - *lru_size += nr_pages;
> -
> - size = *lru_size;
> - if (WARN_ONCE(size < 0,
> - "%s(%p, %d, %d): lru_size %ld\n",
> - __func__, lruvec, lru, nr_pages, size)) {
> - VM_BUG_ON(1);
> - *lru_size = 0;
> - }
> -
> - if (nr_pages > 0)
> - *lru_size += nr_pages;
> + __mod_lru_zone_size(mz, lru, zid, nr_pages);
> }
>
> bool task_in_mem_cgroup(struct task_struct *task, struct mem_cgroup *memcg)
> @@ -1833,9 +1817,10 @@ static int memcg_hotplug_cpu_dead(unsigned int cpu)
> struct mem_cgroup_per_node *pn;
>
> pn = mem_cgroup_nodeinfo(memcg, nid);
> - x = this_cpu_xchg(pn->lruvec_stat_cpu->count[i], 0);
> + x = this_cpu_xchg(pn->lruvec_stat_cpu->node[i],
> + 0);
> if (x)
> - atomic_long_add(x, &pn->lruvec_stat[i]);
> + atomic_long_add(x, &pn->node_stat[i]);
> }
> }
>
next prev parent reply other threads:[~2018-09-11 16:32 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-11 0:42 [RFC PATCH v2 0/8] lru_lock scalability and SMP list functions Daniel Jordan
2018-09-11 0:42 ` [RFC PATCH v2 1/8] mm, memcontrol.c: make memcg lru stats thread-safe without lru_lock Daniel Jordan
2018-09-11 16:32 ` Laurent Dufour [this message]
2018-09-12 13:28 ` Daniel Jordan
2018-09-11 0:42 ` [RFC PATCH v2 2/8] mm: make zone_reclaim_stat updates thread-safe Daniel Jordan
2018-09-11 16:40 ` Laurent Dufour
2018-09-12 13:30 ` Daniel Jordan
2018-09-11 0:42 ` [RFC PATCH v2 3/8] mm: convert lru_lock from a spinlock_t to a rwlock_t Daniel Jordan
2018-09-11 0:59 ` [RFC PATCH v2 4/8] mm: introduce smp_list_del for concurrent list entry removals Daniel Jordan
2018-09-11 0:59 ` [RFC PATCH v2 5/8] mm: enable concurrent LRU removals Daniel Jordan
2018-09-11 0:59 ` [RFC PATCH v2 6/8] mm: splice local lists onto the front of the LRU Daniel Jordan
2018-09-11 0:59 ` [RFC PATCH v2 7/8] mm: introduce smp_list_splice to prepare for concurrent LRU adds Daniel Jordan
2018-09-11 0:59 ` [RFC PATCH v2 8/8] mm: enable " Daniel Jordan
2018-10-19 11:35 ` [RFC PATCH v2 0/8] lru_lock scalability and SMP list functions Vlastimil Babka
2018-10-19 15:35 ` Daniel Jordan
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=e62ef1a0-9518-5a16-df5b-86977b4e8881@linux.vnet.ibm.com \
--to=ldufour@linux.vnet.ibm.com \
--cc=Pavel.Tatashin@microsoft.com \
--cc=aaron.lu@intel.com \
--cc=ak@linux.intel.com \
--cc=akpm@linux-foundation.org \
--cc=cgroups@vger.kernel.org \
--cc=daniel.m.jordan@oracle.com \
--cc=dave.dice@oracle.com \
--cc=dave.hansen@linux.intel.com \
--cc=hannes@cmpxchg.org \
--cc=levyossi@icloud.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mgorman@techsingularity.net \
--cc=mhocko@kernel.org \
--cc=steven.sistare@oracle.com \
--cc=tim.c.chen@intel.com \
--cc=vdavydov.dev@gmail.com \
--cc=ying.huang@intel.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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).