On 6/19/23 1:16 PM, Michał Mirosław wrote:
On Fri, 16 Jun 2023 at 08:57, Muhammad Usama Anjum usama.anjum@collabora.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@collabora.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@collabora.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