[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-dev] Re: [PATCH v22 2/3] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ
On 01/24/2018 12:29 PM, Michael S. Tsirkin wrote:
On Mon, Jan 22, 2018 at 07:25:45PM +0800, Wei Wang wrote:On 01/19/2018 08:39 PM, Michael S. Tsirkin wrote:On Fri, Jan 19, 2018 at 11:44:21AM +0800, Wei Wang wrote:On 01/18/2018 12:44 AM, Michael S. Tsirkin wrote:On Wed, Jan 17, 2018 at 01:10:11PM +0800, Wei Wang wrote:+ vb->start_cmd_id = cmd_id; + queue_work(vb->balloon_wq, &vb->report_free_page_work);It seems that if a command was already queued (with a different id), this will result in new command id being sent to host twice, which will likely confuse the host.I think that case won't happen, because - the host sends a cmd id to the guest via the config, while the guest acks back the received cmd id via the virtqueue; - the guest ack back a cmd id only when a new cmd id is received from the host, that is the above check: if (cmd_id != vb->start_cmd_id) { --> the driver only queues the reporting work only when a new cmd id is received /* * Host requests to start the reporting by sending a * new cmd id. */ WRITE_ONCE(vb->report_free_page, true); vb->start_cmd_id = cmd_id; queue_work(vb->balloon_wq, &vb->report_free_page_work); } So the same cmd id wouldn't queue the reporting work twice.Like this: vb->start_cmd_id = cmd_id; queue_work(vb->balloon_wq, &vb->report_free_page_work); command id changes vb->start_cmd_id = cmd_id; work executes queue_work(vb->balloon_wq, &vb->report_free_page_work); work executes againIf we think about the whole working flow, I think this case couldn't happen: 1) device send cmd_id=1 to driver; 2) driver receives cmd_id=1 in the config and acks cmd_id=1 to the device via the vq; 3) device revives cmd_id=1; 4) device wants to stop the reporting by sending cmd_id=STOP; 5) driver receives cmd_id=STOP from the config, and acks cmd_id=STOP to the device via the vq; 6) device sends cmd_id=2 to driver; ... cmd_id=2 won't come after cmd_id=1, there will be a STOP cmd in between them (STOP won't queue the work). How about defining the correct device behavior in the spec: The device Should NOT send a second cmd id to the driver until a STOP cmd ack for the previous cmd id has been received from the guest. Best, WeiI think we should just fix races in the driver rather than introduce random restrictions in the device. If device wants to start a new sequence, it should be able to do just that without a complicated back and forth with several roundtrips through the driver.
OK, I've fixed it in the new version, v24. Please have a check there. Thanks.
(Other changes based on the comments on v23 have also been included) Best, Wei
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]