On Thu, 2008-12-18 at 06:10 -0500, Oren Laadan wrote:

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 ?

Mike, although we're doing writes of the checkpoint file here, the *mm*
access is read-only.  We only need really mmap_sem for write if we're
creating new VMAs, which we only do on restore.  Was there an action
taken on the mm that would require a write that we missed?

Oren, I never considered the locking overhead, either.  The fact that
the processes are frozen is very, very important here.  The code is fine
as it stands because this *is* a very simple way to do it.  But, this
probably deserves a comment. 

-- Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html