[linux-mm-cc] Re: CCache problem

Nitin Gupta nitingupta.mail at gmail.com
Wed Jun 28 05:31:39 EDT 2006


Rik van Riel wrote:
> On Tue, 27 Jun 2006, Nitin Gupta wrote:
>> Problem 2: These allocation can sleep (esp. under mem pressure).
>> If find_get_page() cannot sleep then how can we do decompression?
> 
> After you allocate a fresh page, re-take the lock.
> 
> It is possible somebody else also allocated a page simultaneously,
> and hooked that page into the the page cache before you.  In that
> case, free your page and use the other guy's page :)
> 

First, I forgot to unlock chunk_head :)

		WLQ();
		*slot = new_page;
		page_cache_get(new_page);
+		unlock_chunk_head(ch);
		/* free chunk_head if only we are the user */
		if (put_page_testzero(ch)) kfree(ch);
		WUQ();

Now, I have doubts in what you suggested:
If 100 people come at find_get_page() (and other lookup functions), only one of 
them will get lock on chunk_head and get to point where we alloc_page and 
kmalloc(64kb). Rest of 99 cannot go beyond wait_on_chunk_head() until that one 
guy frees lock on chunk_head. So, there's no one else who can alloc the page 
simultaneously.

So, the problem still remains: The allocation done by this one guy can sleep 
while find_get_page() cannot.

If allocation sleeps it will cause another problem: chunk_head cannot be 
unlocked till we decompress the page and then hook it into page cache. And we 
have taken spinlock (from prob 1) to wait for decompression to finish.


>> Do you think there are problems in above code?
> 
> You do not check *slot after you re-take the lock.
> 
> Remember that you dropped the lock, so anything could have
> happened.  Better start from the beginning, maybe using a
> goto?
> 
> The function do_generic_mapping_read() in mm/filemap.c
> is a nice example of all the things that could go wrong,
> and how easy it is to deal with those by simply starting
> your lookup from the beginning again :)
> 

...
		RUQ();
		wait_on_chunk_head(ch);
		RLQ();

Now, I think, only foll. cases can occur:
1. Radix node corresponding to slot has disappeared. In this case 
find_get_page() should return NULL. Doing *slot in this case is an error.
2. Page has been freed/truncated. In this case *slot will be NULL and this is 
what we return.
3. *slot != NULL. Here, just return this *slot (as in case 2).

Currently, I'm not making sure if radix node for slot itself has disappeared or 
not. This, I think, can be done simply by:


...
                 RUQ();
                 wait_on_chunk_head(ch);
                 RLQ();

                 /* slot now has decompressed page
                  * return slot (it may be NULL) */
-               page = *slot;
+		page = radix_tree_lookup(&mapping->page_tree, offset);
                 if (page) page_cache_get(page);

                 /* free chunk_head if only we are the user */
		if (put_page_testzero(ch)) kfree(ch);
                 RUQ();
                 return page;
...


Using goto to beginning (just after very first RLQ) should also do.


-- Nitin



More information about the linux-mm-cc mailing list