Re: [PATCH] wait: fix false timeouts when usingwait_event_timeout()

[Oleg Nesterov]

Hi Tejun,

On 06/05, Tejun Heo wrote:
>
> Heh, yeah, this looks good to me and a lot better than trying to do
> the same thing over and over again and ending up with subtle
> differences.

Yes, this is the goal. Of course we could fix wait_event_timout() and
wait_event_interruptible_timeout() without unification, but this would
add more copy-and-paste.

> > Hmm. I compiled the kernel with the patch below,
> >
> > 		$ size vmlinux
> > 		   text	   data	    bss	    dec	    hex	filename
> > 	-	4978601	2935080	10104832	18018513	112f0d1	vmlinux
> > 	+	4977769	2930984	10104832	18013585	112dd91	vmlinux
>
> Nice.  Provided you went over assembly outputs of at least some
> combinations,

I did, and that is why I am surprized by the numbers above.

Yes, gcc optimizes out the unwanted checks but the generated code
differs, of course. And sometimes the difference looks "random" to me.

Simplest example:

	extern wait_queue_head_t WQ;
	extern int COND;

	void we(void)
	{
		wait_event(WQ, COND);
	}

before:
	we:
		pushq	%rbp	#
		movq	%rsp, %rbp	#,
		pushq	%rbx	#
		subq	$56, %rsp	#,
		call	mcount
		movl	COND(%rip), %edx	# COND,
		testl	%edx, %edx	#
		jne	.L5	#,
		leaq	-64(%rbp), %rbx	#, tmp66
		movq	$0, -64(%rbp)	#, __wait
		movq	$autoremove_wake_function, -48(%rbp)	#, __wait.func
	#APP
	# 14 "/home/tmp/K/misc/arch/x86/include/asm/current.h" 1
		movq %gs:current_task,%rax	#, pfo_ret__
	# 0 "" 2
	#NO_APP
		movq	%rax, -56(%rbp)	# pfo_ret__, __wait.private
		leaq	24(%rbx), %rax	#, tmp61
		movq	%rax, -40(%rbp)	# tmp61, __wait.task_list.next
		movq	%rax, -32(%rbp)	# tmp61, __wait.task_list.prev
		.p2align 4,,10
		.p2align 3
	.L4:
		movl	$2, %edx	#,
		movq	%rbx, %rsi	# tmp66,
		movq	$WQ, %rdi	#,
		call	prepare_to_wait	#
		movl	COND(%rip), %eax	# COND,
		testl	%eax, %eax	#
		jne	.L3	#,
		call	schedule	#
		jmp	.L4	#
		.p2align 4,,10
		.p2align 3
	.L3:
		movq	%rbx, %rsi	# tmp66,
		movq	$WQ, %rdi	#,
		call	finish_wait	#
	.L5:
		addq	$56, %rsp	#,
		popq	%rbx	#
		leave
		ret

after:
	we:
		pushq	%rbp	#
		movq	%rsp, %rbp	#,
		pushq	%rbx	#
		subq	$56, %rsp	#,
		call	mcount
		movl	COND(%rip), %edx	# COND,
		testl	%edx, %edx	#
		jne	.L5	#,
		leaq	-64(%rbp), %rbx	#, tmp66
		movq	$0, -64(%rbp)	#, __wait
		movq	$autoremove_wake_function, -48(%rbp)	#, __wait.func
	#APP
	# 14 "/home/tmp/K/misc/arch/x86/include/asm/current.h" 1
		movq %gs:current_task,%rax	#, pfo_ret__
	# 0 "" 2
	#NO_APP
		movq	%rax, -56(%rbp)	# pfo_ret__, __wait.private
		leaq	24(%rbx), %rax	#, tmp61
		movq	%rax, -40(%rbp)	# tmp61, __wait.task_list.next
		movq	%rax, -32(%rbp)	# tmp61, __wait.task_list.prev
		.p2align 4,,10
		.p2align 3
	.L4:
		movl	$2, %edx	#,
		movq	%rbx, %rsi	# tmp66,
		movq	$WQ, %rdi	#,
		call	prepare_to_wait	#
		movl	COND(%rip), %eax	# COND,
		testl	%eax, %eax	#
		je	.L3	#,
		movq	%rbx, %rsi	# tmp66,
		movq	$WQ, %rdi	#,
		call	finish_wait	#
		jmp	.L5	#
		.p2align 4,,10
		.p2align 3
	.L3:
		call	schedule	#
		.p2align 4,,8
		.p2align 3
		jmp	.L4	#
		.p2align 4,,10
		.p2align 3
	.L5:
		addq	$56, %rsp	#,
		popq	%rbx	#
		leave
		.p2align 4,,4
		.p2align 3
		ret

As you can see, with this patch gcc generates a bit more code. But
only because reorderes finish_wait() and inserts the additional nops,
otherwise code the same. I think this difference is not "reliable"
and can be ignored.

But, the code like "return wait_even_timeout(true, non_const_timeout)"
really generates more code and this is correct: the patch tries to fix
the bug (at least I think this is a bug) and the additional code ensures
that __ret != 0.

>  Reviewed-by: Tejun Heo <tj@xxxxxxxxxx>

Thanks! I'll send this patch soon.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/