[patch] mm: shmem: use new radix tree iterator

[patch] mm: shmem: use new radix tree iterator

[Johannes Weiner]

In shmem_find_get_pages_and_swap, use the faster radix tree iterator
construct from 78c1d78 "radix-tree: introduce bit-optimized iterator".

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/shmem.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index a368a1c..c5dc8ae 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -336,19 +336,19 @@ static unsigned shmem_find_get_pages_and_swap(struct address_space *mapping,
 					pgoff_t start, unsigned int nr_pages,
 					struct page **pages, pgoff_t *indices)
 {
-	unsigned int i;
-	unsigned int ret;
-	unsigned int nr_found;
+	void **slot;
+	unsigned int ret = 0;
+	struct radix_tree_iter iter;
+
+	if (!nr_pages)
+		return 0;
 
 	rcu_read_lock();
 restart:
-	nr_found = radix_tree_gang_lookup_slot(&mapping->page_tree,
-				(void ***)pages, indices, start, nr_pages);
-	ret = 0;
-	for (i = 0; i < nr_found; i++) {
+	radix_tree_for_each_slot(slot, &mapping->page_tree, &iter, start) {
 		struct page *page;
 repeat:
-		page = radix_tree_deref_slot((void **)pages[i]);
+		page = radix_tree_deref_slot(slot);
 		if (unlikely(!page))
 			continue;
 		if (radix_tree_exception(page)) {
@@ -365,17 +365,16 @@ static unsigned shmem_find_get_pages_and_swap(struct address_space *mapping,
 			goto repeat;
 
 		/* Has the page moved? */
-		if (unlikely(page != *((void **)pages[i]))) {
+		if (unlikely(page != *slot)) {
 			page_cache_release(page);
 			goto repeat;
 		}
 export:
-		indices[ret] = indices[i];
+		indices[ret] = iter.index;
 		pages[ret] = page;
-		ret++;
+		if (++ret == nr_pages)
+			break;
 	}
-	if (unlikely(!ret && nr_found))
-		goto restart;
 	rcu_read_unlock();
 	return ret;
 }
-- 
1.7.11.7

--
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>

Re: [patch] mm: shmem: use new radix tree iterator

[Konstantin Khlebnikov]

Johannes Weiner wrote:
> In shmem_find_get_pages_and_swap, use the faster radix tree iterator
> construct from 78c1d78 "radix-tree: introduce bit-optimized iterator".
>
> Signed-off-by: Johannes Weiner<hannes@cmpxchg.org>

Hmm, ACK. shmem_unuse_inode() also can be redone in this way.
I did something similar year ago: https://lkml.org/lkml/2012/2/10/388
As result we can rid of radix_tree_locate_item() and shmem_find_get_pages_and_swap()

> ---
>   mm/shmem.c | 25 ++++++++++++-------------
>   1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index a368a1c..c5dc8ae 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -336,19 +336,19 @@ static unsigned shmem_find_get_pages_and_swap(struct address_space *mapping,
>   					pgoff_t start, unsigned int nr_pages,
>   					struct page **pages, pgoff_t *indices)
>   {
> -	unsigned int i;
> -	unsigned int ret;
> -	unsigned int nr_found;
> +	void **slot;
> +	unsigned int ret = 0;
> +	struct radix_tree_iter iter;
> +
> +	if (!nr_pages)
> +		return 0;
>
>   	rcu_read_lock();
>   restart:
> -	nr_found = radix_tree_gang_lookup_slot(&mapping->page_tree,
> -				(void ***)pages, indices, start, nr_pages);
> -	ret = 0;
> -	for (i = 0; i<  nr_found; i++) {
> +	radix_tree_for_each_slot(slot,&mapping->page_tree,&iter, start) {
>   		struct page *page;
>   repeat:
> -		page = radix_tree_deref_slot((void **)pages[i]);
> +		page = radix_tree_deref_slot(slot);
>   		if (unlikely(!page))
>   			continue;
>   		if (radix_tree_exception(page)) {
> @@ -365,17 +365,16 @@ static unsigned shmem_find_get_pages_and_swap(struct address_space *mapping,
>   			goto repeat;
>
>   		/* Has the page moved? */
> -		if (unlikely(page != *((void **)pages[i]))) {
> +		if (unlikely(page != *slot)) {
>   			page_cache_release(page);
>   			goto repeat;
>   		}
>   export:
> -		indices[ret] = indices[i];
> +		indices[ret] = iter.index;
>   		pages[ret] = page;
> -		ret++;
> +		if (++ret == nr_pages)
> +			break;
>   	}
> -	if (unlikely(!ret&&  nr_found))
> -		goto restart;
>   	rcu_read_unlock();
>   	return ret;
>   }

--
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>

Re: [patch] mm: shmem: use new radix tree iterator

[Hugh Dickins]

On Sat, 2 Feb 2013, Konstantin Khlebnikov wrote:
> Johannes Weiner wrote:
> > In shmem_find_get_pages_and_swap, use the faster radix tree iterator
> > construct from 78c1d78 "radix-tree: introduce bit-optimized iterator".
> > 
> > Signed-off-by: Johannes Weiner<hannes@cmpxchg.org>
> 
> Hmm, ACK. shmem_unuse_inode() also can be redone in this way.
> I did something similar year ago: https://lkml.org/lkml/2012/2/10/388
> As result we can rid of radix_tree_locate_item() and
> shmem_find_get_pages_and_swap()

Indeed you did, and never got more than a "I have some reservations"
response out of me; and already we had both moved on to much more
pressing lruvec and other concerns.

My first reaction on seeing Johannes' patch was, not to ack it immediately,
but look back to your series of 6 (or 4): shmem_find_get_pages_and_swap()
doesn't get updated in yours, but vanishes in the last patch, which was
among the ones I was uneasy about.  Here's a belated account of my
reactions to your series.

[PATCH 1/4] shmem: simplify shmem_unlock_mapping
Probably good, though should also update the "only reach" comment in
find_get_pages(); and probably not worthwhile unless shmem_find_get_
pages_and_swap() is to disappear entirely.

[PATCH 2/4] shmem: tag swap entries in radix tree
Using a tag instead of and in addition to the swap exceptional entries
was certainly something I tried when I was updating shmem_unuse(): it
just didn't work as well as I'd hoped and needed, nothing worked as
"well" as the radix_tree_locate_item() thing I added, though I'd have
preferred to avoid adding it.  So I needed to test and understand why
you found tags worked where I had not: probably partly your intervening
radix_tree changes, and partly a difference in how we tested.  There
was also a little issue fo SHMEM_TAG_SWAP == PAGECACHE_TAG_DIRTY: you
were absolutely right not to enlarge the tagspace, but at that time
there was a weird issue of page migration putting a dirty tag into
the tmpfs radix_tree, which later I worked around in 752dc185.

[PATCH 3/4] shmem: use radix-tree iterator in shmem_unuse_inode()
Removes lots of code which is great, but as I said, I'd need
to investigate why tagging worked for you but not for me.

[PATCH 4/4] mm: use swap readahead at swapoff
I've tried that down the years from time to time, and never found
it useful (but I see you found it works better in a virtual machine).
I've no strong objection to the patch, but when I rewrote try_to_unuse()
twelve years ago, I was overly sensitive to readahead adding pressure in
the case where you're already swapping off under pressure, and preferred
to avoid the readahead if it didn't help.  The slowness of swapoff has
very little to do with readahead or not, or that's what I always found:
if swapoff while loaded, readahead increased the memory pressure; if
(usual case) swapoff while not loaded, apparently the disk's own
caching was good enough that kernel readahead made no difference.

[PATCH 5/4] shmem: put shmem_delete_from_page_cache under CONFIG_SWAP
I'm completely schizophrenic about #fidef CONFIG_SWAPs, sometimes I
love to add them, and sometimes I think they're ugly.  You're probably
right that mm/shmem.c should have more of them, it helps document too.

[PATCH 6/4] shmem: simplify shmem_truncate_range
Where shmem_find_get_pages_and_swap() goes away.  But
you replace (what was then) shmem_truncate_range() by two passes,
truncate_inode_pages_range() followed by shmem_truncate_swap_range().
That's not good enough, and part of what I was getting away from with
the radix_tree exceptional swap changes: there needs to be more, to
prevent pages moving to swap before the page pass finds them, then
swap moving to pages before the swap pass finds them.  The old code
used a flag to go back whenever that might happen, effective but not
pretty (and I'm not sure complete).  I ought to like your end result,
with so much code deleted; but somehow I did not, just too attached
to my own I suppose :) Intervening (fallocate) changes have moved this
code around, certainly your old patch would not apply, whether they've
made any material difficulty I've not considered.

As usual, I'm busy with other things, so not actually in any hurry
for a resend; but thought I'd better let you know what I'd thought,
in case Johannes' patch prompted you towards a resend.

Hugh

--
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>

Re: [patch] mm: shmem: use new radix tree iterator

[Konstantin Khlebnikov]

Hugh Dickins wrote:
> On Sat, 2 Feb 2013, Konstantin Khlebnikov wrote:
>> Johannes Weiner wrote:
>>> In shmem_find_get_pages_and_swap, use the faster radix tree iterator
>>> construct from 78c1d78 "radix-tree: introduce bit-optimized iterator".
>>>
>>> Signed-off-by: Johannes Weiner<hannes@cmpxchg.org>
>>
>> Hmm, ACK. shmem_unuse_inode() also can be redone in this way.
>> I did something similar year ago: https://lkml.org/lkml/2012/2/10/388
>> As result we can rid of radix_tree_locate_item() and
>> shmem_find_get_pages_and_swap()
>
> Indeed you did, and never got more than a "I have some reservations"
> response out of me; and already we had both moved on to much more
> pressing lruvec and other concerns.
>
> My first reaction on seeing Johannes' patch was, not to ack it immediately,
> but look back to your series of 6 (or 4): shmem_find_get_pages_and_swap()
> doesn't get updated in yours, but vanishes in the last patch, which was
> among the ones I was uneasy about.  Here's a belated account of my
> reactions to your series.

Thanks for review.

I mentioned that patchset with hope that somebody get interested to
continue that work. Recently there was many patches related to the swap.

Currently I'm working on replacing our homemade memory controller with
something based on memcg. So I'll revisit 'lruvec/lru_lock' patchset soon.

>
> [PATCH 1/4] shmem: simplify shmem_unlock_mapping
> Probably good, though should also update the "only reach" comment in
> find_get_pages(); and probably not worthwhile unless shmem_find_get_
> pages_and_swap() is to disappear entirely.
>
> [PATCH 2/4] shmem: tag swap entries in radix tree
> Using a tag instead of and in addition to the swap exceptional entries
> was certainly something I tried when I was updating shmem_unuse(): it
> just didn't work as well as I'd hoped and needed, nothing worked as
> "well" as the radix_tree_locate_item() thing I added, though I'd have
> preferred to avoid adding it.  So I needed to test and understand why
> you found tags worked where I had not: probably partly your intervening
> radix_tree changes, and partly a difference in how we tested.  There
> was also a little issue fo SHMEM_TAG_SWAP == PAGECACHE_TAG_DIRTY: you
> were absolutely right not to enlarge the tagspace, but at that time
> there was a weird issue of page migration putting a dirty tag into
> the tmpfs radix_tree, which later I worked around in 752dc185.
>
> [PATCH 3/4] shmem: use radix-tree iterator in shmem_unuse_inode()
> Removes lots of code which is great, but as I said, I'd need
> to investigate why tagging worked for you but not for me.
>
> [PATCH 4/4] mm: use swap readahead at swapoff
> I've tried that down the years from time to time, and never found
> it useful (but I see you found it works better in a virtual machine).
> I've no strong objection to the patch, but when I rewrote try_to_unuse()
> twelve years ago, I was overly sensitive to readahead adding pressure in
> the case where you're already swapping off under pressure, and preferred
> to avoid the readahead if it didn't help.  The slowness of swapoff has
> very little to do with readahead or not, or that's what I always found:
> if swapoff while loaded, readahead increased the memory pressure; if
> (usual case) swapoff while not loaded, apparently the disk's own
> caching was good enough that kernel readahead made no difference.
>
> [PATCH 5/4] shmem: put shmem_delete_from_page_cache under CONFIG_SWAP
> I'm completely schizophrenic about #fidef CONFIG_SWAPs, sometimes I
> love to add them, and sometimes I think they're ugly.  You're probably
> right that mm/shmem.c should have more of them, it helps document too.
>
> [PATCH 6/4] shmem: simplify shmem_truncate_range
> Where shmem_find_get_pages_and_swap() goes away.  But
> you replace (what was then) shmem_truncate_range() by two passes,
> truncate_inode_pages_range() followed by shmem_truncate_swap_range().
> That's not good enough, and part of what I was getting away from with
> the radix_tree exceptional swap changes: there needs to be more, to
> prevent pages moving to swap before the page pass finds them, then
> swap moving to pages before the swap pass finds them.  The old code
> used a flag to go back whenever that might happen, effective but not
> pretty (and I'm not sure complete).  I ought to like your end result,
> with so much code deleted; but somehow I did not, just too attached
> to my own I suppose :) Intervening (fallocate) changes have moved this
> code around, certainly your old patch would not apply, whether they've
> made any material difficulty I've not considered.
>
> As usual, I'm busy with other things, so not actually in any hurry
> for a resend; but thought I'd better let you know what I'd thought,
> in case Johannes' patch prompted you towards a resend.
>
> Hugh

--
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>

Re: [patch] mm: shmem: use new radix tree iterator

[Johannes Weiner]

On Sat, Feb 02, 2013 at 12:25:44PM +0400, Konstantin Khlebnikov wrote:
> Johannes Weiner wrote:
> >In shmem_find_get_pages_and_swap, use the faster radix tree iterator
> >construct from 78c1d78 "radix-tree: introduce bit-optimized iterator".
> >
> >Signed-off-by: Johannes Weiner<hannes@cmpxchg.org>
> 
> Hmm, ACK. shmem_unuse_inode() also can be redone in this way.
> I did something similar year ago: https://lkml.org/lkml/2012/2/10/388
> As result we can rid of radix_tree_locate_item() and shmem_find_get_pages_and_swap()

I remember your patches and am working on a totally unrelated series
that also gets rid of shmem_find_get_pages_and_swap().  Either way,
this thing's going down, so just I didn't bother with the conversion.

--
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>

Re: [patch] mm: shmem: use new radix tree iterator

[Hugh Dickins]

On Fri, 1 Feb 2013, Johannes Weiner wrote:

> In shmem_find_get_pages_and_swap, use the faster radix tree iterator
> construct from 78c1d78 "radix-tree: introduce bit-optimized iterator".
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>

Yes, that looks fine, and is testing out fine, thanks.
Acked-by: Hugh Dickins <hughd@google.com>

> ---
>  mm/shmem.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index a368a1c..c5dc8ae 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -336,19 +336,19 @@ static unsigned shmem_find_get_pages_and_swap(struct address_space *mapping,
>  					pgoff_t start, unsigned int nr_pages,
>  					struct page **pages, pgoff_t *indices)
>  {
> -	unsigned int i;
> -	unsigned int ret;
> -	unsigned int nr_found;
> +	void **slot;
> +	unsigned int ret = 0;
> +	struct radix_tree_iter iter;
> +
> +	if (!nr_pages)
> +		return 0;
>  
>  	rcu_read_lock();
>  restart:
> -	nr_found = radix_tree_gang_lookup_slot(&mapping->page_tree,
> -				(void ***)pages, indices, start, nr_pages);
> -	ret = 0;
> -	for (i = 0; i < nr_found; i++) {
> +	radix_tree_for_each_slot(slot, &mapping->page_tree, &iter, start) {
>  		struct page *page;
>  repeat:
> -		page = radix_tree_deref_slot((void **)pages[i]);
> +		page = radix_tree_deref_slot(slot);
>  		if (unlikely(!page))
>  			continue;
>  		if (radix_tree_exception(page)) {
> @@ -365,17 +365,16 @@ static unsigned shmem_find_get_pages_and_swap(struct address_space *mapping,
>  			goto repeat;
>  
>  		/* Has the page moved? */
> -		if (unlikely(page != *((void **)pages[i]))) {
> +		if (unlikely(page != *slot)) {
>  			page_cache_release(page);
>  			goto repeat;
>  		}
>  export:
> -		indices[ret] = indices[i];
> +		indices[ret] = iter.index;
>  		pages[ret] = page;
> -		ret++;
> +		if (++ret == nr_pages)
> +			break;
>  	}
> -	if (unlikely(!ret && nr_found))
> -		goto restart;
>  	rcu_read_unlock();
>  	return ret;
>  }
> -- 
> 1.7.11.7

--
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>

Re: [patch] mm: shmem: use new radix tree iterator

[Ric Mason]

Hi Hugh,
On 02/04/2013 10:01 AM, Hugh Dickins wrote:
> On Fri, 1 Feb 2013, Johannes Weiner wrote:
>
>> In shmem_find_get_pages_and_swap, use the faster radix tree iterator
>> construct from 78c1d78 "radix-tree: introduce bit-optimized iterator".
>>
>> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> Yes, that looks fine, and is testing out fine, thanks.
> Acked-by: Hugh Dickins <hughd@google.com>

Could you share your testcase with me? It seems that you always can test 
shmem patches.

>
>> ---
>>   mm/shmem.c | 25 ++++++++++++-------------
>>   1 file changed, 12 insertions(+), 13 deletions(-)
>>
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index a368a1c..c5dc8ae 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -336,19 +336,19 @@ static unsigned shmem_find_get_pages_and_swap(struct address_space *mapping,
>>   					pgoff_t start, unsigned int nr_pages,
>>   					struct page **pages, pgoff_t *indices)
>>   {
>> -	unsigned int i;
>> -	unsigned int ret;
>> -	unsigned int nr_found;
>> +	void **slot;
>> +	unsigned int ret = 0;
>> +	struct radix_tree_iter iter;
>> +
>> +	if (!nr_pages)
>> +		return 0;
>>   
>>   	rcu_read_lock();
>>   restart:
>> -	nr_found = radix_tree_gang_lookup_slot(&mapping->page_tree,
>> -				(void ***)pages, indices, start, nr_pages);
>> -	ret = 0;
>> -	for (i = 0; i < nr_found; i++) {
>> +	radix_tree_for_each_slot(slot, &mapping->page_tree, &iter, start) {
>>   		struct page *page;
>>   repeat:
>> -		page = radix_tree_deref_slot((void **)pages[i]);
>> +		page = radix_tree_deref_slot(slot);
>>   		if (unlikely(!page))
>>   			continue;
>>   		if (radix_tree_exception(page)) {
>> @@ -365,17 +365,16 @@ static unsigned shmem_find_get_pages_and_swap(struct address_space *mapping,
>>   			goto repeat;
>>   
>>   		/* Has the page moved? */
>> -		if (unlikely(page != *((void **)pages[i]))) {
>> +		if (unlikely(page != *slot)) {
>>   			page_cache_release(page);
>>   			goto repeat;
>>   		}
>>   export:
>> -		indices[ret] = indices[i];
>> +		indices[ret] = iter.index;
>>   		pages[ret] = page;
>> -		ret++;
>> +		if (++ret == nr_pages)
>> +			break;
>>   	}
>> -	if (unlikely(!ret && nr_found))
>> -		goto restart;
>>   	rcu_read_unlock();
>>   	return ret;
>>   }
>> -- 
>> 1.7.11.7
> --
> 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>

--
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>