Re: [PATCH] hw/char/virtio-serial-bus: fix: Unpop throttled VirtQueueElement to queue before discard vq data

[AIERPATIJIANG1 [艾尔帕提江·阿布都赛买提]]

在 2021/8/2 下午11:01，“Laurent Vivier”<lvivier@redhat.com> 写入:

    On 30/07/2021 03:58, AIERPATIJIANG1 [艾尔帕提江·阿布都赛买提] wrote:
    >> Ports enter a "throttled" state when writing to the chardev would block.
    >> The current output VirtQueueElement is kept around until the chardev
    >> becomes writable again.
    >> 
    >> Because closing the virtio serial device does not reset the queue, we 
cannot
    >> directly discard this element,  otherwise the control variables of the 
front
    >> and back ends of the queue are inconsistent such as used_index. We 
should unpop the
    >> VirtQueueElement to queue, let discard_vq_data process it.
    >> 
    >> The test environment:
    >> kernel: linux-5.12
    >> Qemu command:
    >> Qemu-system-x86 -machine pc,accel=kvm \
    >>     -cpu host,host-phys-bits \
    >>     -smp 4 \
    >>     -m 4G \
    >>     -kernel ./kernel \
    >>     -display none \
    >>     -nodefaults \
    >>     -serial mon:stdio \
    >>     -append "panic=1 no_timer_check noreplace-smp rootflags=data=ordered 
rootfstype=ext4 console=ttyS0 reboot=k root=/dev/vda1 rw" \
    >>     -drive id=os,file=./disk,if=none \
    >>     -device virtio-blk-pci,drive=os \
    >>     -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x4 \
    >>     -chardev 
socket,id=charchannel0,path=/tmp/char-dev-test,server,nowait \
    >>   -device 
virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=org.qemu.guest_agent.0
    >> 
    >> full up virtio queue after VM started:
    >> Cat /large-file > /dev/vport1p1
    >> 
    >> Host side:
    >> Open and close character device sockets repeatedly
    >> 
    >> After awhile we can’t write any request to /dev/vport1p1 at VM side, VM 
kernel soft lockup at drivers/char/virtio_console.c: __send_to_port
    >> 
    >> 
    >> Signed-off-by: Arafatms <aierpatijiang1@kingsoft.com>
    >> 
    >> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
    >> index dd6bc27b3b..36236defdf 100644
    >> --- a/hw/char/virtio-serial-bus.c
    >> +++ b/hw/char/virtio-serial-bus.c
    >> @@ -150,8 +150,12 @@ static void discard_vq_data(VirtQueue *vq, 
VirtIODevice *vdev)
    >> 
    >> static void discard_throttle_data(VirtIOSerialPort *port)
    >> {
    >> +    if (!virtio_queue_ready(port->ovq)) {
    >> +        return;
    >> +    }

    >Why do we need to check virtio_queue_ready() now?

        I think we should check virtio_queue_ready before every operation of 
virt_queues.

    >> +
    >>      if (port->elem) {
    >> -        virtqueue_detach_element(port->ovq, port->elem, 0);
    >> +        virtqueue_unpop(port->ovq, port->elem, 0);
    >>          g_free(port->elem);
    >>          port->elem = NULL;
    >>      }
    >> 

    > It seems the problem you report can only happen when the port is closed 
from the host side
    > (because from the guest side I guess the cleanup is done by the guest 
driver).

        Yes, this problem can only happen when the port is closed from the host 
side. 

    > Stefan, you have introduced the function discard_throttle_data() in:

    >  d4c19cdeeb2f ("virtio-serial: add missing virtio_detach_element() call")

    > do you remember why you prefered to use virtqueue_detach_element() rather 
than
    > virtqueue_unpop() (or virtqueue_discard() at this time, see 27e57efe32f5 
("virtio: rename
    > virtqueue_discard to virtqueue_unpop"))

    > Thanks,
    > Laurent

        I observed all the situations where discard_throttle_data is called, 
The result only points to one situation, that is, the virtio device is
        damaged(driver will reset all queues). About virtio-serial device, 
close devise from host side doesn't mean the driver reset all queues,
        so we can't just detach the throttled elem.

        Thanks,
        Arafatms