Message ID | 1348042111-16143-1-git-send-email-mjt@msgid.tls.msk.ru |
---|---|
State | New |
Headers | show |
Ping? /mjt On 19.09.2012 12:08, Michael Tokarev wrote: > This reverts commit 67c5322d7000fd105a926eec44bc1765b7d70bdd: > > I'm not sure if the retry logic has ever worked when not using FIFO mode. I > found this while writing a test case although code inspection confirms it is > definitely broken. > > The TSR retry logic will never actually happen because it is guarded by an > 'if (s->tsr_rety > 0)' but this is the only place that can ever make the > variable greater than zero. That effectively makes the retry logic an 'if (0) > > I believe this is a typo and the intention was >= 0. Once this is fixed thoug > I see double transmits with my test case. This is because in the non FIFO > case, serial_xmit may get invoked while LSR.THRE is still high because the > character was processed but the retransmit timer was still active. > > We can handle this by simply checking for LSR.THRE and returning early. It's > possible that the FIFO paths also need some attention. > > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > > Even if the previous logic was never worked, new logic breaks stuff - > namely, > > qemu -enable-kvm -nographic -kernel /boot/vmlinuz-$(uname -r) -append console=ttyS0 -serial pty > > the above command will cause the virtual machine to stuck at startup > using 100% CPU till one connects to the pty and sends any char to it. > > Note this is rather typical invocation for various headless virtual > machines by libvirt. > > So revert this change for now, till a better solution will be found. > > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> > --- > hw/serial.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/hw/serial.c b/hw/serial.c > index a421d1e..df54de2 100644 > --- a/hw/serial.c > +++ b/hw/serial.c > @@ -327,8 +327,6 @@ static void serial_xmit(void *opaque) > s->tsr = fifo_get(s,XMIT_FIFO); > if (!s->xmit_fifo.count) > s->lsr |= UART_LSR_THRE; > - } else if ((s->lsr & UART_LSR_THRE)) { > - return; > } else { > s->tsr = s->thr; > s->lsr |= UART_LSR_THRE; > @@ -340,7 +338,7 @@ static void serial_xmit(void *opaque) > /* in loopback mode, say that we just received a char */ > serial_receive1(s, &s->tsr, 1); > } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) { > - if ((s->tsr_retry >= 0) && (s->tsr_retry <= MAX_XMIT_RETRY)) { > + if ((s->tsr_retry > 0) && (s->tsr_retry <= MAX_XMIT_RETRY)) { > s->tsr_retry++; > qemu_mod_timer(s->transmit_timer, new_xmit_ts + s->char_transmit_time); > return;
Ping^2 ? /mjt 27.10.2012 12:31, Michael Tokarev wrote: > Ping? > > On 19.09.2012 12:08, Michael Tokarev wrote: >> This reverts commit 67c5322d7000fd105a926eec44bc1765b7d70bdd: >> >> I'm not sure if the retry logic has ever worked when not using FIFO mode. I >> found this while writing a test case although code inspection confirms it is >> definitely broken. >> >> The TSR retry logic will never actually happen because it is guarded by an >> 'if (s->tsr_rety > 0)' but this is the only place that can ever make the >> variable greater than zero. That effectively makes the retry logic an 'if (0) >> >> I believe this is a typo and the intention was >= 0. Once this is fixed thoug >> I see double transmits with my test case. This is because in the non FIFO >> case, serial_xmit may get invoked while LSR.THRE is still high because the >> character was processed but the retransmit timer was still active. >> >> We can handle this by simply checking for LSR.THRE and returning early. It's >> possible that the FIFO paths also need some attention. >> >> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> >> >> Even if the previous logic was never worked, new logic breaks stuff - >> namely, >> >> qemu -enable-kvm -nographic -kernel /boot/vmlinuz-$(uname -r) -append console=ttyS0 -serial pty >> >> the above command will cause the virtual machine to stuck at startup >> using 100% CPU till one connects to the pty and sends any char to it. >> >> Note this is rather typical invocation for various headless virtual >> machines by libvirt. >> >> So revert this change for now, till a better solution will be found. >> >> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> >> --- >> hw/serial.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/hw/serial.c b/hw/serial.c >> index a421d1e..df54de2 100644 >> --- a/hw/serial.c >> +++ b/hw/serial.c >> @@ -327,8 +327,6 @@ static void serial_xmit(void *opaque) >> s->tsr = fifo_get(s,XMIT_FIFO); >> if (!s->xmit_fifo.count) >> s->lsr |= UART_LSR_THRE; >> - } else if ((s->lsr & UART_LSR_THRE)) { >> - return; >> } else { >> s->tsr = s->thr; >> s->lsr |= UART_LSR_THRE; >> @@ -340,7 +338,7 @@ static void serial_xmit(void *opaque) >> /* in loopback mode, say that we just received a char */ >> serial_receive1(s, &s->tsr, 1); >> } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) { >> - if ((s->tsr_retry >= 0) && (s->tsr_retry <= MAX_XMIT_RETRY)) { >> + if ((s->tsr_retry > 0) && (s->tsr_retry <= MAX_XMIT_RETRY)) { >> s->tsr_retry++; >> qemu_mod_timer(s->transmit_timer, new_xmit_ts + s->char_transmit_time); >> return; > >
Ping^3? This issue is still present in qemu 1.3 and current git (1.4-tobe) versions, and the said commit is still revertable, and reverting it still fixes the problem... I wonder why only debian users suffer from this problem ;) Thanks, /mjt 12.11.2012 19:13, Michael Tokarev wrote: > Ping^2 ? > > /mjt > > 27.10.2012 12:31, Michael Tokarev wrote: >> Ping? >> >> On 19.09.2012 12:08, Michael Tokarev wrote: >>> This reverts commit 67c5322d7000fd105a926eec44bc1765b7d70bdd: >>> >>> I'm not sure if the retry logic has ever worked when not using FIFO mode. I >>> found this while writing a test case although code inspection confirms it is >>> definitely broken. >>> >>> The TSR retry logic will never actually happen because it is guarded by an >>> 'if (s->tsr_rety > 0)' but this is the only place that can ever make the >>> variable greater than zero. That effectively makes the retry logic an 'if (0) >>> >>> I believe this is a typo and the intention was >= 0. Once this is fixed thoug >>> I see double transmits with my test case. This is because in the non FIFO >>> case, serial_xmit may get invoked while LSR.THRE is still high because the >>> character was processed but the retransmit timer was still active. >>> >>> We can handle this by simply checking for LSR.THRE and returning early. It's >>> possible that the FIFO paths also need some attention. >>> >>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> >>> >>> Even if the previous logic was never worked, new logic breaks stuff - >>> namely, >>> >>> qemu -enable-kvm -nographic -kernel /boot/vmlinuz-$(uname -r) -append console=ttyS0 -serial pty >>> >>> the above command will cause the virtual machine to stuck at startup >>> using 100% CPU till one connects to the pty and sends any char to it. >>> >>> Note this is rather typical invocation for various headless virtual >>> machines by libvirt. >>> >>> So revert this change for now, till a better solution will be found. >>> >>> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> >>> --- >>> hw/serial.c | 4 +--- >>> 1 file changed, 1 insertion(+), 3 deletions(-) >>> >>> diff --git a/hw/serial.c b/hw/serial.c >>> index a421d1e..df54de2 100644 >>> --- a/hw/serial.c >>> +++ b/hw/serial.c >>> @@ -327,8 +327,6 @@ static void serial_xmit(void *opaque) >>> s->tsr = fifo_get(s,XMIT_FIFO); >>> if (!s->xmit_fifo.count) >>> s->lsr |= UART_LSR_THRE; >>> - } else if ((s->lsr & UART_LSR_THRE)) { >>> - return; >>> } else { >>> s->tsr = s->thr; >>> s->lsr |= UART_LSR_THRE; >>> @@ -340,7 +338,7 @@ static void serial_xmit(void *opaque) >>> /* in loopback mode, say that we just received a char */ >>> serial_receive1(s, &s->tsr, 1); >>> } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) { >>> - if ((s->tsr_retry >= 0) && (s->tsr_retry <= MAX_XMIT_RETRY)) { >>> + if ((s->tsr_retry > 0) && (s->tsr_retry <= MAX_XMIT_RETRY)) { >>> s->tsr_retry++; >>> qemu_mod_timer(s->transmit_timer, new_xmit_ts + s->char_transmit_time); >>> return; >> >> > >
Am 22.01.2013 12:01, schrieb Michael Tokarev: > Ping^3? > > This issue is still present in qemu 1.3 and current git (1.4-tobe) > versions, > and the said commit is still revertable, and reverting it still fixes the > problem... > > I wonder why only debian users suffer from this problem ;) It was reported for openSUSE as well [1], but Anthony promised me it would get reverted for 1.3... I admit, I simply assumed it to be fixed with v1.3.0. Regards, Andreas [1] https://bugzilla.novell.com/show_bug.cgi?id=779727 > > Thanks, > > /mjt > > 12.11.2012 19:13, Michael Tokarev wrote: >> Ping^2 ? >> >> /mjt >> >> 27.10.2012 12:31, Michael Tokarev wrote: >>> Ping? >>> >>> On 19.09.2012 12:08, Michael Tokarev wrote: >>>> This reverts commit 67c5322d7000fd105a926eec44bc1765b7d70bdd: >>>> >>>> I'm not sure if the retry logic has ever worked when not using >>>> FIFO mode. I >>>> found this while writing a test case although code inspection >>>> confirms it is >>>> definitely broken. >>>> >>>> The TSR retry logic will never actually happen because it is >>>> guarded by an >>>> 'if (s->tsr_rety > 0)' but this is the only place that can ever >>>> make the >>>> variable greater than zero. That effectively makes the retry >>>> logic an 'if (0) >>>> >>>> I believe this is a typo and the intention was >= 0. Once this >>>> is fixed thoug >>>> I see double transmits with my test case. This is because in >>>> the non FIFO >>>> case, serial_xmit may get invoked while LSR.THRE is still high >>>> because the >>>> character was processed but the retransmit timer was still active. >>>> >>>> We can handle this by simply checking for LSR.THRE and >>>> returning early. It's >>>> possible that the FIFO paths also need some attention. >>>> >>>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >>>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> >>>> >>>> Even if the previous logic was never worked, new logic breaks stuff - >>>> namely, >>>> >>>> qemu -enable-kvm -nographic -kernel /boot/vmlinuz-$(uname -r) >>>> -append console=ttyS0 -serial pty >>>> >>>> the above command will cause the virtual machine to stuck at startup >>>> using 100% CPU till one connects to the pty and sends any char to it. >>>> >>>> Note this is rather typical invocation for various headless virtual >>>> machines by libvirt. >>>> >>>> So revert this change for now, till a better solution will be found. >>>> >>>> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> >>>> --- >>>> hw/serial.c | 4 +--- >>>> 1 file changed, 1 insertion(+), 3 deletions(-) >>>> >>>> diff --git a/hw/serial.c b/hw/serial.c >>>> index a421d1e..df54de2 100644 >>>> --- a/hw/serial.c >>>> +++ b/hw/serial.c >>>> @@ -327,8 +327,6 @@ static void serial_xmit(void *opaque) >>>> s->tsr = fifo_get(s,XMIT_FIFO); >>>> if (!s->xmit_fifo.count) >>>> s->lsr |= UART_LSR_THRE; >>>> - } else if ((s->lsr & UART_LSR_THRE)) { >>>> - return; >>>> } else { >>>> s->tsr = s->thr; >>>> s->lsr |= UART_LSR_THRE; >>>> @@ -340,7 +338,7 @@ static void serial_xmit(void *opaque) >>>> /* in loopback mode, say that we just received a char */ >>>> serial_receive1(s, &s->tsr, 1); >>>> } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) { >>>> - if ((s->tsr_retry >= 0) && (s->tsr_retry <= MAX_XMIT_RETRY)) { >>>> + if ((s->tsr_retry > 0) && (s->tsr_retry <= MAX_XMIT_RETRY)) { >>>> s->tsr_retry++; >>>> qemu_mod_timer(s->transmit_timer, new_xmit_ts + >>>> s->char_transmit_time); >>>> return; >>> >>> >> >> > >
diff --git a/hw/serial.c b/hw/serial.c index a421d1e..df54de2 100644 --- a/hw/serial.c +++ b/hw/serial.c @@ -327,8 +327,6 @@ static void serial_xmit(void *opaque) s->tsr = fifo_get(s,XMIT_FIFO); if (!s->xmit_fifo.count) s->lsr |= UART_LSR_THRE; - } else if ((s->lsr & UART_LSR_THRE)) { - return; } else { s->tsr = s->thr; s->lsr |= UART_LSR_THRE; @@ -340,7 +338,7 @@ static void serial_xmit(void *opaque) /* in loopback mode, say that we just received a char */ serial_receive1(s, &s->tsr, 1); } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) { - if ((s->tsr_retry >= 0) && (s->tsr_retry <= MAX_XMIT_RETRY)) { + if ((s->tsr_retry > 0) && (s->tsr_retry <= MAX_XMIT_RETRY)) { s->tsr_retry++; qemu_mod_timer(s->transmit_timer, new_xmit_ts + s->char_transmit_time); return;