All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ni zhan Chen <nizhan.chen@gmail.com>
To: Hugh Dickins <hughd@google.com>
Cc: Dave Jones <davej@redhat.com>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: shmem_getpage_gfp VM_BUG_ON triggered. [3.7rc2]
Date: Fri, 26 Oct 2012 09:48:17 +0800	[thread overview]
Message-ID: <5089EBE1.1050009@gmail.com> (raw)
In-Reply-To: <alpine.LNX.2.00.1210251419260.3623@eggly.anvils>

On 10/26/2012 05:27 AM, Hugh Dickins wrote:
> On Thu, 25 Oct 2012, Ni zhan Chen wrote:
>> On 10/25/2012 02:59 PM, Hugh Dickins wrote:
>>> On Thu, 25 Oct 2012, Ni zhan Chen wrote:
>>>> I think it maybe caused by your commit [d189922862e03ce: shmem: fix
>>>> negative
>>>> rss in memcg memory.stat], one question:
>>> Well, yes, I added the VM_BUG_ON in that commit.
>>>
>>>> if function shmem_confirm_swap confirm the entry has already brought back
>>>> from swap by a racing thread,
>>> The reverse: true confirms that the swap entry has not been brought back
>>> from swap by a racing thread; false indicates that there has been a race.
>>>
>>>> then why call shmem_add_to_page_cache to add
>>>> page from swapcache to pagecache again?
>>> Adding it to pagecache again, after such a race, would set error to
>>> -EEXIST (originating from radix_tree_insert); but we don't do that,
>>> we add it to pagecache when it has not already been added.
>>>
>>> Or that's the intention: but Dave seems to have found an unexpected
>>> exception, despite us holding the page lock across all this.
>>>
>>> (But if it weren't for the memcg and replace_page issues, I'd much
>>> prefer to let shmem_add_to_page_cache discover the race as before.)
>>>
>>> Hugh
>> Hi Hugh
>>
>> Thanks for your response. You mean the -EEXIST originating from
>> radix_tree_insert, in radix_tree_insert:
>> if (slot != NULL)
>>      return -EEXIST;
>> But why slot should be NULL? if no race, the pagecache related radix tree
>> entry should be RADIX_TREE_EXCEPTIONAL_ENTRY+swap_entry_t.val, where I miss?
> I was describing what would happen in a case that should not exist,
> that you had thought the common case.  In actuality, the entry should
> not be NULL, it should be as you say there.

Thanks for your patience. So in the common case, the entry should be the 
value I mentioned, then why has this check?
if (slot != NULL)
     return -EEXIST;

the common case will return -EEXIST.

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

WARNING: multiple messages have this Message-ID (diff)
From: Ni zhan Chen <nizhan.chen@gmail.com>
To: Hugh Dickins <hughd@google.com>
Cc: Dave Jones <davej@redhat.com>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: shmem_getpage_gfp VM_BUG_ON triggered. [3.7rc2]
Date: Fri, 26 Oct 2012 09:48:17 +0800	[thread overview]
Message-ID: <5089EBE1.1050009@gmail.com> (raw)
In-Reply-To: <alpine.LNX.2.00.1210251419260.3623@eggly.anvils>

On 10/26/2012 05:27 AM, Hugh Dickins wrote:
> On Thu, 25 Oct 2012, Ni zhan Chen wrote:
>> On 10/25/2012 02:59 PM, Hugh Dickins wrote:
>>> On Thu, 25 Oct 2012, Ni zhan Chen wrote:
>>>> I think it maybe caused by your commit [d189922862e03ce: shmem: fix
>>>> negative
>>>> rss in memcg memory.stat], one question:
>>> Well, yes, I added the VM_BUG_ON in that commit.
>>>
>>>> if function shmem_confirm_swap confirm the entry has already brought back
>>>> from swap by a racing thread,
>>> The reverse: true confirms that the swap entry has not been brought back
>>> from swap by a racing thread; false indicates that there has been a race.
>>>
>>>> then why call shmem_add_to_page_cache to add
>>>> page from swapcache to pagecache again?
>>> Adding it to pagecache again, after such a race, would set error to
>>> -EEXIST (originating from radix_tree_insert); but we don't do that,
>>> we add it to pagecache when it has not already been added.
>>>
>>> Or that's the intention: but Dave seems to have found an unexpected
>>> exception, despite us holding the page lock across all this.
>>>
>>> (But if it weren't for the memcg and replace_page issues, I'd much
>>> prefer to let shmem_add_to_page_cache discover the race as before.)
>>>
>>> Hugh
>> Hi Hugh
>>
>> Thanks for your response. You mean the -EEXIST originating from
>> radix_tree_insert, in radix_tree_insert:
>> if (slot != NULL)
>>      return -EEXIST;
>> But why slot should be NULL? if no race, the pagecache related radix tree
>> entry should be RADIX_TREE_EXCEPTIONAL_ENTRY+swap_entry_t.val, where I miss?
> I was describing what would happen in a case that should not exist,
> that you had thought the common case.  In actuality, the entry should
> not be NULL, it should be as you say there.

