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@xxxxxxxxxxx> 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@xxxxxxxxxxx> --- 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 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>