Message-ID: <444ed144-a2ee-cb16-880a-128383c83a08@collabora.com>
Date:   Tue, 20 Jun 2023 16:15:55 +0500
From:   Muhammad Usama Anjum <usama.anjum@...labora.com>
To:     Michał Mirosław <emmir@...gle.com>
Cc:     Muhammad Usama Anjum <usama.anjum@...labora.com>,
        Peter Xu <peterx@...hat.com>,
        David Hildenbrand <david@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Andrei Vagin <avagin@...il.com>,
        Danylo Mocherniuk <mdanylo@...gle.com>,
        Paul Gofman <pgofman@...eweavers.com>,
        Cyrill Gorcunov <gorcunov@...il.com>,
        Mike Rapoport <rppt@...nel.org>, Nadav Amit <namit@...are.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Shuah Khan <shuah@...nel.org>,
        Christian Brauner <brauner@...nel.org>,
        Yang Shi <shy828301@...il.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>,
        Yun Zhou <yun.zhou@...driver.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Alex Sierra <alex.sierra@....com>,
        Matthew Wilcox <willy@...radead.org>,
        Pasha Tatashin <pasha.tatashin@...een.com>,
        Axel Rasmussen <axelrasmussen@...gle.com>,
        "Gustavo A . R . Silva" <gustavoars@...nel.org>,
        Dan Williams <dan.j.williams@...el.com>,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-mm@...ck.org, linux-kselftest@...r.kernel.org,
        Greg KH <gregkh@...uxfoundation.org>, kernel@...labora.com
Subject: Re: [PATCH v18 2/5] fs/proc/task_mmu: Implement IOCTL to get and
 optionally clear info about PTEs

On 6/19/23 1:16 PM, Michał Mirosław wrote:
> On Fri, 16 Jun 2023 at 08:57, Muhammad Usama Anjum
> <usama.anjum@...labora.com> wrote:
>>
>> On 6/16/23 1:07 AM, Michał Mirosław wrote:
>>> On Thu, 15 Jun 2023 at 17:11, Muhammad Usama Anjum
>>> <usama.anjum@...labora.com> wrote:
>>>> On 6/15/23 7:52 PM, Michał Mirosław wrote:
>>>>> On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum
>>>>> <usama.anjum@...labora.com> wrote:
>>>>>> I'll send next revision now.
>>>>>> On 6/14/23 11:00 PM, Michał Mirosław wrote:
>>>>>>> (A quick reply to answer open questions in case they help the next version.)
> [...]
>>>>>>> I guess this will be reworked anyway, but I'd prefer this didn't need
>>>>>>> custom errors etc. If we agree to decoupling the selection and GET
>>>>>>> output, it could be:
>>>>>>>
>>>>>>> bool is_interesting_page(p, flags); // this one does the
>>>>>>> required/anyof/excluded match
>>>>>>> size_t output_range(p, start, len, flags); // this one fills the
>>>>>>> output vector and returns how many pages were fit
>>>>>>>
>>>>>>> In this setup, `is_interesting_page() && (n_out = output_range()) <
>>>>>>> n_pages` means this is the final range, no more will fit. And if
>>>>>>> `n_out == 0` then no pages fit and no WP is needed (no other special
>>>>>>> cases).
>>>>>> Right now, pagemap_scan_output() performs the work of both of these two
>>>>>> functions. The part can be broken into is_interesting_pages() and we can
>>>>>> leave the remaining part as it is.
>>>>>>
>>>>>> Saying that n_out < n_pages tells us the buffer is full covers one case.
>>>>>> But there is case of maximum pages have been found and walk needs to be
>>>>>> aborted.
>>>>>
>>>>> This case is exactly what `n_out < n_pages` will cover (if scan_output
>>>>> uses max_pages properly to limit n_out).
>>>>> Isn't it that when the buffer is full we want to abort the scan always
>>>>> (with WP if `n_out > 0`)?
>>>> Wouldn't it be duplication of condition if buffer is full inside
>>>> pagemap_scan_output() and just outside it. Inside pagemap_scan_output() we
>>>> check if we have space before putting data inside it. I'm using this same
>>>> condition to indicate that buffer is full.
>>>
>>> I'm not sure what do you mean? The buffer-full conditions would be
>>> checked in ..scan_output() and communicated to the caller by returning
>>> N less than `n_pages` passed in. This is exactly how e.g. read()
>>> works: if you get less than requested you've hit the end of the file.
>>> If the file happens to have size that is equal to the provided buffer
>>> length, the next read() will return 0.
>> Right now we have:
>>
>> pagemap_scan_output():
>>         if (p->vec_buf_index >= p->vec_buf_len)
>>                 return PM_SCAN_BUFFER_FULL;
>>         if (p->found_pages == p->max_pages)
>>                 return PM_SCAN_FOUND_MAX_PAGES;
> 
> Why do you need to differentiate between those cases?
> 
>> pagemap_scan_pmd_entry():
>>         ret = pagemap_scan_output(bitmap, p, start, n_pages);
>>         if (ret >= 0) // success
>>                 make_UFFD_WP and flush
>>         else
>>                 buffer_error
>>
>> You are asking me to do:
>>
>> pagemap_scan_output():
>>         if (p->vec_buf_index >= p->vec_buf_len)
>>                 return 0;
> 
>>         if (p->found_pages == p->max_pages)
>>                 return PM_SCAN_FOUND_MAX_PAGES;
> 
> This should be instead:
> 
> n_pages = min(p->max_pags - p_found_pages, n_pages)
> ...
> return n_pages;
You are missing the optimization here that we check for full buffer every
time adding to user buffer. This was added to remove extra iteration of
page walk if buffer is full already. The way you are suggesting will remove it.

So you are returning remaining pages to be found now. This doesn't seem
right. If max_pages is 520, found_pages is 0 and n_pages is 512 before
calling pagemap_scan_output(). found_pages would become 512 after adding
512 pages to output buffer. But n_pages would return 8 instead of 512. You
were saying we should return the number of pages added to the output buffer.

> 
>> pagemap_scan_pmd_entry():
>>         ret = pagemap_scan_output(bitmap, p, start, n_pages);
>>         if (ret > 0) // success
>>                 make_UFFD_WP and flush
>>         else if (ret == 0) // buffer full
>>                 return PM_SCAN_BUFFER_FULL;
>>         else //other errors
>>                 buffer_error
> 
> And this would be:
> 
> if (ret > 0 && WP)
>    WP + flush
> 
> if (ret < n_pages)
>    return -ENOSPC;
> 
>> So you are asking me to go from consie code to write more lines of code. I
>> would write more lines without any issue if it improves readability and
>> logical sense. But I don't see here any benefit.
> 
> Please see the clarifications above.
> 
> Best Regards
> Michał Mirosław

-- 
BR,
Muhammad Usama Anjum

Powered by blists - more mailing lists