Thanks for your patience. So in the common case, the entry should be the 
value I mentioned, then why has this check?
if (slot != NULL)
     return -EEXIST;

the common case will return -EEXIST.

>
> Hugh
>


  reply	other threads:[~2012-10-26  1:48 UTC|newest]

Thread overview: 69+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-10-25  2:37 shmem_getpage_gfp VM_BUG_ON triggered. [3.7rc2] Dave Jones
2012-10-25  2:37 ` Dave Jones
2012-10-25  4:36 ` Hugh Dickins
2012-10-25  4:36   ` Hugh Dickins
2012-10-25  4:50   ` Ni zhan Chen
2012-10-25  4:50     ` Ni zhan Chen
2012-10-25  6:59     ` Hugh Dickins
2012-10-25  6:59       ` Hugh Dickins
2012-10-25  9:53       ` Ni zhan Chen
2012-10-25 10:21       ` Ni zhan Chen
2012-10-25 10:21         ` Ni zhan Chen
2012-10-25 21:27         ` Hugh Dickins
2012-10-25 21:27           ` Hugh Dickins
2012-10-26  1:48           ` Ni zhan Chen [this message]
2012-10-26  1:48             ` Ni zhan Chen
2012-10-25 11:14   ` Dave Jones
2012-10-25 11:14     ` Dave Jones
2012-10-25 21:28     ` Hugh Dickins
2012-10-25 21:28       ` Hugh Dickins
2012-10-25 20:52   ` Johannes Weiner
2012-10-25 20:52     ` Johannes Weiner
2012-10-25 21:48     ` Hugh Dickins
2012-10-25 21:48       ` Hugh Dickins
2012-10-26  2:15       ` Ni zhan Chen
2012-10-26  2:15         ` Ni zhan Chen
2012-11-01 19:10   ` Dave Jones
2012-11-01 19:10     ` Dave Jones
2012-11-01 23:03     ` Hugh Dickins
2012-11-01 23:03       ` Hugh Dickins
2012-11-01 23:20       ` Dave Jones
2012-11-01 23:20         ` Dave Jones
2012-11-01 23:48         ` Hugh Dickins
2012-11-01 23:48           ` Hugh Dickins
2012-11-02  1:43           ` Dave Jones
2012-11-02  1:43             ` Dave Jones
2012-11-02 23:26             ` Hugh Dickins
2012-11-02 23:26               ` Hugh Dickins
2012-11-06  1:32               ` [PATCH] tmpfs: fix shmem_getpage_gfp VM_BUG_ON Hugh Dickins
2012-11-06  1:32                 ` Hugh Dickins
2012-11-06 13:54                 ` Dave Jones
2012-11-06 13:54                   ` Dave Jones
2012-11-06 23:48                   ` Hugh Dickins
2012-11-06 23:48                     ` Hugh Dickins
2012-11-07 22:38                     ` Dave Jones
2012-11-07 22:38                       ` Dave Jones
2012-11-14  1:36                       ` [PATCH] tmpfs: fix shmem_getpage_gfp VM_BUG_ON fix Hugh Dickins
2012-11-14  1:36                         ` Hugh Dickins
2012-11-14  3:07                     ` [PATCH] tmpfs: fix shmem_getpage_gfp VM_BUG_ON Jaegeuk Hanse
2012-11-14  3:07                       ` Jaegeuk Hanse
2012-11-14  3:50                       ` Hugh Dickins
2012-11-14  3:50                         ` Hugh Dickins
2012-11-14  6:14                         ` Dave Jones
2012-11-14  6:14                           ` Dave Jones
2012-11-14 10:06                           ` Hugh Dickins
2012-11-14 10:06                             ` Hugh Dickins
2012-11-15  7:39                         ` Jaegeuk Hanse
2012-11-15  7:39                           ` Jaegeuk Hanse
2012-11-15 19:56                           ` Hugh Dickins
2012-11-15 19:56                             ` Hugh Dickins
2012-11-16  0:40                             ` Jaegeuk Hanse
2012-11-16  0:40                               ` Jaegeuk Hanse
2012-11-16  9:34                             ` Jaegeuk Hanse
2012-11-16  9:34                               ` Jaegeuk Hanse
2012-11-17  4:48                               ` Hugh Dickins
2012-11-17  4:48                                 ` Hugh Dickins
2012-11-18  0:57                                 ` Jaegeuk Hanse
2012-11-18  0:57                                   ` Jaegeuk Hanse
2012-11-18  1:48                                 ` Jaegeuk Hanse
2012-11-18  1:48                                   ` Jaegeuk Hanse

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5089EBE1.1050009@gmail.com \
    --to=nizhan.chen@gmail.com \
    --cc=davej@redhat.com \
    --cc=hughd@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.