Serge E. Hallyn wrote:
> Quoting Oren Laadan (orenl@xxxxxxxxxxx):
>> Instead of playing tricks with xchg() of task->checkpoint_ctx, use the
>> task_{lock,unlock} to protect changes to that pointer. This simplifies
>> the logic since we no longer need to check for races (and old_ctx).
>>
>> The remaining changes include cleanup, and unification of common code
>> to handle errors during restart, and some debug statements.
>>
>> Signed-off-by: Oren Laadan <orenl@xxxxxxxxxxxxxxx>
>> ---
>> checkpoint/restart.c | 170 ++++++++++++++++++++++----------------------
>> checkpoint/sys.c | 6 +-
>> include/linux/checkpoint.h | 2 +-
>> kernel/fork.c | 6 +-
>> 4 files changed, 92 insertions(+), 92 deletions(-)
>>
>> diff --git a/checkpoint/restart.c b/checkpoint/restart.c
>> index 543b380..5d936cf 100644
>> --- a/checkpoint/restart.c
>> +++ b/checkpoint/restart.c
>> @@ -529,17 +529,54 @@ static inline int is_task_active(struct ckpt_ctx *ctx, pid_t pid)
>>
>> static inline void _restore_notify_error(struct ckpt_ctx *ctx, int errno)
>> {
>> - ckpt_set_ctx_error(ctx, errno);
>> - complete(&ctx->complete);
>> + /* first to fail: notify everyone (racy but harmless) */
>> + if (!ckpt_test_ctx_error(ctx)) {
>> + ckpt_debug("setting restart error %d\n", errno); \
>> + ckpt_set_ctx_error(ctx, errno);
>> + complete(&ctx->complete);
>> + wake_up_all(&ctx->waitq);
>> + wake_up_all(&ctx->ghostq);
>> + }
>> }
>>
>> /* Need to call ckpt_debug such that it will get the correct source location */
>> #define restore_notify_error(ctx, errno) \
>> do { \
>> - ckpt_debug("ctx root pid %d err %d", ctx->root_pid, errno); \
>> + ckpt_debug("restart error %d, root pid %d\n", errno, ctx->root_pid); \
>> _restore_notify_error(ctx, errno); \
>> } while(0)
>>
>
> Maybe make a note that these can't be called under
> write_lock_irq(&tasklist_lock)? Also, update the comment above
> task_lock() to add checkpoint_ctx to the list of things protected
> by it.
Right.
>
>> +static inline struct ckpt_ctx *get_task_ctx(struct task_struct *task)
>> +{
>> + struct ckpt_ctx *ctx;
>> +
>> + task_lock(task);
>> + ctx = ckpt_ctx_get(task->checkpoint_ctx);
>> + task_unlock(task);
>> + return ctx;
>> +}
>> +
>> +/* returns 1 on success, 0 otherwise */
>
> This works, but it's more confusing than it needs to be. I think using
> two helpers, 'set_task_ctx(tsk, ctx)' and 'clear_task-ctx(tsk)', where
> set_task_ctx always bails if task->checkpoint_ctx is set, would be much
> easier to read.
>
Sure.
> But
>
> Acked-by: Serge Hallyn <serue@xxxxxxxxxx>
Thanks,
Oren.
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
