Mike Waychison wrote:

Oren Laadan wrote:

quoted

Mike Waychison wrote:

quoted

Comments below.

Thanks for the detailed review.

quoted

Oren Laadan wrote:

quoted

For each VMA, there is a 'struct cr_vma'; if the VMA is file-mapped,
it will be followed by the file name. Then comes the actual contents,
in one or more chunk: each chunk begins with a header that specifies
how many pages it holds, then the virtual addresses of all the dumped
pages in that chunk, followed by the actual contents of all dumped
pages. A header with zero number of pages marks the end of the
contents.
Then comes the next VMA and so on.

[...]

quoted

quoted

+    mutex_lock(&mm->context.lock);
+
+    hh->ldt_entry_size = LDT_ENTRY_SIZE;
+    hh->nldt = mm->context.size;
+
+    cr_debug("nldt %d\n", hh->nldt);
+
+    ret = cr_write_obj(ctx, &h, hh);
+    cr_hbuf_put(ctx, sizeof(*hh));
+    if (ret < 0)
+        goto out;
+
+    ret = cr_kwrite(ctx, mm->context.ldt,
+            mm->context.size * LDT_ENTRY_SIZE);

Do we really want to emit anything under lock?  I realize that this
patch goes and does a ton of writes with mmap_sem held for read -- is
this ok?

Because all tasks in the container must be frozen during the checkpoint,
there is no performance penalty for keeping the locks. Although the
object
should not change in the interim anyways, the locks protects us from,
e.g.
the task unfreezing somehow, or being killed by the OOM killer, or any
other change incurred from the "outside world" (even future code).

Put in other words - in the long run it is safer to assume that the
underlying object may otherwise change.

(If we want to drop the lock here before cr_kwrite(), we need to copy the
data to a temporary buffer first. If we also want to drop mmap_sem(), we
need to be more careful with following the vma's.)

Do you see a reason to not keeping the locks ?

I just thought it was a bit ugly, but I can't think of a case
specifically where it's going to cause us harm.  If tasks are frozen,
are they still subject to the oom killer?   Even that should be
reasonably ok considering that the exit-path requires a
down_read(mmap_sem) (at least, it used to..  I haven't gone over that
path in a while..).

Excatly: this is safe because we keep the lock. It all boils down to
two points: holding the locks doesn't impair performance or functionality,
and it protects us against existing (if any) and future undesired
interactions with other code.

[...]

Oren.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>