lkml.org 
[lkml]   [2013]   [Jun]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] wait: fix false timeouts when using wait_event_timeout()
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@kernel.org>

Thanks! I'll send this patch soon.

Oleg.



\
 
 \ /
  Last update: 2013-06-06 21:41    [W:0.067 / U:0.968 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site