Re: [RFC v11][PATCH 04/13] x86 support for checkpoint/restart

Mike Waychison wrote:

Oren Laadan wrote:

quoted

Add logic to save and restore architecture specific state, including
thread-specific state, CPU registers and FPU state.

In addition, architecture capabilities are saved in an architecure
specific extension of the header (cr_hdr_head_arch); Currently this
includes only FPU capabilities.

Currently only x86-32 is supported. Compiling on x86-64 will trigger
an explicit error.

[...]

quoted

+
+    hh->gdt_entry_tls_entries = GDT_ENTRY_TLS_ENTRIES;
+    hh->sizeof_tls_array = sizeof(thread->tls_array);
+    hh->ntls = ntls;
+
+    ret = cr_write_obj(ctx, &h, hh);
+    cr_hbuf_put(ctx, sizeof(*hh));
+    if (ret < 0)
+        return ret;
+
+    cr_debug("ntls %d\n", ntls);
+    if (ntls == 0)
+        return 0;
+
+    /* for simplicity dump the entire array, cherry-pick upon restart */
+    ret = cr_kwrite(ctx, thread->tls_array, sizeof(thread->tls_array));

Again, the the TLS descriptors in the GDT should be called out an not
tied to the in-kernel representation.

True. I'll add a 'FIXME' comment.

However, I'm yet to see a case where this breaks among x86_32, and I'm no
expert in that area to tell whether it could. (Moving from x86_32 to x86_64
is another story, and will require some compatibility layer anyway).

quoted

+
+    /* IGNORE RESTART BLOCKS FOR NOW ... */

[...]

quoted

+static int cr_write_cpu_fpu(struct cr_ctx *ctx, struct task_struct *t)
+{
+    void *xstate_buf = cr_hbuf_get(ctx, xstate_size);
+
+    /* i387 + MMU + SSE logic */
+    preempt_disable();    /* needed it (t == current) */
+
+    /*
+     * normally, no need to unlazy_fpu(), since TS_USEDFPU flag
+     * have been cleared when task was context-switched out...
+     * except if we are in process context, in which case we do
+     */
+    if (t == current && (task_thread_info(t)->status & TS_USEDFPU))
+        unlazy_fpu(current);
+
+    memcpy(xstate_buf, t->thread.xstate, xstate_size);

This is probably better off being very deliberate about what registers
we are dumping from a traceability and compatibility point of view?

Same here.

quoted

+    preempt_enable();    /* needed it (t == current) */
+
+    return cr_kwrite(ctx, xstate_buf, xstate_size);

Missed cr_huf_put()

Ooops ... will fix.

quoted

+}
+
+#endif    /* CONFIG_X86_64 */

[...]

quoted

+        /*
+         * restore TLS by hand: why convert to struct user_desc if
+         * sys_set_thread_entry() will convert it back ?
+         */
+
+        size = sizeof(*desc) * GDT_ENTRY_TLS_ENTRIES;
+        desc = kmalloc(size, GFP_KERNEL);
+        if (!desc)

cr_hbuf_put() here.

Will fix.

quoted

+            return -ENOMEM;
+
+        ret = cr_kread(ctx, desc, size);
+        if (ret >= 0) {

if (ret == 0)

Right.

quoted

+            /*
+             * FIX: add sanity checks (eg. that values makes
+             * sense, that we don't overwrite old values, etc
+             */
+            cpu = get_cpu();
+            memcpy(thread->tls_array, desc, size);
+            load_TLS(thread, cpu);
+            put_cpu();
+        }
+        kfree(desc);
+    }
+
+    ret = 0;
+ out:
+    cr_hbuf_put(ctx, sizeof(*hh));
+    return ret;
+}

[...]

Thanks for the review !

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>