use find_trylock_page in free_swap_and_cache instead of hand coding

PATCH In review Marcelo Tosatti 2 participants 5 messages Oct 15, 2004

Subsystems (2)

Memory management MM swap

lore

Share & tools

lore

Marcelo Tosatti

author

2004-10-15 10:45 UTC #0 lore raw

PATCH 1/1 use find_trylock_page in free_swap_and_cache instead of hand coding

Hi,

This small cleanup to free_swap_and_cache() substitues a "lock - radix lookup - TestSetPageLocked - unlock" sequence of instructions with "find_trylock_page()" (which does exactly that).

Please apply

mm/swapfile.c +2 -8

@@ -391,14 +391,8 @@ void free_swap_and_cache(swp_entry_t ent
 
 	p = swap_info_get(entry);
 	if (p) {
-		if (swap_entry_free(p, swp_offset(entry)) == 1) {
-			read_lock_irq(&swapper_space.tree_lock);
-			page = radix_tree_lookup(&swapper_space.page_tree,
-				entry.val);
-			if (page && TestSetPageLocked(page))
-				page = NULL;
-			read_unlock_irq(&swapper_space.tree_lock);
-		}
+		if (swap_entry_free(p, swp_offset(entry)) == 1) 
+			page = find_trylock_page(&swapper_space, entry.val);
 		swap_info_put(p);
 	}
 	if (page) {

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

Hugh Dickins

2004-10-15 13:20 UTC #1 lore raw

Quoted 5 lines · Marcelo Tosatti

This small cleanup to free_swap_and_cache() substitues a "lock - radix lookup - TestSetPageLocked - unlock" sequence of instructions with "find_trylock_page()" (which does exactly that).

You're right: I must have been so excited by distinguishing the swapcache from the pagecache, that I was blind to how that was still applicable (unlike inserting and removing).

But please extend your patch to mm/swap_state.c, where you can get rid of the two radix_tree_lookups by reverting to find_get_page - thanks!

Hugh

Marcelo Tosatti

author

2004-10-15 18:35 UTC #2 lore raw

Quoted 7 lines · Hugh Dickins

Quoted 5 lines · Marcelo Tosatti
This small cleanup to free_swap_and_cache() substitues a "lock - radix lookup - TestSetPageLocked - unlock" sequence of instructions with "find_trylock_page()" (which does exactly that).

Quoted 6 lines

You're right: I must have been so excited by distinguishing the swapcache from the pagecache, that I was blind to how that was still applicable (unlike inserting and removing).
But please extend your patch to mm/swap_state.c, where you can get rid of the two radix_tree_lookups by reverting to find_get_page - thanks!

Hugh,

Here it is. Can you please review an Acked-by?

That raises a question in my mind: The swapper space statistics are not protected by anything.

Two processors can write to it at the same time - I can imagine we lose a increment (two CPUs increasing at the same time), but what else can happen to the statistics due to the lack of locking?

Not that its critical data, just curiosity.

Andrew, please apply to -mm.

Description:
Use find_*_page helpers in swap code instead handcoding it.

Signed-off-by Marcelo Tosatti <[email protected]>

mm/swapfile.c +2 -8

@@ -391,14 +391,8 @@ void free_swap_and_cache(swp_entry_t ent
 
 	p = swap_info_get(entry);
 	if (p) {
-		if (swap_entry_free(p, swp_offset(entry)) == 1) {
-			read_lock_irq(&swapper_space.tree_lock);
-			page = radix_tree_lookup(&swapper_space.page_tree,
-				entry.val);
-			if (page && TestSetPageLocked(page))
-				page = NULL;
-			read_unlock_irq(&swapper_space.tree_lock);
-		}
+		if (swap_entry_free(p, swp_offset(entry)) == 1) 
+			page = find_trylock_page(&swapper_space, entry.val);
 		swap_info_put(p);
 	}
 	if (page) {

mm/swap_state.c +5 ~~-12~~

@@ -313,13 +313,11 @@ struct page * lookup_swap_cache(swp_entr
 {
 	struct page *page;
 
-	read_lock_irq(&swapper_space.tree_lock);
-	page = radix_tree_lookup(&swapper_space.page_tree, entry.val);
-	if (page) {
-		page_cache_get(page);
+	page = find_get_page(&swapper_space, entry.val);
+
+	if (page) 
 		INC_CACHE_INFO(find_success);
-	}
-	read_unlock_irq(&swapper_space.tree_lock);
+
 	INC_CACHE_INFO(find_total);
 	return page;
 }

@@ -342,12 +340,7 @@ struct page *read_swap_cache_async(swp_e
 		 * called after lookup_swap_cache() failed, re-calling
 		 * that would confuse statistics.
 		 */
-		read_lock_irq(&swapper_space.tree_lock);
-		found_page = radix_tree_lookup(&swapper_space.page_tree,
-						entry.val);
-		if (found_page)
-			page_cache_get(found_page);
-		read_unlock_irq(&swapper_space.tree_lock);
+		found_page = find_get_page(&swapper_space, entry.val);
 		if (found_page)
 			break;

Hugh Dickins

Acked-by

2004-10-15 21:54 UTC #3 lore raw

Quoted 5 lines · Marcelo Tosatti

Quoted 2 lines · Hugh Dickins
But please extend your patch to mm/swap_state.c, where you can get rid of the two radix_tree_lookups by reverting to find_get_page - thanks!
Here it is. Can you please review an Acked-by?

Looks good, thanks, yes, help yourself to one of these:

Acked-by Hugh Dickins <[email protected]>

Quoted 6 lines

That raises a question in my mind: The swapper space statistics are not protected by anything.
Two processors can write to it at the same time - I can imagine we lose a increment (two CPUs increasing at the same time), but what else can happen to the statistics due to the lack of locking?

That's right. It just doesn't matter at all: much better to lose the occasional increment than weigh it down with locking or atomicity.

When was the last time you or anyone took any interest in those numbers? From time to time I think of just ripping them out.

Hugh

Marcelo Tosatti

author

2004-10-15 21:09 UTC #4 lore raw

Quoted 9 lines · Hugh Dickins

Quoted 5 lines · Marcelo Tosatti
Quoted 2 lines · Hugh Dickins
But please extend your patch to mm/swap_state.c, where you can get rid of the two radix_tree_lookups by reverting to find_get_page - thanks!
Here it is. Can you please review an Acked-by?
Looks good, thanks, yes, help yourself to one of these:
Acked-by Hugh Dickins <[email protected]>

OK - Andrew can you please apply it to -mm.

Quoted 9 lines

Quoted 6 lines
That raises a question in my mind: The swapper space statistics are not protected by anything.
Two processors can write to it at the same time - I can imagine we lose a increment (two CPUs increasing at the same time), but what else can happen to the statistics due to the lack of locking?
That's right. It just doesn't matter at all: much better to lose the occasional increment than weigh it down with locking or atomicity.

Agreed.

Quoted 2 lines

When was the last time you or anyone took any interest in those numbers? From time to time I think of just ripping them out.

I was thinking the same when reading the code.

The thing is, there might be users still - we probably want to keep compatibility (heck, I dont know compatibility to what, but lets imagine there is some application out there who uses it).

I think we can make it optional on CONFIG_EMBEDDED - if its set, make the INC_CACHE_ #defines NULL. What you think of that?

Then remove later on v2.7.

Thats a conservative approach - we could just rip off it completly.