* [PATCH mmotm/next] mm: memcontrol: rewrite charge API: fix shmem_unuse
@ 2014-06-30 22:48 Hugh Dickins
2014-06-30 23:02 ` Andrew Morton
2014-07-02 22:37 ` Johannes Weiner
0 siblings, 2 replies; 6+ messages in thread
From: Hugh Dickins @ 2014-06-30 22:48 UTC (permalink / raw)
To: Johannes Weiner; +Cc: Andrew Morton, Michal Hocko, linux-mm, linux-kernel
Under shmem swapping and swapoff load, I sometimes hit the
VM_BUG_ON_PAGE(!page->mapping) in mem_cgroup_commit_charge() at
mm/memcontrol.c:6502! Each time it has been a call from shmem_unuse().
Yes, there are some cases (most commonly when the page being unswapped
is in a file being unlinked and evicted at that time) when the charge
should not be committed. In the old scheme, the page got uncharged
again on release; but in the new scheme, it hits that BUG beforehand.
It's a useful BUG, so adapt shmem_unuse() to allow for it. Which needs
more info from shmem_unuse_inode(): so abuse -EAGAIN internally to
replace the previous !found state (-ENOENT would be a more natural
code, but that's exactly what you get when the swap has been evicted).
Signed-off-by: Hugh Dickins <hughd@google.com>
---
mm/shmem.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
--- 3.16-rc2-mm1/mm/shmem.c 2014-06-25 18:43:59.868588121 -0700
+++ linux/mm/shmem.c 2014-06-30 15:05:50.736335600 -0700
@@ -611,7 +611,7 @@ static int shmem_unuse_inode(struct shme
radswap = swp_to_radix_entry(swap);
index = radix_tree_locate_item(&mapping->page_tree, radswap);
if (index == -1)
- return 0;
+ return -EAGAIN;
/*
* Move _head_ to start search for next from here.
@@ -670,7 +670,6 @@ static int shmem_unuse_inode(struct shme
spin_unlock(&info->lock);
swap_free(swap);
}
- error = 1; /* not an error, but entry was found */
}
return error;
}
@@ -683,7 +682,6 @@ int shmem_unuse(swp_entry_t swap, struct
struct list_head *this, *next;
struct shmem_inode_info *info;
struct mem_cgroup *memcg;
- int found = 0;
int error = 0;
/*
@@ -702,22 +700,24 @@ int shmem_unuse(swp_entry_t swap, struct
if (error)
goto out;
/* No radix_tree_preload: swap entry keeps a place for page in tree */
+ error = -EAGAIN;
mutex_lock(&shmem_swaplist_mutex);
list_for_each_safe(this, next, &shmem_swaplist) {
info = list_entry(this, struct shmem_inode_info, swaplist);
if (info->swapped)
- found = shmem_unuse_inode(info, swap, &page);
+ error = shmem_unuse_inode(info, swap, &page);
else
list_del_init(&info->swaplist);
cond_resched();
- if (found)
+ if (error != -EAGAIN)
break;
}
mutex_unlock(&shmem_swaplist_mutex);
- if (found < 0) {
- error = found;
+ if (error) {
+ if (error != -ENOMEM)
+ error = 0;
mem_cgroup_cancel_charge(page, memcg);
} else
mem_cgroup_commit_charge(page, memcg, true);
--
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH mmotm/next] mm: memcontrol: rewrite charge API: fix shmem_unuse
2014-06-30 22:48 [PATCH mmotm/next] mm: memcontrol: rewrite charge API: fix shmem_unuse Hugh Dickins
@ 2014-06-30 23:02 ` Andrew Morton
2014-07-01 0:10 ` Hugh Dickins
2014-07-02 22:37 ` Johannes Weiner
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2014-06-30 23:02 UTC (permalink / raw)
To: Hugh Dickins; +Cc: Johannes Weiner, Michal Hocko, linux-mm, linux-kernel
On Mon, 30 Jun 2014 15:48:39 -0700 (PDT) Hugh Dickins <hughd@google.com> wrote:
> Under shmem swapping and swapoff load, I sometimes hit the
> VM_BUG_ON_PAGE(!page->mapping) in mem_cgroup_commit_charge() at
> mm/memcontrol.c:6502! Each time it has been a call from shmem_unuse().
>
> Yes, there are some cases (most commonly when the page being unswapped
> is in a file being unlinked and evicted at that time) when the charge
> should not be committed. In the old scheme, the page got uncharged
> again on release; but in the new scheme, it hits that BUG beforehand.
>
> It's a useful BUG, so adapt shmem_unuse() to allow for it. Which needs
> more info from shmem_unuse_inode(): so abuse -EAGAIN internally to
> replace the previous !found state (-ENOENT would be a more natural
> code, but that's exactly what you get when the swap has been evicted).
>
> ...
>
> --- 3.16-rc2-mm1/mm/shmem.c 2014-06-25 18:43:59.868588121 -0700
> +++ linux/mm/shmem.c 2014-06-30 15:05:50.736335600 -0700
> @@ -611,7 +611,7 @@ static int shmem_unuse_inode(struct shme
> radswap = swp_to_radix_entry(swap);
> index = radix_tree_locate_item(&mapping->page_tree, radswap);
> if (index == -1)
> - return 0;
> + return -EAGAIN;
Maybe it's time to document the shmem_unuse_inode() return values.
> /*
> * Move _head_ to start search for next from here.
> @@ -670,7 +670,6 @@ static int shmem_unuse_inode(struct shme
> spin_unlock(&info->lock);
> swap_free(swap);
> }
> - error = 1; /* not an error, but entry was found */
> }
> return error;
> }
> @@ -683,7 +682,6 @@ int shmem_unuse(swp_entry_t swap, struct
> struct list_head *this, *next;
> struct shmem_inode_info *info;
> struct mem_cgroup *memcg;
> - int found = 0;
> int error = 0;
>
> /*
> @@ -702,22 +700,24 @@ int shmem_unuse(swp_entry_t swap, struct
> if (error)
> goto out;
> /* No radix_tree_preload: swap entry keeps a place for page in tree */
> + error = -EAGAIN;
>
> mutex_lock(&shmem_swaplist_mutex);
> list_for_each_safe(this, next, &shmem_swaplist) {
> info = list_entry(this, struct shmem_inode_info, swaplist);
> if (info->swapped)
> - found = shmem_unuse_inode(info, swap, &page);
> + error = shmem_unuse_inode(info, swap, &page);
> else
> list_del_init(&info->swaplist);
> cond_resched();
> - if (found)
> + if (error != -EAGAIN)
> break;
> }
> mutex_unlock(&shmem_swaplist_mutex);
>
> - if (found < 0) {
> - error = found;
> + if (error) {
> + if (error != -ENOMEM)
> + error = 0;
> mem_cgroup_cancel_charge(page, memcg);
> } else
> mem_cgroup_commit_charge(page, memcg, true);
If I'm reading this correctly, shmem_unuse() can now return -EAGAIN and
that can get all the way back to userspace. `man 2 swapoff' doesn't
know this...
--
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH mmotm/next] mm: memcontrol: rewrite charge API: fix shmem_unuse
2014-06-30 23:02 ` Andrew Morton
@ 2014-07-01 0:10 ` Hugh Dickins
2014-07-01 0:34 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Hugh Dickins @ 2014-07-01 0:10 UTC (permalink / raw)
To: Andrew Morton
Cc: Hugh Dickins, Johannes Weiner, Michal Hocko, linux-mm,
linux-kernel
On Mon, 30 Jun 2014, Andrew Morton wrote:
> On Mon, 30 Jun 2014 15:48:39 -0700 (PDT) Hugh Dickins <hughd@google.com> wrote:
> > - return 0;
> > + return -EAGAIN;
>
> Maybe it's time to document the shmem_unuse_inode() return values.
Oh dear. I had hoped they would look after themselves. This one is a
private matter between shmem_unuse_inode and its one caller, just below.
> > + if (error) {
> > + if (error != -ENOMEM)
> > + error = 0;
> > mem_cgroup_cancel_charge(page, memcg);
> > } else
> > mem_cgroup_commit_charge(page, memcg, true);
>
> If I'm reading this correctly, shmem_unuse() can now return -EAGAIN and
> that can get all the way back to userspace. `man 2 swapoff' doesn't
> know this...
if (error) {
if (error != -ENOMEM)
error = 0;
...
return error;
So the only values returned from shmem_unuse_inode() to its caller
try_to_unuse() are 0 and -ENOMEM. Those may get passed back to the
user, but -EAGAIN was just an internal shmem.c detail.
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH mmotm/next] mm: memcontrol: rewrite charge API: fix shmem_unuse
2014-07-01 0:10 ` Hugh Dickins
@ 2014-07-01 0:34 ` Andrew Morton
2014-07-01 1:06 ` Hugh Dickins
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2014-07-01 0:34 UTC (permalink / raw)
To: Hugh Dickins; +Cc: Johannes Weiner, Michal Hocko, linux-mm, linux-kernel
On Mon, 30 Jun 2014 17:10:54 -0700 (PDT) Hugh Dickins <hughd@google.com> wrote:
> On Mon, 30 Jun 2014, Andrew Morton wrote:
> > On Mon, 30 Jun 2014 15:48:39 -0700 (PDT) Hugh Dickins <hughd@google.com> wrote:
> > > - return 0;
> > > + return -EAGAIN;
> >
> > Maybe it's time to document the shmem_unuse_inode() return values.
>
> Oh dear. I had hoped they would look after themselves. This one is a
> private matter between shmem_unuse_inode and its one caller, just below.
Well, readers of shmem_unuse_inode() won't know that unless we tell them.
> > > + if (error) {
> > > + if (error != -ENOMEM)
> > > + error = 0;
> > > mem_cgroup_cancel_charge(page, memcg);
> > > } else
> > > mem_cgroup_commit_charge(page, memcg, true);
> >
> > If I'm reading this correctly, shmem_unuse() can now return -EAGAIN and
> > that can get all the way back to userspace. `man 2 swapoff' doesn't
> > know this...
>
> if (error) {
> if (error != -ENOMEM)
> error = 0;
> ...
> return error;
>
> So the only values returned from shmem_unuse_inode() to its caller
> try_to_unuse() are 0 and -ENOMEM. Those may get passed back to the
> user, but -EAGAIN was just an internal shmem.c detail.
OK.
--
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH mmotm/next] mm: memcontrol: rewrite charge API: fix shmem_unuse
2014-07-01 0:34 ` Andrew Morton
@ 2014-07-01 1:06 ` Hugh Dickins
0 siblings, 0 replies; 6+ messages in thread
From: Hugh Dickins @ 2014-07-01 1:06 UTC (permalink / raw)
To: Andrew Morton
Cc: Hugh Dickins, Johannes Weiner, Michal Hocko, linux-mm,
linux-kernel
On Mon, 30 Jun 2014, Andrew Morton wrote:
> On Mon, 30 Jun 2014 17:10:54 -0700 (PDT) Hugh Dickins <hughd@google.com> wrote:
> > On Mon, 30 Jun 2014, Andrew Morton wrote:
> > > On Mon, 30 Jun 2014 15:48:39 -0700 (PDT) Hugh Dickins <hughd@google.com> wrote:
> > > > - return 0;
> > > > + return -EAGAIN;
> > >
> > > Maybe it's time to document the shmem_unuse_inode() return values.
> >
> > Oh dear. I had hoped they would look after themselves. This one is a
> > private matter between shmem_unuse_inode and its one caller, just below.
>
> Well, readers of shmem_unuse_inode() won't know that unless we tell them.
Add comments on the private use of -EAGAIN.
Signed-off-by: Hugh Dickins <hughd@google.com>
---
mm/shmem.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- 3.16-rc2-mm1+/mm/shmem.c 2014-06-30 15:05:50.736335600 -0700
+++ linux/mm/shmem.c 2014-06-30 18:00:02.820584009 -0700
@@ -611,7 +611,7 @@ static int shmem_unuse_inode(struct shme
radswap = swp_to_radix_entry(swap);
index = radix_tree_locate_item(&mapping->page_tree, radswap);
if (index == -1)
- return -EAGAIN;
+ return -EAGAIN; /* tell shmem_unuse we found nothing */
/*
* Move _head_ to start search for next from here.
@@ -712,6 +712,7 @@ int shmem_unuse(swp_entry_t swap, struct
cond_resched();
if (error != -EAGAIN)
break;
+ /* found nothing in this: move on to search the next */
}
mutex_unlock(&shmem_swaplist_mutex);
--
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH mmotm/next] mm: memcontrol: rewrite charge API: fix shmem_unuse
2014-06-30 22:48 [PATCH mmotm/next] mm: memcontrol: rewrite charge API: fix shmem_unuse Hugh Dickins
2014-06-30 23:02 ` Andrew Morton
@ 2014-07-02 22:37 ` Johannes Weiner
1 sibling, 0 replies; 6+ messages in thread
From: Johannes Weiner @ 2014-07-02 22:37 UTC (permalink / raw)
To: Hugh Dickins; +Cc: Andrew Morton, Michal Hocko, linux-mm, linux-kernel
On Mon, Jun 30, 2014 at 03:48:39PM -0700, Hugh Dickins wrote:
> Under shmem swapping and swapoff load, I sometimes hit the
> VM_BUG_ON_PAGE(!page->mapping) in mem_cgroup_commit_charge() at
> mm/memcontrol.c:6502! Each time it has been a call from shmem_unuse().
>
> Yes, there are some cases (most commonly when the page being unswapped
> is in a file being unlinked and evicted at that time) when the charge
> should not be committed. In the old scheme, the page got uncharged
> again on release; but in the new scheme, it hits that BUG beforehand.
>
> It's a useful BUG, so adapt shmem_unuse() to allow for it. Which needs
> more info from shmem_unuse_inode(): so abuse -EAGAIN internally to
> replace the previous !found state (-ENOENT would be a more natural
> code, but that's exactly what you get when the swap has been evicted).
>
> Signed-off-by: Hugh Dickins <hughd@google.com>
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
Thanks, 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>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-07-02 22:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-06-30 22:48 [PATCH mmotm/next] mm: memcontrol: rewrite charge API: fix shmem_unuse Hugh Dickins
2014-06-30 23:02 ` Andrew Morton
2014-07-01 0:10 ` Hugh Dickins
2014-07-01 0:34 ` Andrew Morton
2014-07-01 1:06 ` Hugh Dickins
2014-07-02 22:37 ` Johannes Weiner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).