lkml.org 
[lkml]   [2014]   [Feb]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: mmotm 2014-02-05 list_lru_add lockdep splat
On Fri, Feb 07, 2014 at 12:52:33PM -0800, Andrew Morton wrote:
> On Thu, 6 Feb 2014 11:41:36 -0500 Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> >
> > Make the shadow lru->node[i].lock IRQ-safe to remove the order
> > dictated by interruption. This slightly increases the IRQ-disabled
> > section in the shadow shrinker, but it still drops all locks and
> > enables IRQ after every reclaimed shadow radix tree node.
> >
> > ...
> >
> > --- a/mm/workingset.c
> > +++ b/mm/workingset.c
> > @@ -273,7 +273,10 @@ static unsigned long count_shadow_nodes(struct shrinker *shrinker,
> > unsigned long max_nodes;
> > unsigned long pages;
> >
> > + local_irq_disable();
> > shadow_nodes = list_lru_count_node(&workingset_shadow_nodes, sc->nid);
> > + local_irq_enable();
>
> This is a bit ugly-looking.
>
> A reader will look at that and wonder why the heck we're disabling
> interrupts here. Against what? Is there some way in which we can
> clarify this?

We need the list_lru's internal locking to be IRQ-safe because of the
mapping->tree_lock nesting.

This particular instance should go away once Dave's scalability fixes
from last summer get merged, the lock and thus the IRQ-disabling is
not necessary to read that counter: https://lkml.org/lkml/2013/7/31/7

This leaves the IRQ-disabling in the scan and isolate function.

[ I also noticed the patch that does an optimistic list_empty() check
before acquiring the locks, which is something my callers also do,
so it might be worth revisiting the missing pieces of this patch
set: https://lkml.org/lkml/2013/7/31/9 -- Dave? ]

> Perhaps adding list_lru_count_node_irq[save] and
> list_lru_walk_node_irq[save] would be better - is it reasonable to
> assume this is the only caller of the list_lru code which will ever
> want irq-safe treatment?

It's a combination of our objects being locked from IRQ context and
using the object lock for lifetime management, which means list_lru
locking has to nest inside the IRQ-safe object lock. I suspect this
will not be too common and mapping->tree_lock will remain the oddball.

> This is all somewhat a side-effect of list_lru implementing its own
> locking rather than requiring caller-provided locking. It's always a
> mistake.

The locks are embedded in the internal per-node structure that first
has to be looked up. All the users could live with the internal
locking, so I can see why Dave didn't want to push 4 manual steps
(lookup, lock, list op, unlock) into all callsites unnecessarily.

Are the two manual IRQ-disabling sections in one user enough to change
all that? I'm perfectly neutral on this, so... path of least
resistance ;)

---
Subject: [patch] mm: keep page cache radix tree nodes in check fix fix fix

document IRQ-disabling around list_lru API calls

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
mm/workingset.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/mm/workingset.c b/mm/workingset.c
index 20aa16754305..f7216fa7da27 100644
--- a/mm/workingset.c
+++ b/mm/workingset.c
@@ -273,6 +273,7 @@ static unsigned long count_shadow_nodes(struct shrinker *shrinker,
unsigned long max_nodes;
unsigned long pages;

+ /* list_lru lock nests inside IRQ-safe mapping->tree_lock */
local_irq_disable();
shadow_nodes = list_lru_count_node(&workingset_shadow_nodes, sc->nid);
local_irq_enable();
@@ -373,6 +374,7 @@ static unsigned long scan_shadow_nodes(struct shrinker *shrinker,
{
unsigned long ret;

+ /* list_lru lock nests inside IRQ-safe mapping->tree_lock */
local_irq_disable();
ret = list_lru_walk_node(&workingset_shadow_nodes, sc->nid,
shadow_lru_isolate, NULL, &sc->nr_to_scan);
--
1.8.5.3


\
 
 \ /
  Last update: 2014-02-09 20:01    [W:1.062 / U:0.424 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site