Re: And another patch...

[Sergey Bugaev]

On Sat, May 8, 2021 at 2:36 PM Samuel Thibault <samuel.thibault@gnu.org> wrote:
>
> Hello,
>
> Just to be sure: this is not only because of your previous patch
> libpager: Do not flush in-core pages on offer
> ?

Yes; this issue is completely orthogonal to that fix (but there's an
interesting interaction I haven't thought about, see below)

This issue is about the Mach mechanism of "precious pages". Namely,
when offering a page to the kernel (with memory_object_data_supply ()
or pager_offer_page () which wraps it), a memory manager can tell the
kernel that the page is "precious". This means the kernel should not
silently drop the page when the kernel no longer needs the page, even
if the page is not dirtied (written to by clients) in the meantime.
The intended use case is the memory manager supplying the kernel with
the actual page where it stores its data, not a copy from some other
sort of storage. The kernel will eventually return the page through
memory_object_data_return ().

Now, offering precious pages that are already in core (this is where
that other patch is relevant) will result in KERN_MEMORY_PRESENT error
being delivered via memory_object_supply_completed ()... which
libpager currently ignores. So that might be something to fix, too.
Thanks!

Now, for something slightly different:

> @@ -205,14 +190,13 @@ _pager_do_write_request (struct pager *p,
>         }
>        else
>         {
> -         munmap ((void *) (data + (vm_page_size * i)),
> -                 vm_page_size);
>           notified[i] = (! kcopy && p->notify_on_evict);
>           if (! kcopy)
>             pm_entries[i] &= ~PM_INCORE;
>        }

I've included this in the patch up-thread; it definitely should have
been a separate patch; and now I'm doubting if it's a right change at
all.

Namely, pager_write_page () is documented to "In addition, mfree (or
equivalent) BUF." I don't know what mfree is, but I assume this means
munmap () or vm_deallocate (). In other words, pager_write_page () is
documented to _assume ownership_ of the passed in page. So it would
seem like _pager_do_write_request () *also* trying to unmap the page
after the pager_write_page () calls is wrong (and may even lead to
accidentally unmapping an unrelated page), which is why I removed it.

However.

The other branch in _pager_do_write_request () tries to return the
page back to the kernel, so it definitely assumes that the page is
still valid/available. And looking at various pager_write_page ()
implementations throughout the Hurd, some do deallocate the page
(storeio), but most don't (defpager, ext2fs, fatfs).

So overall, it would appear that pager_write_page () is actually *not*
supposed to deallocate the page. If that's correct, storeio (and
potentially other users outside of the main tree?) needs changing, and
the documentation for pager_write_page () needs to be fixed.

What do you think?

Sergey