[PATCH 0/3] mm: use irq locking suffix instead local_irq_disable()

[PATCH 0/3] mm: use irq locking suffix instead local_irq_disable()

[Sebastian Andrzej Siewior]

small series which avoids using local_irq_disable()/local_irq_enable()
but instead does spin_lock_irq()/spin_unlock_irq() so it is within the
context of the lock which it belongs to.
Patch #1 is a cleanup where local_irq_.*() remained after the lock was
removed.

Sebastian

[PATCH 1/3] mm: workingset: remove local_irq_disable() from count_shadow_nodes()

[Sebastian Andrzej Siewior]

In commit 0c7c1bed7e13 ("mm: make counting of list_lru_one::nr_items
lockless") the
	spin_lock(&nlru->lock);

statement was replaced with
	rcu_read_lock();

in __list_lru_count_one(). The comment in count_shadow_nodes() says that
the local_irq_disable() is required because the lock must be acquired
with disabled interrupts and (spin_lock()) does not do so.
Since the lock is replaced with rcu_read_lock() the local_irq_disable()
is no longer needed. The code path is
  list_lru_shrink_count()
    -> list_lru_count_one()
      -> __list_lru_count_one()
        -> rcu_read_lock()
        -> list_lru_from_memcg_idx()
        -> rcu_read_unlock()

Remove the local_irq_disable() statement.

Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 mm/workingset.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/mm/workingset.c b/mm/workingset.c
index 40ee02c83978..ed8151180899 100644
--- a/mm/workingset.c
+++ b/mm/workingset.c
@@ -366,10 +366,7 @@ static unsigned long count_shadow_nodes(struct shrinker *shrinker,
 	unsigned long nodes;
 	unsigned long cache;
 
-	/* list_lru lock nests inside the IRQ-safe i_pages lock */
-	local_irq_disable();
 	nodes = list_lru_shrink_count(&shadow_nodes, sc);
-	local_irq_enable();
 
 	/*
 	 * Approximate a reasonable limit for the radix tree nodes
-- 
2.18.0

[PATCH 2/3] mm: workingset: make shadow_lru_isolate() use locking suffix

[Sebastian Andrzej Siewior]

shadow_lru_isolate() disables interrupts and acquires a lock. It could
use spin_lock_irq() instead. It also uses local_irq_enable() while it
could use spin_unlock_irq()/xa_unlock_irq().

Use proper suffix for lock/unlock in order to enable/disable interrupts
during release/acquire of a lock.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 mm/workingset.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/mm/workingset.c b/mm/workingset.c
index ed8151180899..529480c21f93 100644
--- a/mm/workingset.c
+++ b/mm/workingset.c
@@ -431,7 +431,7 @@ static enum lru_status shadow_lru_isolate(struct list_head *item,
 
 	/* Coming from the list, invert the lock order */
 	if (!xa_trylock(&mapping->i_pages)) {
-		spin_unlock(lru_lock);
+		spin_unlock_irq(lru_lock);
 		ret = LRU_RETRY;
 		goto out;
 	}
@@ -469,13 +469,11 @@ static enum lru_status shadow_lru_isolate(struct list_head *item,
 				 workingset_lookup_update(mapping));
 
 out_invalid:
-	xa_unlock(&mapping->i_pages);
+	xa_unlock_irq(&mapping->i_pages);
 	ret = LRU_REMOVED_RETRY;
 out:
-	local_irq_enable();
 	cond_resched();
-	local_irq_disable();
-	spin_lock(lru_lock);
+	spin_lock_irq(lru_lock);
 	return ret;
 }
 
-- 
2.18.0

[PATCH 3/3] mm: list_lru: Add lock_irq member to __list_lru_init()

[Sebastian Andrzej Siewior]

scan_shadow_nodes() is the only user of __list_lru_walk_one() which
disables interrupts before invoking it. The reason is that nlru->lock is
nesting inside IRQ-safe i_pages lock. Some functions unconditionally
acquire the lock with the _irq() suffix.

__list_lru_walk_one() can't acquire the lock unconditionally with _irq()
suffix because it might invoke a callback which unlocks the nlru->lock
and invokes a sleeping function without enabling interrupts.

Add an argument to __list_lru_init() which identifies wheather the
nlru->lock needs to be acquired with disabling interrupts or without.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/list_lru.h | 12 ++++++++----
 mm/list_lru.c            | 14 ++++++++++----
 mm/workingset.c          | 12 ++++--------
 3 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/include/linux/list_lru.h b/include/linux/list_lru.h
index 96def9d15b1b..c2161c3a1809 100644
--- a/include/linux/list_lru.h
+++ b/include/linux/list_lru.h
@@ -51,18 +51,22 @@ struct list_lru_node {
 
 struct list_lru {
 	struct list_lru_node	*node;
+	bool			lock_irq;
 #if defined(CONFIG_MEMCG) && !defined(CONFIG_SLOB)
 	struct list_head	list;
 #endif
 };
 
 void list_lru_destroy(struct list_lru *lru);
-int __list_lru_init(struct list_lru *lru, bool memcg_aware,
+int __list_lru_init(struct list_lru *lru, bool memcg_aware, bool lock_irq,
 		    struct lock_class_key *key);
 
-#define list_lru_init(lru)		__list_lru_init((lru), false, NULL)
-#define list_lru_init_key(lru, key)	__list_lru_init((lru), false, (key))
-#define list_lru_init_memcg(lru)	__list_lru_init((lru), true, NULL)
+#define list_lru_init(lru)		__list_lru_init((lru), false, false, \
+							NULL)
+#define list_lru_init_key(lru, key)	__list_lru_init((lru), false, false, \
+							(key))
+#define list_lru_init_memcg(lru)	__list_lru_init((lru), true, false, \
+							NULL)
 
 int memcg_update_all_list_lrus(int num_memcgs);
 void memcg_drain_all_list_lrus(int src_idx, int dst_idx);
diff --git a/mm/list_lru.c b/mm/list_lru.c
index fcfb6c89ed47..1c49d48078e4 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -204,7 +204,10 @@ __list_lru_walk_one(struct list_lru *lru, int nid, int memcg_idx,
 	struct list_head *item, *n;
 	unsigned long isolated = 0;
 
-	spin_lock(&nlru->lock);
+	if (lru->lock_irq)
+		spin_lock_irq(&nlru->lock);
+	else
+		spin_lock(&nlru->lock);
 	l = list_lru_from_memcg_idx(nlru, memcg_idx);
 restart:
 	list_for_each_safe(item, n, &l->list) {
@@ -251,7 +254,10 @@ __list_lru_walk_one(struct list_lru *lru, int nid, int memcg_idx,
 		}
 	}
 
-	spin_unlock(&nlru->lock);
+	if (lru->lock_irq)
+		spin_unlock_irq(&nlru->lock);
+	else
+		spin_unlock(&nlru->lock);
 	return isolated;
 }
 
