Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7aee68e9-6e31-925f-68bc-73557c032a42@redhat.com>
Date:   Thu, 23 Mar 2023 11:38:52 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH v7 4/4] mm: vmalloc: convert vread() to vread_iter()
Content-Language: en-US
To:     Baoquan He <bhe@redhat.com>, Lorenzo Stoakes <lstoakes@gmail.com>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        linux-fsdevel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Uladzislau Rezki <urezki@gmail.com>,
        Matthew Wilcox <willy@infradead.org>,
        Liu Shixin <liushixin2@huawei.com>,
        Jiri Olsa <jolsa@kernel.org>, Jens Axboe <axboe@kernel.dk>,
        Alexander Viro <viro@zeniv.linux.org.uk>
References: <cover.1679511146.git.lstoakes@gmail.com>
 <941f88bc5ab928e6656e1e2593b91bf0f8c81e1b.1679511146.git.lstoakes@gmail.com>
 <ZBu+2cPCQvvFF/FY@MiWiFi-R3L-srv>
 <ff630c2e-42ff-42ec-9abb-38922d5107ec@lucifer.local>
 <ZBwroYh22pEqJYhv@MiWiFi-R3L-srv>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
In-Reply-To: <ZBwroYh22pEqJYhv@MiWiFi-R3L-srv>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 23.03.23 11:36, Baoquan He wrote:
> On 03/23/23 at 06:44am, Lorenzo Stoakes wrote:
>> On Thu, Mar 23, 2023 at 10:52:09AM +0800, Baoquan He wrote:
>>> On 03/22/23 at 06:57pm, Lorenzo Stoakes wrote:
>>>> Having previously laid the foundation for converting vread() to an iterator
>>>> function, pull the trigger and do so.
>>>>
>>>> This patch attempts to provide minimal refactoring and to reflect the
>>>> existing logic as best we can, for example we continue to zero portions of
>>>> memory not read, as before.
>>>>
>>>> Overall, there should be no functional difference other than a performance
>>>> improvement in /proc/kcore access to vmalloc regions.
>>>>
>>>> Now we have eliminated the need for a bounce buffer in read_kcore_iter(),
>>>> we dispense with it, and try to write to user memory optimistically but
>>>> with faults disabled via copy_page_to_iter_nofault(). We already have
>>>> preemption disabled by holding a spin lock. We continue faulting in until
>>>> the operation is complete.
>>>
>>> I don't understand the sentences here. In vread_iter(), the actual
>>> content reading is done in aligned_vread_iter(), otherwise we zero
>>> filling the region. In aligned_vread_iter(), we will use
>>> vmalloc_to_page() to get the mapped page and read out, otherwise zero
>>> fill. While in this patch, fault_in_iov_iter_writeable() fault in memory
>>> of iter one time and will bail out if failed. I am wondering why we
>>> continue faulting in until the operation is complete, and how that is done.
>>
>> This is refererrring to what's happening in kcore.c, not vread_iter(),
>> i.e. the looped read/faultin.
>>
>> The reason we bail out if failt_in_iov_iter_writeable() is that would
>> indicate an error had occurred.
>>
>> The whole point is to _optimistically_ try to perform the operation
>> assuming the pages are faulted in. Ultimately we fault in via
>> copy_to_user_nofault() which will either copy data or fail if the pages are
>> not faulted in (will discuss this below a bit more in response to your
>> other point).
>>
>> If this fails, then we fault in, and try again. We loop because there could
>> be some extremely unfortunate timing with a race on e.g. swapping out or
>> migrating pages between faulting in and trying to write out again.
>>
>> This is extremely unlikely, but to avoid any chance of breaking userland we
>> repeat the operation until it completes. In nearly all real-world
>> situations it'll either work immediately or loop once.
> 
> Thanks a lot for these helpful details with patience. I got it now. I was
> mainly confused by the while(true) loop in KCORE_VMALLOC case of read_kcore_iter.
> 
> Now is there any chance that the faulted in memory is swapped out or
> migrated again before vread_iter()? fault_in_iov_iter_writeable() will
> pin the memory? I didn't find it from code and document. Seems it only
> falults in memory. If yes, there's window between faluting in and
> copy_to_user_nofault().
> 

See the documentation of fault_in_safe_writeable():

"Note that we don't pin or otherwise hold the pages referenced that we 
fault in.  There's no guarantee that they'll stay in memory for any 
duration of time."

-- 
Thanks,

David / dhildenb