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.)
On Wed, 14 Jun 2023 at 19:10, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/14/23 8:14 PM, Michał Mirosław wrote: > On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum > usama.anjum@collabora.com wrote: >> >> On 6/14/23 3:36 AM, Michał Mirosław wrote: >>> On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum >>> usama.anjum@collabora.com wrote:
[...]
>>>> + if (cur_buf->bitmap == bitmap && >>>> + cur_buf->start + cur_buf->len * PAGE_SIZE == addr) { >>>> + cur_buf->len += n_pages; >>>> + p->found_pages += n_pages; >>>> + } else { >>>> + if (cur_buf->len && p->vec_buf_index >= p->vec_buf_len) >>>> + return -ENOMEM; >>> >>> Shouldn't this be -ENOSPC? -ENOMEM usually signifies that the kernel >>> ran out of memory when allocating, not that there is no space in a >>> user-provided buffer. >> There are 3 kinds of return values here: >> * PM_SCAN_FOUND_MAX_PAGES (1) ---> max_pages have been found. Abort the >> page walk from next entry >> * 0 ---> continue the page walk >> * -ENOMEM --> Abort the page walk from current entry, user buffer is full >> which is not error, but only a stop signal. This -ENOMEM is just >> differentiater from (1). This -ENOMEM is for internal use and isn't >> returned to user. > > But why ENOSPC is not good here? I was used before, I think. -ENOSPC is being returned in form of true error from pagemap_scan_hugetlb_entry(). So I'd to remove -ENOSPC from here as it wasn't true error here, it was only a way to abort the walk immediately. I'm liking the following erturn code from here now:
#define PM_SCAN_BUFFER_FULL (-256)
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;
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;
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
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.
> While here, I wonder if we really need to fail the call if there are > unknown bits in those masks set: if this bit set is expanded with > another category flags, a newer userspace run on older kernel would > get EINVAL even if the "treat unknown as 0" be what it requires. > There is no simple way in the API to discover what bits the kernel > supports. We could allow a no-op (no WP nor GET) call to help with > that and then rejecting unknown bits would make sense. I've not seen any examples of this. But I've seen examples of returning error if kernel doesn't support a feature. Each new feature comes with a kernel version, greater than this version support this feature. If user is trying to use advanced feature which isn't present in a kernel, we should return error and not proceed to confuse the user/kernel. In fact if we look at userfaultfd_api(), we return error immediately if feature has some bit set which kernel doesn't support.
I think we should have a way of detecting the supported flags if we don't want a forward compatibility policy for flags here. Maybe it would be enough to allow all the no-op combinations for this purpose?
Again I don't think UFFD is doing anything like this.
If it's cheap and easy to provide a user with a way to detect the supported features - why not do it?
I'm sorry. But it would bring up something new and iterations will be needed to just play around. I like the UFFD way.
Let's then first agree on what would have to be changed. I guess we could leverage that `scan_len = 0` doesn't make much sense otherwise and let it be used to check the other fields for support.
We are making things more and more complex. I don't like multi-plexing variables. Can you give examples where multi-plexing has been done on variables inside linux kernel? Muti-plexing means user gives input and takes output from same variable. It makes variable double meaning.
Best Regards Michał Mirosław