Hi,
The comment for struct shrinker in include/linux/mm.h says
"shrink...It should return the number of objects which remain in the
cache."
Please notice the word "remain".
In fs/mbcache.h, mb_cache_shrink_fn is used as the shrink function:
static struct shrinker mb_cache_shrinker = {
.shrink = mb_cache_shrink_fn,
.seeks = DEFAULT_SEEKS,
};
In mb_cache_shrink_fn, the return value for nr_to_scan > 0 is the
number of mb_cache_entry before shrink operation. It may because the
memory usage for mbcache is low, so the effect is not so obvious.
I think we'd better fix the return value issue.
Following patch is against 2.6.35-rc5. Please check it.
Signed-off-by: Wang Sheng-Hui <[email protected]>
---
fs/mbcache.c | 10 ++++++++++
1 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/fs/mbcache.c b/fs/mbcache.c
index ec88ff3..412e7cc 100644
--- a/fs/mbcache.c
+++ b/fs/mbcache.c
@@ -228,6 +228,16 @@ mb_cache_shrink_fn(int nr_to_scan, gfp_t gfp_mask)
__mb_cache_entry_forget(list_entry(l, struct mb_cache_entry,
e_lru_list), gfp_mask);
}
+ spin_lock(&mb_cache_spinlock);
+ count = 0;
+ list_for_each(l, &mb_cache_list) {
+ struct mb_cache *cache =
+ list_entry(l, struct mb_cache, c_cache_list);
+ mb_debug("cache %s (%d)", cache->c_name,
+ atomic_read(&cache->c_entry_count));
+ count += atomic_read(&cache->c_entry_count);
+ }
+ spin_unlock(&mb_cache_spinlock);
out:
return (count / 100) * sysctl_vfs_cache_pressure;
}
--
1.7.1.1
--
Thanks and Regards,
shenghui
--
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>
Wang Sheng-Hui wrote:
> Hi,
>
> The comment for struct shrinker in include/linux/mm.h says
> "shrink...It should return the number of objects which remain in the
> cache."
> Please notice the word "remain".
>
> In fs/mbcache.h, mb_cache_shrink_fn is used as the shrink function:
> static struct shrinker mb_cache_shrinker = {
> .shrink = mb_cache_shrink_fn,
> .seeks = DEFAULT_SEEKS,
> };
> In mb_cache_shrink_fn, the return value for nr_to_scan > 0 is the
> number of mb_cache_entry before shrink operation. It may because the
> memory usage for mbcache is low, so the effect is not so obvious.
> I think we'd better fix the return value issue.
>
> Following patch is against 2.6.35-rc5. Please check it.
>
>
you are right that it's not returning the remaining entries, but I think
we can do this more simply; there isn't any reason to calculate it twice
How about just moving the accounting to the end, since "count" isn't actually
used when freeing, anyway.... something like this?
diff --git a/fs/mbcache.c b/fs/mbcache.c
index ec88ff3..3af79de 100644
--- a/fs/mbcache.c
+++ b/fs/mbcache.c
@@ -203,19 +203,11 @@ mb_cache_shrink_fn(int nr_to_scan, gfp_t gfp_mask)
struct list_head *l, *ltmp;
int count = 0;
- spin_lock(&mb_cache_spinlock);
- list_for_each(l, &mb_cache_list) {
- struct mb_cache *cache =
- list_entry(l, struct mb_cache, c_cache_list);
- mb_debug("cache %s (%d)", cache->c_name,
- atomic_read(&cache->c_entry_count));
- count += atomic_read(&cache->c_entry_count);
- }
mb_debug("trying to free %d entries", nr_to_scan);
- if (nr_to_scan == 0) {
- spin_unlock(&mb_cache_spinlock);
+ if (nr_to_scan == 0)
goto out;
- }
+
+ spin_lock &mb_cache_spinlock);
while (nr_to_scan-- && !list_empty(&mb_cache_lru_list)) {
struct mb_cache_entry *ce =
list_entry(mb_cache_lru_list.next,
@@ -229,6 +221,17 @@ mb_cache_shrink_fn(int nr_to_scan, gfp_t gfp_mask)
e_lru_list), gfp_mask);
}
out:
+ /* Count remaining entries */
+ spin_lock(&mb_cache_spinlock);
+ list_for_each(l, &mb_cache_list) {
+ struct mb_cache *cache =
+ list_entry(l, struct mb_cache, c_cache_list);
+ mb_debug("cache %s (%d)", cache->c_name,
+ atomic_read(&cache->c_entry_count));
+ count += atomic_read(&cache->c_entry_count);
+ }
+ spin_unlock(&mb_cache_spinlock);
+
return (count / 100) * sysctl_vfs_cache_pressure;
}
On Sat, Jul 17, 2010 at 11:06:32PM -0500, Eric Sandeen wrote:
> + /* Count remaining entries */
> + spin_lock(&mb_cache_spinlock);
> + list_for_each(l, &mb_cache_list) {
> + struct mb_cache *cache =
> + list_entry(l, struct mb_cache, c_cache_list);
This should be using list_for_each_entry.
--
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>
于 2010-7-18 14:01, Christoph Hellwig 写道:
> On Sat, Jul 17, 2010 at 11:06:32PM -0500, Eric Sandeen wrote:
>> + /* Count remaining entries */
>> + spin_lock(&mb_cache_spinlock);
>> + list_for_each(l,&mb_cache_list) {
>> + struct mb_cache *cache =
>> + list_entry(l, struct mb_cache, c_cache_list);
>
> This should be using list_for_each_entry.
>
I regenerated the patch. Please check it.
Signed-off-by: Wang Sheng-Hui <[email protected]>
---
fs/mbcache.c | 22 +++++++++++-----------
1 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/fs/mbcache.c b/fs/mbcache.c
index ec88ff3..5697d9e 100644
--- a/fs/mbcache.c
+++ b/fs/mbcache.c
@@ -201,21 +201,13 @@ mb_cache_shrink_fn(int nr_to_scan, gfp_t gfp_mask)
{
LIST_HEAD(free_list);
struct list_head *l, *ltmp;
+ struct mb_cache *cache;
int count = 0;
- spin_lock(&mb_cache_spinlock);
- list_for_each(l, &mb_cache_list) {
- struct mb_cache *cache =
- list_entry(l, struct mb_cache, c_cache_list);
- mb_debug("cache %s (%d)", cache->c_name,
- atomic_read(&cache->c_entry_count));
- count += atomic_read(&cache->c_entry_count);
- }
mb_debug("trying to free %d entries", nr_to_scan);
- if (nr_to_scan == 0) {
- spin_unlock(&mb_cache_spinlock);
+ if (nr_to_scan == 0)
goto out;
- }
+
while (nr_to_scan-- && !list_empty(&mb_cache_lru_list)) {
struct mb_cache_entry *ce =
list_entry(mb_cache_lru_list.next,
@@ -229,6 +221,14 @@ mb_cache_shrink_fn(int nr_to_scan, gfp_t gfp_mask)
e_lru_list), gfp_mask);
}
out:
+ spin_lock(&mb_cache_spinlock);
+ list_for_each_entry(cache, &mb_cache_list, c_cache_list) {
+ mb_debug("cache %s (%d)", cache->c_name,
+ atomic_read(&cache->c_entry_count));
+ count += atomic_read(&cache->c_entry_count);
+ }
+ spin_unlock(&mb_cache_spinlock);
+
return (count / 100) * sysctl_vfs_cache_pressure;
}
--
1.7.1.1
--
Thanks and Regards,
shenghui
--
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>
On Sunday 18 July 2010 08:36:59 Wang Sheng-Hui wrote:
> I regenerated the patch. Please check it.
The logic for calculating how many objects to free is still wrong:
mb_cache_shrink_fn returns the number of entries scaled by
sysctl_vfs_cache_pressure / 100. It should also scale nr_to_scan by the
inverse of that. The sysctl_vfs_cache_pressure == 0 case (never scale) may
require special attention.
See dcache_shrinker() in fs/dcache.c.
Thanks,
Andreas
--
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>
On Sunday 18 July 2010 08:36:59 Wang Sheng-Hui wrote:
> 于 2010-7-18 14:01, Christoph Hellwig 写道:
> > This should be using list_for_each_entry.
It would make sense to change this throughout the whole file.
Thanks,
Andreas
--
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
ilto:"[email protected]"> [email protected] </a>
2010/7/20 Andreas Gruenbacher <[email protected]>:
> On Sunday 18 July 2010 08:36:59 Wang Sheng-Hui wrote:
>> I regenerated the patch. Please check it.
>
> The logic for calculating how many objects to free is still wrong:
> mb_cache_shrink_fn returns the number of entries scaled by
> sysctl_vfs_cache_pressure / 100. It should also scale nr_to_scan by the
> inverse of that. The sysctl_vfs_cache_pressure == 0 case (never scale) may
> require special attention.
>
> See dcache_shrinker() in fs/dcache.c.
>
> Thanks,
> Andreas
>
Sorry, I haven't found any special attention on
sysctl_vfs_cache_pressure == 0 case or scale
nr_to_scan in fs/dcache.c
900static int shrink_dcache_memory(int nr, gfp_t gfp_mask)
901{
902 if (nr) {
903 if (!(gfp_mask & __GFP_FS))
904 return -1;
905 prune_dcache(nr);
906 }
907 return (dentry_stat.nr_unused / 100) * sysctl_vfs_cache_pressure;
908}
--
Thanks and Best Regards,
shenghui
--
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
ilto:"[email protected]"> [email protected] </a>
2010/7/20 shenghui <[email protected]>:
> 2010/7/20 Andreas Gruenbacher <[email protected]>:
>
> Sorry, I haven't found any special attention on
> sysctl_vfs_cache_pressure == 0 case or scale
> nr_to_scan in fs/dcache.c
>
> 900static int shrink_dcache_memory(int nr, gfp_t gfp_mask)
> 901{
> 902 if (nr) {
> 903 if (!(gfp_mask & __GFP_FS))
> 904 return -1;
> 905 prune_dcache(nr);
> 906 }
> 907 return (dentry_stat.nr_unused / 100) * sysctl_vfs_cache_pressure;
> 908}
>
And for sysctl_vfs_cache_pressure == 0 case, it's
enough to return 0 to indicate no cache entries left.
--
Thanks and Best Regards,
shenghui
Andreas Gruenbacher wrote:
> On Sunday 18 July 2010 08:36:59 Wang Sheng-Hui wrote:
>> I regenerated the patch. Please check it.
>
> The logic for calculating how many objects to free is still wrong:
> mb_cache_shrink_fn returns the number of entries scaled by
> sysctl_vfs_cache_pressure / 100. It should also scale nr_to_scan by the
> inverse of that. The sysctl_vfs_cache_pressure == 0 case (never scale) may
> require special attention.
I don't think that's right:
vfs_cache_pressure
------------------
Controls the tendency of the kernel to reclaim the memory which is used for
caching of directory and inode objects.
At the default value of vfs_cache_pressure=100 the kernel will attempt to
reclaim dentries and inodes at a "fair" rate with respect to pagecache and
swapcache reclaim. Decreasing vfs_cache_pressure causes the kernel to prefer
to retain dentry and inode caches. When vfs_cache_pressure=0, the kernel will
never reclaim dentries and inodes due to memory pressure and this can easily
lead to out-of-memory conditions. Increasing vfs_cache_pressure beyond 100
causes the kernel to prefer to reclaim dentries and inodes.
0 means "never reclaim," it doesn't mean "never scale."
As for nr_to_scan, after the first call, the shrinker has a scaled
version of the total count, so the requested nr_to_scan on the
next call is already scaled based on that.
I think the logic in the mbcache shrinker is fine.
-Eric
> See dcache_shrinker() in fs/dcache.c.
>
> Thanks,
> Andreas
--
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>
On Tuesday 20 July 2010 17:13:56 Eric Sandeen wrote:
> I think the logic in the mbcache shrinker is fine.
Indeed yes, I got confused, sorry.
On Sunday 18 July 2010 08:36:59 Wang Sheng-Hui wrote:
> I regenerated the patch. Please check it.
Sheng-Hui, the mb_cache_lru_list list is now accessed without holding
mb_cache_spinlock.
Thanks,
Andreas