[Devel] Re: [PATCH] checkpoint/restart of robust futex lists

[Oren Laadan]

Serge E. Hallyn wrote:
> Quoting Matt Helsley (matthltc at us.ibm.com):
>> diff --git a/checkpoint/process.c b/checkpoint/process.c
>> index b604a85..084a2e4 100644
>> --- a/checkpoint/process.c
>> +++ b/checkpoint/process.c
>> @@ -42,6 +42,17 @@ static int checkpoint_task_struct(struct ckpt_ctx *ctx, struct task_struct *t)
>>
>>  	h->task_comm_len = TASK_COMM_LEN;
>>
>> +#ifdef CONFIG_FUTEX
>> +	/* These are __user pointers and can be saved without the objhash. */
>> +	h->robust_futex_list = t->robust_list;
>> +	h->robust_futex_head_len = sizeof(t->robust_list);
>> +#ifdef CONFIG_COMPAT
>> +	h->compat_robust_futex_list = t->compat_robust_list;
>> +	h->compat_robust_futex_head_len = sizeof(t->compat_robust_list);
>> +#endif
>> +	/* FIXME save pi futex state?? */
>> +#endif
>> +
> 
> So, I'm torn on this, but this does look like a prime example of code
> which is destined to go stale and out of sync with the main futex code.
> 
> On the other hand, if we define futex_checkpoint() and futex_restart(),
> and do that for every little thing we c/r, that could get out of hand...
> 
> But I think it's a risk worth taking.  What do you think?

Which risk is the one worth taking ?  -- I assume splitting the c/r
code to smaller pieces near/at related subsystems source code. The
question is: where do you draw the line ?

(This is also related to Andrew Morgan's concern about capabilities).

I think that for a "self-contained" object (e.g. capabilities),
it makes much sense to separate it.

I'm not so sure about passing struct ckpt_task_struct_hdr around
to small functions to fill it... can become very scattered. And
a dedicated "futex" object doesn't make sense either.

If anything, perhaps use a 'struct ckpt_task_futex' which will
be embedded in ckpt_task_struct_hdr:

struct ckpt_task_struct_hdr {
	struct ckpt_hdr h;
	...
	struct ckpt_task_futex futex;
	...
};

There are more examples, for instance:
* nsproxy:  in checkpoint/process.c or in kernel/nxproxy.c ?
* uts_ns:  in checkpoint/process.c or in kernel/utsname.c ?

And also:
* files/fd:  in checkpoint/files.c or in fs/checkpoint.c ?
* memory:  in checkpoint/memory.c or in mm/checkpoint.c ?

Originally it seemed natural to keep everything under checkpoint/
subdir. The cost is (a) risks getting out of sync, and (b) need
to export functions that could remain static/private otherwise.

Comments ?

Oren.

_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers