Am 11.12.2020 um 22:36 schrieb Pavel Tatashin [off-list ref]:

On Fri, Dec 11, 2020 at 4:29 PM David Hildenbrand [off-list ref] wrote:

quoted

quoted

quoted

Am 11.12.2020 um 22:09 schrieb Pavel Tatashin [off-list ref]:

On Fri, Dec 11, 2020 at 3:46 PM Jason Gunthorpe [off-list ref] wrote:

quoted

quoted

On Fri, Dec 11, 2020 at 03:40:57PM -0500, Pavel Tatashin wrote:
On Fri, Dec 11, 2020 at 3:23 PM Jason Gunthorpe [off-list ref] wrote:

quoted

On Fri, Dec 11, 2020 at 03:21:39PM -0500, Pavel Tatashin wrote:

quoted

@@ -1593,7 +1592,7 @@ static long check_and_migrate_cma_pages(struct mm_struct *mm,
                            }

                            if (!isolate_lru_page(head)) {
-                                     list_add_tail(&head->lru, &cma_page_list);
+                                     list_add_tail(&head->lru, &movable_page_list);
                                    mod_node_page_state(page_pgdat(head),
                                                        NR_ISOLATED_ANON +
                                                        page_is_file_lru(head),

@@ -1605,7 +1604,7 @@ static long check_and_migrate_cma_pages(struct mm_struct *mm,
            i += step;
    }

-     if (!list_empty(&cma_page_list)) {
+     if (!list_empty(&movable_page_list)) {

You didn't answer my earlier question, is it OK that ZONE_MOVABLE
pages leak out here if ioslate_lru_page() fails but the
moval_page_list is empty?

I think the answer is no, right?

In my opinion it is OK. We are doing our best to not pin movable
pages, but if isolate_lru_page() fails because pages are currently
locked by someone else, we will end up long-term pinning them.
See comment in this patch:
+        * 1. Pinned pages: (long-term) pinning of movable pages is avoided
+        *    when pages are pinned and faulted, but it is still possible that
+        *    address space already has pages in ZONE_MOVABLE at the time when
+        *    pages are pinned (i.e. user has touches that memory before
+        *    pinning). In such case we try to migrate them to a different zone,
+        *    but if migration fails the pages can still end-up pinned in
+        *    ZONE_MOVABLE. In such case, memory offlining might retry a long
+        *    time and will only succeed once user application unpins pages.

It is not "retry a long time" it is "might never complete" because
userspace will hold the DMA pin indefinitely.

Confused what the point of all this is then ??

I thought to goal here is to make memory unplug reliable, if you leave
a hole like this then any hostile userspace can block it forever.

You are right, I used a wording from the previous comment, and it
should be made clear that pin may be forever. Without these patches it
is guaranteed that hot-remove will fail if there are pinned pages as
ZONE_MOVABLE is actually the first to be searched. Now, it will fail
only due to exceptions listed in ZONE_MOVABLE comment:

1. pin + migration/isolation failure

Not sure what that really means. We have short-term pinnings (although we might have a better term for „pinning“ here) for example, when a process dies (IIRC). There is a period where pages cannot get migrated and offlining code has to retry (which might take a while). This still applies after your change - are you referring to that?

quoted

2. memblock allocation due to limited amount of space for kernelcore
3. memory holes
4. hwpoison
5. Unmovable PG_offline pages (? need to study why this is a scenario).

Virtio-mem is the primary user in this context.

quoted

Do you think we should unconditionally unpin pages, and return error
when isolation/migration fails?

I‘m not sure what you mean here. Who’s supposed to unpin which pages?

Hi David,

When check_and_migrate_movable_pages() is called, the pages are
already pinned. If some of those pages are in movable zone, and we
fail to migrate or isolate them what should we do: proceed, and keep
it as exception of when movable zone can actually have pinned pages or
unpin all pages in the array, and return an error, or unpin only pages
in movable zone, and return an error?

I guess revert what we did (unpin) and return an error. The interesting question is what can make migration/isolation fail

a) out of memory: smells like a zone setup issue. Failures are acceptable I guess.

b) short term pinnings: process dying - not relevant I guess. Other cases? (Fork?)

c) ?

Once we clarified that, we actually know how likely it will be to return an error (and making vfio pinnings fail etc).

Pasha