Re: [PATCH v5 08/31] list: add a new LRU list type

On Fri, May 10, 2013 at 01:56:29PM +0400, Glauber Costa wrote:

On 05/10/2013 01:21 PM, Mel Gorman wrote:

quoted

On Fri, May 10, 2013 at 01:02:07AM +0400, Glauber Costa wrote:

quoted

On 05/09/2013 05:37 PM, Mel Gorman wrote:

quoted

On Thu, May 09, 2013 at 10:06:25AM +0400, Glauber Costa wrote:

quoted

From: Dave Chinner <redacted>

Several subsystems use the same construct for LRU lists - a list
head, a spin lock and and item count. They also use exactly the same
code for adding and removing items from the LRU. Create a generic
type for these LRU lists.

This is the beginning of generic, node aware LRUs for shrinkers to
work with.

[ glommer: enum defined constants for lru. Suggested by gthelen,
  don't relock over retry ]
Signed-off-by: Dave Chinner <redacted>
Signed-off-by: Glauber Costa <redacted>
Reviewed-by: Greg Thelen <redacted>

quoted

<SNIP>

+
+unsigned long
+list_lru_walk(
+	struct list_lru *lru,
+	list_lru_walk_cb isolate,
+	void		*cb_arg,
+	long		nr_to_walk)
+{
+	struct list_head *item, *n;
+	unsigned long removed = 0;
+
+	spin_lock(&lru->lock);
+restart:
+	list_for_each_safe(item, n, &lru->list) {
+		enum lru_status ret;
+
+		if (nr_to_walk-- < 0)
+			break;
+
+		ret = isolate(item, &lru->lock, cb_arg);
+		switch (ret) {
+		case LRU_REMOVED:
+			lru->nr_items--;
+			removed++;
+			break;
+		case LRU_ROTATE:
+			list_move_tail(item, &lru->list);
+			break;
+		case LRU_SKIP:
+			break;
+		case LRU_RETRY:
+			goto restart;
+		default:
+			BUG();
+		}
+	}

What happened your suggestion to only retry once for each object to
avoid any possibility of infinite looping or stalling for prolonged
periods of time waiting on XFS to do something?

Sorry. It wasn't clear for me if you were just trying to make sure we
had a way out in case it proves to be a problem, or actually wanted a
change.

Either. If you are sure there is a way out for XFS using LRU_RETRY without
prolonged stalls then it's fine. If it is not certain then I would be much
more comfortable with a retry-once and then moving onto the next LRU node.

quoted

In any case, I cannot claim to be as knowledgeable as Dave in the
subtleties of such things in the final behavior of the shrinker. Dave,
can you give us your input here?

I also have another recent observation on this:

The main difference between LRU_SKIP and LRU_RETRY is that LRU_RETRY
will go back to the beginning of the list, and start scanning it again.

Only sortof true. Lets say we had a list of 8 LRU nodes. Nodes 1-3 get
isolated. Node 4 returns LRU_RETRY so we goto restart. The first item on
the list is now potentially LRU_RETRY which it must handle before
reaching Nodes 5-8

LRU_SKIP is different. If Node 4 returned LRU_SKIP then Node 5-8 are
ignored entirely. Actually..... why is that? LRU_SKIP is documented to
"item cannot be locked, skip" but what it actually does it "item cannot
be locked, abort the walk". It's documented behaviour LRU_SKIP implies
continue, not break.

	case LRU_SKIP:
		continue;

but we are only breaking the switch statement, so this is a de facto
continue.

Bah, I'm a tool.

-- 
Mel Gorman
SUSE Labs

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