@@ -553,7 +559,7 @@ static void memcg_destroy_list_lru(struct list_lru *lru)
 }
 #endif /* CONFIG_MEMCG && !CONFIG_SLOB */
 
-int __list_lru_init(struct list_lru *lru, bool memcg_aware,
+int __list_lru_init(struct list_lru *lru, bool memcg_aware, bool lock_irq,
 		    struct lock_class_key *key)
 {
 	int i;
@@ -580,7 +586,7 @@ int __list_lru_init(struct list_lru *lru, bool memcg_aware,
 		lru->node = NULL;
 		goto out;
 	}
-
+	lru->lock_irq = lock_irq;
 	list_lru_register(lru);
 out:
 	memcg_put_cache_ids();
diff --git a/mm/workingset.c b/mm/workingset.c
index 529480c21f93..23ce00f48212 100644
--- a/mm/workingset.c
+++ b/mm/workingset.c
@@ -480,13 +480,8 @@ static enum lru_status shadow_lru_isolate(struct list_head *item,
 static unsigned long scan_shadow_nodes(struct shrinker *shrinker,
 				       struct shrink_control *sc)
 {
-	unsigned long ret;
-
-	/* list_lru lock nests inside the IRQ-safe i_pages lock */
-	local_irq_disable();
-	ret = list_lru_shrink_walk(&shadow_nodes, sc, shadow_lru_isolate, NULL);
-	local_irq_enable();
-	return ret;
+	return list_lru_shrink_walk(&shadow_nodes, sc, shadow_lru_isolate,
+				    NULL);
 }
 
 static struct shrinker workingset_shadow_shrinker = {
@@ -523,7 +518,8 @@ static int __init workingset_init(void)
 	pr_info("workingset: timestamp_bits=%d max_order=%d bucket_order=%u\n",
 	       timestamp_bits, max_order, bucket_order);
 
-	ret = __list_lru_init(&shadow_nodes, true, &shadow_nodes_key);
+	/* list_lru lock nests inside the IRQ-safe i_pages lock */
+	ret = __list_lru_init(&shadow_nodes, true, true, &shadow_nodes_key);
 	if (ret)
 		goto err;
 	ret = register_shrinker(&workingset_shadow_shrinker);
-- 
2.18.0

Re: [PATCH 0/3] mm: use irq locking suffix instead local_irq_disable()

[Andrew Morton]

On Fri, 22 Jun 2018 17:12:18 +0200 Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> small series which avoids using local_irq_disable()/local_irq_enable()
> but instead does spin_lock_irq()/spin_unlock_irq() so it is within the
> context of the lock which it belongs to.
> Patch #1 is a cleanup where local_irq_.*() remained after the lock was
> removed.

Looks OK.

And we may as well do this...

From: Andrew Morton <akpm@linux-foundation.org>
Subject: mm/list_lru.c: fold __list_lru_count_one() into its caller

__list_lru_count_one() has a single callsite.

Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/list_lru.c |   12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff -puN mm/list_lru.c~mm-list_lruc-fold-__list_lru_count_one-into-its-caller mm/list_lru.c
--- a/mm/list_lru.c~mm-list_lruc-fold-__list_lru_count_one-into-its-caller
+++ a/mm/list_lru.c
@@ -162,26 +162,20 @@ void list_lru_isolate_move(struct list_l
 }
 EXPORT_SYMBOL_GPL(list_lru_isolate_move);
 
-static unsigned long __list_lru_count_one(struct list_lru *lru,
-					  int nid, int memcg_idx)
+unsigned long list_lru_count_one(struct list_lru *lru,
+				 int nid, struct mem_cgroup *memcg)
 {
 	struct list_lru_node *nlru = &lru->node[nid];
 	struct list_lru_one *l;
 	unsigned long count;
 
 	rcu_read_lock();
-	l = list_lru_from_memcg_idx(nlru, memcg_idx);
+	l = list_lru_from_memcg_idx(nlru, memcg_cache_id(memcg));
 	count = l->nr_items;
 	rcu_read_unlock();
 
 	return count;
 }
-
-unsigned long list_lru_count_one(struct list_lru *lru,
-				 int nid, struct mem_cgroup *memcg)
-{
-	return __list_lru_count_one(lru, nid, memcg_cache_id(memcg));
-}
 EXPORT_SYMBOL_GPL(list_lru_count_one);
 
 unsigned long list_lru_count_node(struct list_lru *lru, int nid)
_

Re: [PATCH 1/3] mm: workingset: remove local_irq_disable() from count_shadow_nodes()

[Vladimir Davydov]

On Fri, Jun 22, 2018 at 05:12:19PM +0200, Sebastian Andrzej Siewior wrote:
> In commit 0c7c1bed7e13 ("mm: make counting of list_lru_one::nr_items
> lockless") the
> 	spin_lock(&nlru->lock);
> 
> statement was replaced with
> 	rcu_read_lock();
> 
> in __list_lru_count_one(). The comment in count_shadow_nodes() says that
> the local_irq_disable() is required because the lock must be acquired
> with disabled interrupts and (spin_lock()) does not do so.
> Since the lock is replaced with rcu_read_lock() the local_irq_disable()
> is no longer needed. The code path is
>   list_lru_shrink_count()
>     -> list_lru_count_one()
>       -> __list_lru_count_one()
>         -> rcu_read_lock()
>         -> list_lru_from_memcg_idx()
>         -> rcu_read_unlock()
> 
> Remove the local_irq_disable() statement.
> 
> Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Acked-by: Vladimir Davydov <vdavydov.dev@gmail.com>

Re: [PATCH 2/3] mm: workingset: make shadow_lru_isolate() use locking suffix

[Vladimir Davydov]

On Fri, Jun 22, 2018 at 05:12:20PM +0200, Sebastian Andrzej Siewior wrote:
> shadow_lru_isolate() disables interrupts and acquires a lock. It could
> use spin_lock_irq() instead. It also uses local_irq_enable() while it
> could use spin_unlock_irq()/xa_unlock_irq().
> 
> Use proper suffix for lock/unlock in order to enable/disable interrupts
> during release/acquire of a lock.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

I don't like when a spin lock is locked with local_irq_disabled +
spin_lock and unlocked with spin_unlock_irq - it looks asymmetric.
IMHO the code is pretty easy to follow as it is - local_irq_disable in
scan_shadow_nodes matches local_irq_enable in shadow_lru_isolate.

> ---
>  mm/workingset.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/workingset.c b/mm/workingset.c
> index ed8151180899..529480c21f93 100644
> --- a/mm/workingset.c
> +++ b/mm/workingset.c
> @@ -431,7 +431,7 @@ static enum lru_status shadow_lru_isolate(struct list_head *item,
>  
>  	/* Coming from the list, invert the lock order */
>  	if (!xa_trylock(&mapping->i_pages)) {
> -		spin_unlock(lru_lock);
> +		spin_unlock_irq(lru_lock);
>  		ret = LRU_RETRY;
>  		goto out;
>  	}
> @@ -469,13 +469,11 @@ static enum lru_status shadow_lru_isolate(struct list_head *item,
>  				 workingset_lookup_update(mapping));
>  
>  out_invalid:
> -	xa_unlock(&mapping->i_pages);
> +	xa_unlock_irq(&mapping->i_pages);
>  	ret = LRU_REMOVED_RETRY;
>  out:
> -	local_irq_enable();
>  	cond_resched();
> -	local_irq_disable();
> -	spin_lock(lru_lock);
> +	spin_lock_irq(lru_lock);
>  	return ret;
>  }

Re: [PATCH 3/3] mm: list_lru: Add lock_irq member to __list_lru_init()

[Vladimir Davydov]

On Fri, Jun 22, 2018 at 05:12:21PM +0200, Sebastian Andrzej Siewior wrote:
> scan_shadow_nodes() is the only user of __list_lru_walk_one() which
> disables interrupts before invoking it. The reason is that nlru->lock is
> nesting inside IRQ-safe i_pages lock. Some functions unconditionally
> acquire the lock with the _irq() suffix.
> 
> __list_lru_walk_one() can't acquire the lock unconditionally with _irq()
> suffix because it might invoke a callback which unlocks the nlru->lock
> and invokes a sleeping function without enabling interrupts.
> 
> Add an argument to __list_lru_init() which identifies wheather the
> nlru->lock needs to be acquired with disabling interrupts or without.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  include/linux/list_lru.h | 12 ++++++++----
>  mm/list_lru.c            | 14 ++++++++++----
>  mm/workingset.c          | 12 ++++--------
>  3 files changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/include/linux/list_lru.h b/include/linux/list_lru.h
> index 96def9d15b1b..c2161c3a1809 100644
> --- a/include/linux/list_lru.h
> +++ b/include/linux/list_lru.h
> @@ -51,18 +51,22 @@ struct list_lru_node {
>  
>  struct list_lru {
>  	struct list_lru_node	*node;
> +	bool			lock_irq;
>  #if defined(CONFIG_MEMCG) && !defined(CONFIG_SLOB)
>  	struct list_head	list;
>  #endif
>  };

TBO I don't like this patch, because the new member of struct list_lru,
lock_irq, has rather obscure meaning IMHO: it makes list_lru_walk
disable irq before taking lru_lock, but at the same time list_lru_add
and list_lru_del never do that, no matter whether lock_irq is true or
false. That is, if a user of struct list_lru sets this flag, he's
supposed to disable irq for list_lru_add/del by himself (mm/workingset
does that). IMHO the code of mm/workingset is clear as it is. Since it
is the only place where this flag is used, I'd rather leave it as is.

>  
>  void list_lru_destroy(struct list_lru *lru);
> -int __list_lru_init(struct list_lru *lru, bool memcg_aware,
> +int __list_lru_init(struct list_lru *lru, bool memcg_aware, bool lock_irq,
>  		    struct lock_class_key *key);
>  
> -#define list_lru_init(lru)		__list_lru_init((lru), false, NULL)
> -#define list_lru_init_key(lru, key)	__list_lru_init((lru), false, (key))
> -#define list_lru_init_memcg(lru)	__list_lru_init((lru), true, NULL)
> +#define list_lru_init(lru)		__list_lru_init((lru), false, false, \
> +							NULL)
> +#define list_lru_init_key(lru, key)	__list_lru_init((lru), false, false, \
> +							(key))
> +#define list_lru_init_memcg(lru)	__list_lru_init((lru), true, false, \
> +							NULL)
>  
>  int memcg_update_all_list_lrus(int num_memcgs);
>  void memcg_drain_all_list_lrus(int src_idx, int dst_idx);
> diff --git a/mm/list_lru.c b/mm/list_lru.c
> index fcfb6c89ed47..1c49d48078e4 100644
> --- a/mm/list_lru.c
> +++ b/mm/list_lru.c
> @@ -204,7 +204,10 @@ __list_lru_walk_one(struct list_lru *lru, int nid, int memcg_idx,
>  	struct list_head *item, *n;
>  	unsigned long isolated = 0;
>  
> -	spin_lock(&nlru->lock);
> +	if (lru->lock_irq)
> +		spin_lock_irq(&nlru->lock);
> +	else
> +		spin_lock(&nlru->lock);
>  	l = list_lru_from_memcg_idx(nlru, memcg_idx);
>  restart:
>  	list_for_each_safe(item, n, &l->list) {
> @@ -251,7 +254,10 @@ __list_lru_walk_one(struct list_lru *lru, int nid, int memcg_idx,
>  		}
>  	}
>  
> -	spin_unlock(&nlru->lock);
> +	if (lru->lock_irq)
> +		spin_unlock_irq(&nlru->lock);
> +	else
> +		spin_unlock(&nlru->lock);
>  	return isolated;
>  }
>  
> @@ -553,7 +559,7 @@ static void memcg_destroy_list_lru(struct list_lru *lru)
>  }
>  #endif /* CONFIG_MEMCG && !CONFIG_SLOB */
>  
> -int __list_lru_init(struct list_lru *lru, bool memcg_aware,
> +int __list_lru_init(struct list_lru *lru, bool memcg_aware, bool lock_irq,
>  		    struct lock_class_key *key)
>  {
>  	int i;
> @@ -580,7 +586,7 @@ int __list_lru_init(struct list_lru *lru, bool memcg_aware,
>  		lru->node = NULL;
>  		goto out;
>  	}
> -
> +	lru->lock_irq = lock_irq;
>  	list_lru_register(lru);
>  out:
>  	memcg_put_cache_ids();
> diff --git a/mm/workingset.c b/mm/workingset.c
> index 529480c21f93..23ce00f48212 100644
> --- a/mm/workingset.c
> +++ b/mm/workingset.c
> @@ -480,13 +480,8 @@ static enum lru_status shadow_lru_isolate(struct list_head *item,
>  static unsigned long scan_shadow_nodes(struct shrinker *shrinker,
>  				       struct shrink_control *sc)
>  {
> -	unsigned long ret;
> -
> -	/* list_lru lock nests inside the IRQ-safe i_pages lock */
> -	local_irq_disable();
> -	ret = list_lru_shrink_walk(&shadow_nodes, sc, shadow_lru_isolate, NULL);
> -	local_irq_enable();
> -	return ret;
> +	return list_lru_shrink_walk(&shadow_nodes, sc, shadow_lru_isolate,
> +				    NULL);
>  }
>  
>  static struct shrinker workingset_shadow_shrinker = {
> @@ -523,7 +518,8 @@ static int __init workingset_init(void)
>  	pr_info("workingset: timestamp_bits=%d max_order=%d bucket_order=%u\n",
>  	       timestamp_bits, max_order, bucket_order);
>  
> -	ret = __list_lru_init(&shadow_nodes, true, &shadow_nodes_key);
> +	/* list_lru lock nests inside the IRQ-safe i_pages lock */
> +	ret = __list_lru_init(&shadow_nodes, true, true, &shadow_nodes_key);
>  	if (ret)
>  		goto err;
>  	ret = register_shrinker(&workingset_shadow_shrinker);

Re: [PATCH 0/3] mm: use irq locking suffix instead local_irq_disable()

[Vladimir Davydov]

On Fri, Jun 22, 2018 at 02:39:00PM -0700, Andrew Morton wrote:
> From: Andrew Morton <akpm@linux-foundation.org>
> Subject: mm/list_lru.c: fold __list_lru_count_one() into its caller
> 
> __list_lru_count_one() has a single callsite.
> 
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
> Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

Acked-by: Vladimir Davydov <vdavydov.dev@gmail.com>

Re: [PATCH 1/3] mm: workingset: remove local_irq_disable() from count_shadow_nodes()

[Kirill Tkhai]

On 22.06.2018 18:12, Sebastian Andrzej Siewior wrote:
> In commit 0c7c1bed7e13 ("mm: make counting of list_lru_one::nr_items
> lockless") the
> 	spin_lock(&nlru->lock);
> 
> statement was replaced with
> 	rcu_read_lock();
> 
> in __list_lru_count_one(). The comment in count_shadow_nodes() says that
> the local_irq_disable() is required because the lock must be acquired
> with disabled interrupts and (spin_lock()) does not do so.
> Since the lock is replaced with rcu_read_lock() the local_irq_disable()
> is no longer needed. The code path is
>   list_lru_shrink_count()
>     -> list_lru_count_one()
>       -> __list_lru_count_one()
>         -> rcu_read_lock()
>         -> list_lru_from_memcg_idx()
>         -> rcu_read_unlock()
> 
> Remove the local_irq_disable() statement.
> 
> Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Looks good for me.

Reviewed-by: Kirill Tkhai <ktkhai@virtuozzo.com>

> ---
>  mm/workingset.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/mm/workingset.c b/mm/workingset.c
> index 40ee02c83978..ed8151180899 100644
> --- a/mm/workingset.c
> +++ b/mm/workingset.c
> @@ -366,10 +366,7 @@ static unsigned long count_shadow_nodes(struct shrinker *shrinker,
>  	unsigned long nodes;
>  	unsigned long cache;
>  
> -	/* list_lru lock nests inside the IRQ-safe i_pages lock */
> -	local_irq_disable();
>  	nodes = list_lru_shrink_count(&shadow_nodes, sc);
> -	local_irq_enable();
>  
>  	/*
>  	 * Approximate a reasonable limit for the radix tree nodes
> 

Re: [PATCH 2/3] mm: workingset: make shadow_lru_isolate() use locking suffix

[Sebastian Andrzej Siewior]

On 2018-06-24 22:57:53 [+0300], Vladimir Davydov wrote:
> On Fri, Jun 22, 2018 at 05:12:20PM +0200, Sebastian Andrzej Siewior wrote:
> > shadow_lru_isolate() disables interrupts and acquires a lock. It could
> > use spin_lock_irq() instead. It also uses local_irq_enable() while it
> > could use spin_unlock_irq()/xa_unlock_irq().
> > 
> > Use proper suffix for lock/unlock in order to enable/disable interrupts
> > during release/acquire of a lock.
> > 
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> I don't like when a spin lock is locked with local_irq_disabled +
> spin_lock and unlocked with spin_unlock_irq - it looks asymmetric.
> IMHO the code is pretty easy to follow as it is - local_irq_disable in
> scan_shadow_nodes matches local_irq_enable in shadow_lru_isolate.

it is not asymmetric because a later patch makes it use spin_lock_irq(),
too. If you use local_irq_disable() and a spin_lock() (like you suggest
in 3/3 as well) then you separate the locking instruction. It works as
expected on vanilla but break other locking implementations like those
on RT. Also if the locking changes then the local_irq_disable() part
will be forgotten like you saw in 1/3 of this series.

Sebastian

Re: [PATCH 2/3] mm: workingset: make shadow_lru_isolate() use locking suffix

[Vladimir Davydov]

On Tue, Jun 26, 2018 at 11:25:34PM +0200, Sebastian Andrzej Siewior wrote:
> On 2018-06-24 22:57:53 [+0300], Vladimir Davydov wrote:
> > On Fri, Jun 22, 2018 at 05:12:20PM +0200, Sebastian Andrzej Siewior wrote:
> > > shadow_lru_isolate() disables interrupts and acquires a lock. It could
> > > use spin_lock_irq() instead. It also uses local_irq_enable() while it
> > > could use spin_unlock_irq()/xa_unlock_irq().
> > > 
> > > Use proper suffix for lock/unlock in order to enable/disable interrupts
> > > during release/acquire of a lock.
> > > 
> > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > 
> > I don't like when a spin lock is locked with local_irq_disabled +
> > spin_lock and unlocked with spin_unlock_irq - it looks asymmetric.
> > IMHO the code is pretty easy to follow as it is - local_irq_disable in
> > scan_shadow_nodes matches local_irq_enable in shadow_lru_isolate.
> 
> it is not asymmetric because a later patch makes it use
> spin_lock_irq(), too. If you use local_irq_disable() and a spin_lock()
> (like you suggest in 3/3 as well) then you separate the locking
> instruction. It works as expected on vanilla but break other locking
> implementations like those on RT.

As I said earlier, I don't like patch 3 either, because I find the
notion of list_lru::lock_irq flag abstruse since it doesn't make all
code paths taking the lock disable irq: list_lru_add/del use spin_lock
no matter whether the flag is set or not. That is, when you initialize a
list_lru and pass lock_irq=true, you'll have to keep in mind that it
only protects list_lru_walk, while list_lru_add/del must be called with
irq disabled by the caller. Disabling irq before list_lru_walk
explicitly looks much more straightforward IMO.

As for RT, it wouldn't need mm/workingset altogether AFAIU. Anyway, it's
rather unusual to care about out-of-the-tree patches when changing the
vanilla kernel code IMO. Using local_irq_disable + spin_lock instead of
spin_lock_irq is a typical pattern, and I don't see how changing this
particular place would help RT.

> Also if the locking changes then the local_irq_disable() part will be
> forgotten like you saw in 1/3 of this series.

If the locking changes, we'll have to revise all list_lru users anyway.
Yeah, we missed it last time, but it didn't break anything, and it was
finally found and fixed (by you, thanks BTW).

Re: [PATCH 2/3] mm: workingset: make shadow_lru_isolate() use locking suffix

[Sebastian Andrzej Siewior]

On 2018-06-27 11:50:03 [+0300], Vladimir Davydov wrote:
> > it is not asymmetric because a later patch makes it use
> > spin_lock_irq(), too. If you use local_irq_disable() and a spin_lock()
> > (like you suggest in 3/3 as well) then you separate the locking
> > instruction. It works as expected on vanilla but break other locking
> > implementations like those on RT.
> 
> As I said earlier, I don't like patch 3 either, because I find the
> notion of list_lru::lock_irq flag abstruse since it doesn't make all
> code paths taking the lock disable irq: list_lru_add/del use spin_lock
> no matter whether the flag is set or not. That is, when you initialize a
> list_lru and pass lock_irq=true, you'll have to keep in mind that it
> only protects list_lru_walk, while list_lru_add/del must be called with
> irq disabled by the caller. Disabling irq before list_lru_walk
> explicitly looks much more straightforward IMO.

It helps to keep the locking annotation in one place. If it helps I
could add the _irqsave() suffix to list_lru_add/del like it is already
done in other places (in this file).

> As for RT, it wouldn't need mm/workingset altogether AFAIU. 
Why wouldn't it need it?

> Anyway, it's
> rather unusual to care about out-of-the-tree patches when changing the
> vanilla kernel code IMO. 
The plan is not stay out-of-tree forever. And I don't intend to make
impossible or hard to argue changes just for RT's sake. This is only to
keep the correct locking context/primitives in one place and not
scattered around.
The only reason for the separation is that most users don't disable
interrupts (one user does) and there a few places which already use
irqsave() because they can be called from both places. This
list_lru_walk() is the only which can't do so due to the callback it
invokes. I could also add a different function (say
list_lru_walk_one_irq()) which behaves like list_lru_walk_one() but does
spin_lock_irq() instead.

> Using local_irq_disable + spin_lock instead of
> spin_lock_irq is a typical pattern, and I don't see how changing this
> particular place would help RT.
It is not that typical. It is how the locking primitives work, yes, but
they are not so many places that do so and those that did got cleaned
up.

> > Also if the locking changes then the local_irq_disable() part will be
> > forgotten like you saw in 1/3 of this series.
> 
> If the locking changes, we'll have to revise all list_lru users anyway.
> Yeah, we missed it last time, but it didn't break anything, and it was
> finally found and fixed (by you, thanks BTW).
You are very welcome. But having the locking primitives in one place you
would have less things to worry about.

Sebastian

Re: [PATCH 2/3] mm: workingset: make shadow_lru_isolate() use locking suffix

[Vladimir Davydov]

On Wed, Jun 27, 2018 at 11:20:59AM +0200, Sebastian Andrzej Siewior wrote:
> On 2018-06-27 11:50:03 [+0300], Vladimir Davydov wrote:
> > > it is not asymmetric because a later patch makes it use
> > > spin_lock_irq(), too. If you use local_irq_disable() and a spin_lock()
> > > (like you suggest in 3/3 as well) then you separate the locking
> > > instruction. It works as expected on vanilla but break other locking
> > > implementations like those on RT.
> > 
> > As I said earlier, I don't like patch 3 either, because I find the
> > notion of list_lru::lock_irq flag abstruse since it doesn't make all
> > code paths taking the lock disable irq: list_lru_add/del use spin_lock
> > no matter whether the flag is set or not. That is, when you initialize a
> > list_lru and pass lock_irq=true, you'll have to keep in mind that it
> > only protects list_lru_walk, while list_lru_add/del must be called with
> > irq disabled by the caller. Disabling irq before list_lru_walk
> > explicitly looks much more straightforward IMO.
> 
> It helps to keep the locking annotation in one place. If it helps I
> could add the _irqsave() suffix to list_lru_add/del like it is already
> done in other places (in this file).

AFAIK local_irqsave/restore don't come for free so using them just to
keep the code clean doesn't seem to be reasonable.

> 
> > As for RT, it wouldn't need mm/workingset altogether AFAIU. 
> Why wouldn't it need it?

I may be wrong, but AFAIU RT kernel doesn't do swapping.

> 
> > Anyway, it's
> > rather unusual to care about out-of-the-tree patches when changing the
> > vanilla kernel code IMO. 
> The plan is not stay out-of-tree forever. And I don't intend to make
> impossible or hard to argue changes just for RT's sake. This is only to
> keep the correct locking context/primitives in one place and not
> scattered around.
> The only reason for the separation is that most users don't disable
> interrupts (one user does) and there a few places which already use
> irqsave() because they can be called from both places. This
> list_lru_walk() is the only which can't do so due to the callback it
> invokes. I could also add a different function (say
> list_lru_walk_one_irq()) which behaves like list_lru_walk_one() but does
> spin_lock_irq() instead.

That would look better IMHO. I mean, passing the flag as an argument to
__list_lru_walk_one and introducing list_lru_shrink_walk_irq.

> 
> > Using local_irq_disable + spin_lock instead of
> > spin_lock_irq is a typical pattern, and I don't see how changing this
> > particular place would help RT.
> It is not that typical. It is how the locking primitives work, yes, but
> they are not so many places that do so and those that did got cleaned
> up.

Missed that. There used to be a lot of places like that in the past.
I guess things have changed.

> 
> > > Also if the locking changes then the local_irq_disable() part will be
> > > forgotten like you saw in 1/3 of this series.
> > 
> > If the locking changes, we'll have to revise all list_lru users anyway.
> > Yeah, we missed it last time, but it didn't break anything, and it was
> > finally found and fixed (by you, thanks BTW).
> You are very welcome. But having the locking primitives in one place you
> would have less things to worry about.

Re: [PATCH 2/3] mm: workingset: make shadow_lru_isolate() use locking suffix

[Sebastian Andrzej Siewior]

On 2018-06-28 12:30:57 [+0300], Vladimir Davydov wrote:
> > It helps to keep the locking annotation in one place. If it helps I
> > could add the _irqsave() suffix to list_lru_add/del like it is already
> > done in other places (in this file).
> 
> AFAIK local_irqsave/restore don't come for free so using them just to
> keep the code clean doesn't seem to be reasonable.

exactly. So I kept those two as is since there is no need for it.

> > > As for RT, it wouldn't need mm/workingset altogether AFAIU. 
> > Why wouldn't it need it?
> 
> I may be wrong, but AFAIU RT kernel doesn't do swapping.

swapping the RT task out would be bad indeed. This does not stop you
from using it. You can mlock() your RT application (well should because
you don't want do remove RO-data or code from memory because it is
unchanged on disk) and everything else that is not essential (say
SCHED_OTHER) could be swapped out then if memory goes low.

> > invokes. I could also add a different function (say
> > list_lru_walk_one_irq()) which behaves like list_lru_walk_one() but does
> > spin_lock_irq() instead.
> 
> That would look better IMHO. I mean, passing the flag as an argument to
> __list_lru_walk_one and introducing list_lru_shrink_walk_irq.

You think so? So I had this earlier and decided to go with what I
posted. But hey. I will post it later as suggested here and we will see
how it goes.
I just wrote this here to let akpm know that I will do as asked here
(since he Cc: me in other thread on this topic, thank you will act).

Sebastian

(no subject)

[Sebastian Andrzej Siewior]

My intepretation of situtation is that Vladimir Davydon is fine patch #1
and #2 of the series [0] but dislikes the irq argument and struct
member. It has been suggested to use list_lru_shrink_walk_irq() instead
the approach I went on in "mm: list_lru: Add lock_irq member to
__list_lru_init()".

This series is based on the former two patches and introduces
list_lru_shrink_walk_irq() (and makes the third patch of series
obsolete).
In patch 1-3 I tried a tiny cleanup so the different locking
(spin_lock() vs spin_lock_irq()) is simply lifted to the caller of the
function.

[0] The patch
      mm: workingset: remove local_irq_disable() from count_shadow_nodes() 
   and
      mm: workingset: make shadow_lru_isolate() use locking suffix

Sebastian

[PATCH 1/4] mm/list_lru: use list_lru_walk_one() in list_lru_walk_node()

[Sebastian Andrzej Siewior]

list_lru_walk_node() invokes __list_lru_walk_one() with -1 as the
memcg_idx parameter. The same can be achieved by list_lru_walk_one() and
passing NULL as memcg argument which then gets converted into -1. This
is a preparation step when the spin_lock() function is lifted to the
caller of __list_lru_walk_one().
Invoke list_lru_walk_one() instead __list_lru_walk_one() when possible.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 mm/list_lru.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/list_lru.c b/mm/list_lru.c
index fcfb6c89ed47..ddbffbdd3d72 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -272,8 +272,8 @@ unsigned long list_lru_walk_node(struct list_lru *lru, int nid,
 	long isolated = 0;
 	int memcg_idx;
 
-	isolated += __list_lru_walk_one(lru, nid, -1, isolate, cb_arg,
-					nr_to_walk);
+	isolated += list_lru_walk_one(lru, nid, NULL, isolate, cb_arg,
+				      nr_to_walk);
 	if (*nr_to_walk > 0 && list_lru_memcg_aware(lru)) {
 		for_each_memcg_cache_index(memcg_idx) {
 			isolated += __list_lru_walk_one(lru, nid, memcg_idx,
-- 
2.18.0

[PATCH 2/4] mm/list_lru: Move locking from __list_lru_walk_one() to its caller

[Sebastian Andrzej Siewior]

Move the locking inside __list_lru_walk_one() to its caller. This is a
preparation step in order to introduce list_lru_walk_one_irq() which
does spin_lock_irq() instead of spin_lock() for the locking.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 mm/list_lru.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/mm/list_lru.c b/mm/list_lru.c
index ddbffbdd3d72..819e0595303e 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -204,7 +204,6 @@ __list_lru_walk_one(struct list_lru *lru, int nid, int memcg_idx,
 	struct list_head *item, *n;
 	unsigned long isolated = 0;
 
-	spin_lock(&nlru->lock);
 	l = list_lru_from_memcg_idx(nlru, memcg_idx);
 restart:
 	list_for_each_safe(item, n, &l->list) {
@@ -250,8 +249,6 @@ __list_lru_walk_one(struct list_lru *lru, int nid, int memcg_idx,
 			BUG();
 		}
 	}
-
-	spin_unlock(&nlru->lock);
 	return isolated;
 }
 
@@ -260,8 +257,14 @@ list_lru_walk_one(struct list_lru *lru, int nid, struct mem_cgroup *memcg,
 		  list_lru_walk_cb isolate, void *cb_arg,
 		  unsigned long *nr_to_walk)
 {
-	return __list_lru_walk_one(lru, nid, memcg_cache_id(memcg),
-				   isolate, cb_arg, nr_to_walk);
+	struct list_lru_node *nlru = &lru->node[nid];
+	unsigned long ret;
+
+	spin_lock(&nlru->lock);
+	ret = __list_lru_walk_one(lru, nid, memcg_cache_id(memcg),
+				  isolate, cb_arg, nr_to_walk);
+	spin_unlock(&nlru->lock);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(list_lru_walk_one);
 
@@ -276,8 +279,13 @@ unsigned long list_lru_walk_node(struct list_lru *lru, int nid,
 				      nr_to_walk);
 	if (*nr_to_walk > 0 && list_lru_memcg_aware(lru)) {
 		for_each_memcg_cache_index(memcg_idx) {
+			struct list_lru_node *nlru = &lru->node[nid];
+
+			spin_lock(&nlru->lock);
 			isolated += __list_lru_walk_one(lru, nid, memcg_idx,
 						isolate, cb_arg, nr_to_walk);
+			spin_unlock(&nlru->lock);
+
 			if (*nr_to_walk <= 0)
 				break;
 		}
-- 
2.18.0

[PATCH 3/4] mm/list_lru: Pass struct list_lru_node as an argument __list_lru_walk_one()

[Sebastian Andrzej Siewior]

__list_lru_walk_one() is invoked with struct list_lru *lru, int nid as
the first two argument. Those two are only used to retrieve struct
list_lru_node. Since this is already done by the caller of the function
for the locking, we can pass struct list_lru_node directly and avoid the
dance around it.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 mm/list_lru.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/mm/list_lru.c b/mm/list_lru.c
index 819e0595303e..4d7f981e6144 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -194,12 +194,11 @@ unsigned long list_lru_count_node(struct list_lru *lru, int nid)
 EXPORT_SYMBOL_GPL(list_lru_count_node);
 
 static unsigned long
-__list_lru_walk_one(struct list_lru *lru, int nid, int memcg_idx,
+__list_lru_walk_one(struct list_lru_node *nlru, int memcg_idx,
 		    list_lru_walk_cb isolate, void *cb_arg,
 		    unsigned long *nr_to_walk)
 {
 
-	struct list_lru_node *nlru = &lru->node[nid];
 	struct list_lru_one *l;
 	struct list_head *item, *n;
 	unsigned long isolated = 0;
@@ -261,8 +260,8 @@ list_lru_walk_one(struct list_lru *lru, int nid, struct mem_cgroup *memcg,
 	unsigned long ret;
 
 	spin_lock(&nlru->lock);
-	ret = __list_lru_walk_one(lru, nid, memcg_cache_id(memcg),
-				  isolate, cb_arg, nr_to_walk);
+	ret = __list_lru_walk_one(nlru, memcg_cache_id(memcg), isolate, cb_arg,
+				  nr_to_walk);
 	spin_unlock(&nlru->lock);
 	return ret;
 }
@@ -282,8 +281,9 @@ unsigned long list_lru_walk_node(struct list_lru *lru, int nid,
 			struct list_lru_node *nlru = &lru->node[nid];
 
 			spin_lock(&nlru->lock);
-			isolated += __list_lru_walk_one(lru, nid, memcg_idx,
-						isolate, cb_arg, nr_to_walk);
+			isolated += __list_lru_walk_one(nlru, memcg_idx,
+							isolate, cb_arg,
+							nr_to_walk);
 			spin_unlock(&nlru->lock);
 
 			if (*nr_to_walk <= 0)
-- 
2.18.0

[PATCH 4/4] mm/list_lru: Introduce list_lru_shrink_walk_irq()

[Sebastian Andrzej Siewior]

Provide list_lru_shrink_walk_irq() and let it behave like
list_lru_walk_one() except that it locks the spinlock with
spin_lock_irq(). This is used by scan_shadow_nodes() because its lock
nests within the i_pages lock which is acquired with IRQ.
This change allows to use proper locking promitives instead hand crafted
lock_irq_disable() plus spin_lock().
There is no EXPORT_SYMBOL provided because the current user is in-KERNEL
only.

Add list_lru_shrink_walk_irq() which acquires the spinlock with the
proper locking primitives.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/list_lru.h | 25 +++++++++++++++++++++++++
 mm/list_lru.c            | 15 +++++++++++++++
 mm/workingset.c          |  8 ++------
 3 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/include/linux/list_lru.h b/include/linux/list_lru.h
index 96def9d15b1b..798c41743657 100644
--- a/include/linux/list_lru.h
+++ b/include/linux/list_lru.h
@@ -162,6 +162,23 @@ unsigned long list_lru_walk_one(struct list_lru *lru,
 				int nid, struct mem_cgroup *memcg,
 				list_lru_walk_cb isolate, void *cb_arg,
 				unsigned long *nr_to_walk);
+/**
+ * list_lru_walk_one_irq: walk a list_lru, isolating and disposing freeable items.
+ * @lru: the lru pointer.
+ * @nid: the node id to scan from.
+ * @memcg: the cgroup to scan from.
+ * @isolate: callback function that is resposible for deciding what to do with
+ *  the item currently being scanned
+ * @cb_arg: opaque type that will be passed to @isolate
+ * @nr_to_walk: how many items to scan.
+ *
+ * Same as @list_lru_walk_one except that the spinlock is acquired with
+ * spin_lock_irq().
+ */
+unsigned long list_lru_walk_one_irq(struct list_lru *lru,
+				    int nid, struct mem_cgroup *memcg,
+				    list_lru_walk_cb isolate, void *cb_arg,
+				    unsigned long *nr_to_walk);
 unsigned long list_lru_walk_node(struct list_lru *lru, int nid,
 				 list_lru_walk_cb isolate, void *cb_arg,
 				 unsigned long *nr_to_walk);
@@ -174,6 +191,14 @@ list_lru_shrink_walk(struct list_lru *lru, struct shrink_control *sc,
 				 &sc->nr_to_scan);
 }
 
+static inline unsigned long
+list_lru_shrink_walk_irq(struct list_lru *lru, struct shrink_control *sc,
+			 list_lru_walk_cb isolate, void *cb_arg)
+{
+	return list_lru_walk_one_irq(lru, sc->nid, sc->memcg, isolate, cb_arg,
+				     &sc->nr_to_scan);
+}
+
 static inline unsigned long
 list_lru_walk(struct list_lru *lru, list_lru_walk_cb isolate,
 	      void *cb_arg, unsigned long nr_to_walk)
diff --git a/mm/list_lru.c b/mm/list_lru.c
index 4d7f981e6144..1bf53a08cda8 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -267,6 +267,21 @@ list_lru_walk_one(struct list_lru *lru, int nid, struct mem_cgroup *memcg,
 }
 EXPORT_SYMBOL_GPL(list_lru_walk_one);
 
+unsigned long
+list_lru_walk_one_irq(struct list_lru *lru, int nid, struct mem_cgroup *memcg,
+		      list_lru_walk_cb isolate, void *cb_arg,
+		      unsigned long *nr_to_walk)
+{
+	struct list_lru_node *nlru = &lru->node[nid];
+	unsigned long ret;
+
+	spin_lock_irq(&nlru->lock);
+	ret = __list_lru_walk_one(nlru, memcg_cache_id(memcg), isolate, cb_arg,
+				  nr_to_walk);
+	spin_unlock_irq(&nlru->lock);
+	return ret;
+}
+
 unsigned long list_lru_walk_node(struct list_lru *lru, int nid,
 				 list_lru_walk_cb isolate, void *cb_arg,
 				 unsigned long *nr_to_walk)
diff --git a/mm/workingset.c b/mm/workingset.c
index 529480c21f93..aa75c0027079 100644
--- a/mm/workingset.c
+++ b/mm/workingset.c
@@ -480,13 +480,9 @@ static enum lru_status shadow_lru_isolate(struct list_head *item,
 static unsigned long scan_shadow_nodes(struct shrinker *shrinker,
 				       struct shrink_control *sc)
 {
-	unsigned long ret;
-
 	/* list_lru lock nests inside the IRQ-safe i_pages lock */
-	local_irq_disable();
-	ret = list_lru_shrink_walk(&shadow_nodes, sc, shadow_lru_isolate, NULL);
-	local_irq_enable();
-	return ret;
+	return list_lru_shrink_walk_irq(&shadow_nodes, sc, shadow_lru_isolate,
+					NULL);
 }
 
 static struct shrinker workingset_shadow_shrinker = {
-- 
2.18.0

Re:

[Andrew Morton]

> Reply-To: "[PATCH 0/4] mm/list_lru": add.list_lru_shrink_walk_irq@mail.linuxfoundation.org.and.use.it ()

Well that's messed up.

On Tue,  3 Jul 2018 16:52:31 +0200 Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> My intepretation of situtation is that Vladimir Davydon is fine patch #1
> and #2 of the series [0] but dislikes the irq argument and struct
> member. It has been suggested to use list_lru_shrink_walk_irq() instead
> the approach I went on in "mm: list_lru: Add lock_irq member to
> __list_lru_init()".
> 
> This series is based on the former two patches and introduces
> list_lru_shrink_walk_irq() (and makes the third patch of series
> obsolete).
> In patch 1-3 I tried a tiny cleanup so the different locking
> (spin_lock() vs spin_lock_irq()) is simply lifted to the caller of the
> function.
> 
> [0] The patch
>       mm: workingset: remove local_irq_disable() from count_shadow_nodes() 
>    and
>       mm: workingset: make shadow_lru_isolate() use locking suffix
> 

This isn't a very informative [0/n] changelog.  Some overall summary of
the patchset's objective, behaviour, use cases, testing results, etc.

I'm seeing significant conflicts with Kirill's "Improve shrink_slab()
scalability (old complexity was O(n^2), new is O(n))" series, which I
merged eight milliseconds ago.  Kirill's patchset is large but fairly
straightforward so I expect it's good for 4.18.  So I suggest we leave
things a week or more then please take a look at redoing this patchset
on top of that work?  

Re:

[Sebastian Andrzej Siewior]

On 2018-07-03 14:14:29 [-0700], Andrew Morton wrote:
> 
> > Reply-To: "[PATCH 0/4] mm/list_lru": add.list_lru_shrink_walk_irq@mail.linuxfoundation.org.and.use.it ()
> 
> Well that's messed up.

indeed it is. This should get into Subject:

> On Tue,  3 Jul 2018 16:52:31 +0200 Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> 
> > My intepretation of situtation is that Vladimir Davydon is fine patch #1
> > and #2 of the series [0] but dislikes the irq argument and struct
> > member. It has been suggested to use list_lru_shrink_walk_irq() instead
> > the approach I went on in "mm: list_lru: Add lock_irq member to
> > __list_lru_init()".
> > 
> > This series is based on the former two patches and introduces
> > list_lru_shrink_walk_irq() (and makes the third patch of series
> > obsolete).
> > In patch 1-3 I tried a tiny cleanup so the different locking
> > (spin_lock() vs spin_lock_irq()) is simply lifted to the caller of the
> > function.
> > 
> > [0] The patch
> >       mm: workingset: remove local_irq_disable() from count_shadow_nodes() 
> >    and
> >       mm: workingset: make shadow_lru_isolate() use locking suffix
> > 
> 
> This isn't a very informative [0/n] changelog.  Some overall summary of
> the patchset's objective, behaviour, use cases, testing results, etc.

The patches should be threaded as a reply to 3/3 of the series so I
assumed it was enough. And while Vladimir complained about 2/3 and 3/3
the discussion went on in 2/3 where he suggested to go on with the _irq
function. And testing, well with and without RT the function was invoked
as part of swapping (allocating memory until OOM) without complains.

> I'm seeing significant conflicts with Kirill's "Improve shrink_slab()
> scalability (old complexity was O(n^2), new is O(n))" series, which I
> merged eight milliseconds ago.  Kirill's patchset is large but fairly
> straightforward so I expect it's good for 4.18.  So I suggest we leave
> things a week or more then please take a look at redoing this patchset
> on top of that work?  

If Vladimir is okay with to redo and nobody else complains then I could
rebase these four patches on top of your tree next week.

Sebastian

Re:

[Vladimir Davydov]

On Tue, Jul 03, 2018 at 11:44:29PM +0200, Sebastian Andrzej Siewior wrote:
> On 2018-07-03 14:14:29 [-0700], Andrew Morton wrote:
> > 
> > > Reply-To: "[PATCH 0/4] mm/list_lru": add.list_lru_shrink_walk_irq@mail.linuxfoundation.org.and.use.it ()
> > 
> > Well that's messed up.
> 
> indeed it is. This should get into Subject:
> 
> > On Tue,  3 Jul 2018 16:52:31 +0200 Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> > 
> > > My intepretation of situtation is that Vladimir Davydon is fine patch #1
> > > and #2 of the series [0] but dislikes the irq argument and struct
> > > member. It has been suggested to use list_lru_shrink_walk_irq() instead
> > > the approach I went on in "mm: list_lru: Add lock_irq member to
> > > __list_lru_init()".
> > > 
> > > This series is based on the former two patches and introduces
> > > list_lru_shrink_walk_irq() (and makes the third patch of series
> > > obsolete).
> > > In patch 1-3 I tried a tiny cleanup so the different locking
> > > (spin_lock() vs spin_lock_irq()) is simply lifted to the caller of the
> > > function.
> > > 
> > > [0] The patch
> > >       mm: workingset: remove local_irq_disable() from count_shadow_nodes() 
> > >    and
> > >       mm: workingset: make shadow_lru_isolate() use locking suffix
> > > 
> > 
> > This isn't a very informative [0/n] changelog.  Some overall summary of
> > the patchset's objective, behaviour, use cases, testing results, etc.
> 
> The patches should be threaded as a reply to 3/3 of the series so I
> assumed it was enough. And while Vladimir complained about 2/3 and 3/3
> the discussion went on in 2/3 where he suggested to go on with the _irq
> function. And testing, well with and without RT the function was invoked
> as part of swapping (allocating memory until OOM) without complains.
> 
> > I'm seeing significant conflicts with Kirill's "Improve shrink_slab()
> > scalability (old complexity was O(n^2), new is O(n))" series, which I
> > merged eight milliseconds ago.  Kirill's patchset is large but fairly
> > straightforward so I expect it's good for 4.18.  So I suggest we leave
> > things a week or more then please take a look at redoing this patchset
> > on top of that work?  
> 
> If Vladimir is okay with to redo and nobody else complains then I could
> rebase these four patches on top of your tree next week.

IMHO this approach is more straightforward than the one with the per
list_lru flag. For all patches,

Reviewed-by: Vladimir Davydov <vdavydov.dev@gmail.com>

[PATCH 3/4] mm/list_lru: Pass struct list_lru_node as an argument __list_lru_walk_one()

[Sebastian Andrzej Siewior]

__list_lru_walk_one() is invoked with struct list_lru *lru, int nid as
the first two argument. Those two are only used to retrieve struct
list_lru_node. Since this is already done by the caller of the function
for the locking, we can pass struct list_lru_node directly and avoid the
dance around it.

Reviewed-by: Vladimir Davydov <vdavydov.dev@gmail.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 mm/list_lru.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/mm/list_lru.c b/mm/list_lru.c
index 3e36e7a239e5..7b7a737f0963 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -216,12 +216,11 @@ unsigned long list_lru_count_node(struct list_lru *lru, int nid)
 EXPORT_SYMBOL_GPL(list_lru_count_node);
 
 static unsigned long
-__list_lru_walk_one(struct list_lru *lru, int nid, int memcg_idx,
+__list_lru_walk_one(struct list_lru_node *nlru, int memcg_idx,
 		    list_lru_walk_cb isolate, void *cb_arg,
 		    unsigned long *nr_to_walk)
 {
 
-	struct list_lru_node *nlru = &lru->node[nid];
 	struct list_lru_one *l;
 	struct list_head *item, *n;
 	unsigned long isolated = 0;
@@ -283,8 +282,8 @@ list_lru_walk_one(struct list_lru *lru, int nid, struct mem_cgroup *memcg,
 	unsigned long ret;
 
 	spin_lock(&nlru->lock);
-	ret = __list_lru_walk_one(lru, nid, memcg_cache_id(memcg),
-				  isolate, cb_arg, nr_to_walk);
+	ret = __list_lru_walk_one(nlru, memcg_cache_id(memcg), isolate, cb_arg,
+				  nr_to_walk);
 	spin_unlock(&nlru->lock);
 	return ret;
 }
@@ -304,8 +303,9 @@ unsigned long list_lru_walk_node(struct list_lru *lru, int nid,
 			struct list_lru_node *nlru = &lru->node[nid];
 
 			spin_lock(&nlru->lock);
-			isolated += __list_lru_walk_one(lru, nid, memcg_idx,
-						isolate, cb_arg, nr_to_walk);
+			isolated += __list_lru_walk_one(nlru, memcg_idx,
+							isolate, cb_arg,
+							nr_to_walk);
 			spin_unlock(&nlru->lock);
 
 			if (*nr_to_walk <= 0)
-- 
2.18.0