linux-kernel - Re: [v3 PATCH] mm: move_pages: return valid node id in status if the page is already on the target node

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9588b886-7ced-6502-67f0-0eb623045903@linux.alibaba.com>
Date:   Thu, 5 Dec 2019 17:11:58 -0800
From:   Yang Shi <yang.shi@...ux.alibaba.com>
To:     Qian Cai <cai@....pw>, John Hubbard <jhubbard@...dia.com>
Cc:     fabecassis@...dia.com, mhocko@...e.com, cl@...ux.com,
        vbabka@...e.cz, mgorman@...hsingularity.net,
        akpm@...ux-foundation.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [v3 PATCH] mm: move_pages: return valid node id in status if the
 page is already on the target node

On 12/5/19 4:19 PM, Qian Cai wrote:
>
>> On Dec 5, 2019, at 7:04 PM, John Hubbard <jhubbard@...dia.com> wrote:
>>
>> Felix's code is not random test code. It's code he wrote and he expected it to work.
> Sure, but could he show a bit detail if the kernel will start to behavior as expected like what was written in the manpage to return ENOENT in this case, why is it not acceptable? i.e., why is it important to depend on the broken behavior?

I think I found the root cause. It did return -ENOENT when the syscall 
was introduced (my first impression was wrong), but the behavior was 
changed since 2.6.28 by commit e78bbfa82624 ("mm: stop returning -ENOENT 
from sys_move_pages() if nothing got migrated"). The full commit log is 
as below:

Author: Brice Goglin <Brice.Goglin@...ia.fr>
Date:   Sat Oct 18 20:27:15 2008 -0700

     mm: stop returning -ENOENT from sys_move_pages() if nothing got 
migrated

     A patchset reworking sys_move_pages().  It removes the possibly large
     vmalloc by using multiple chunks when migrating large buffers. It also
     dramatically increases the throughput for large buffers since the 
lookup
     in new_page_node() is now limited to a single chunk, causing the 
quadratic
     complexity to have a much slower impact.  There is no need to use any
     radix-tree-like structure to improve this lookup.

     sys_move_pages() duration on a 4-quadcore-opteron 2347HE (1.9Gz),
     migrating between nodes #2 and #3:

             length          move_pages (us)         move_pages+patch (us)
             4kB             126                     98
             40kB            198                     168
             400kB           963                     937
             4MB             12503                   11930
             40MB            246867                  11848

     Patches #1 and #4 are the important ones:
     1) stop returning -ENOENT from sys_move_pages() if nothing got migrated
     2) don't vmalloc a huge page_to_node array for do_pages_stat()
     3) extract do_pages_move() out of sys_move_pages()
     4) rework do_pages_move() to work on page_sized chunks
     5) move_pages: no need to set pp->page to ZERO_PAGE(0) by default

     This patch:

     There is no point in returning -ENOENT from sys_move_pages() if all 
pages
     were already on the right node, while we return 0 if only 1 page 
was not.
     Most application don't know where their pages are allocated, so 
it's not
     an error to try to migrate them anyway.

     Just return 0 and let the status array in user-space be checked if the
     application needs details.

     It will make the upcoming chunked-move_pages() support much easier.

     Signed-off-by: Brice Goglin <Brice.Goglin@...ia.fr>
     Acked-by: Christoph Lameter <cl@...ux-foundation.org>
     Cc: Nick Piggin <nickpiggin@...oo.com.au>
     Signed-off-by: Andrew Morton <akpm@...ux-foundation.org>
     Signed-off-by: Linus Torvalds <torvalds@...ux-foundation.org>

So the behavior was changed in kernel intentionally but never reflected 
in the manpage. I will propose a patch to fix the document.