On (Tue) Oct 19 2010 [08:57:43], Hans de Goede wrote: > Hi, > > Ok replying to my own reply, because I misread the code. > > On 10/19/2010 08:55 AM, Hans de Goede wrote: > >Hi, > > > >On 10/19/2010 07:45 AM, Amit Shah wrote: > >>If the host is slow in reading data or doesn't read data at all, > >>blocking write calls not only blocked the program that called write() > >>but the entire guest itself. > >> > >>To overcome this, let's not block till the host signals it has given > >>back the virtio ring element we passed it. Instead, send the buffer to > >>the host and return to userspace. This operation then becomes similar > >>to how non-blocking writes work, so let's use the existing code for this > >>path as well. > >> > >>This code change also ensures blocking write calls do get blocked if > >>there's not enough room in the virtio ring as well as they don't return > >>-EAGAIN to userspace. > >> > >>Signed-off-by: Amit Shah<amit.shah@xxxxxxxxxx> > >>CC: stable@xxxxxxxxxx > >>--- > >>drivers/char/virtio_console.c | 17 ++++++++++++++--- > >>1 files changed, 14 insertions(+), 3 deletions(-) > >> > >>diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c > >>index c810481..0f69c5e 100644 > >>--- a/drivers/char/virtio_console.c > >>+++ b/drivers/char/virtio_console.c > >>@@ -459,9 +459,12 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count, > >> > >>/* > >>* Wait till the host acknowledges it pushed out the data we > >>- * sent. This is done for ports in blocking mode or for data > >>- * from the hvc_console; the tty operations are performed with > >>- * spinlocks held so we can't sleep here. > >>+ * sent. This is done for data from the hvc_console; the tty > >>+ * operations are performed with spinlocks held so we can't > >>+ * sleep here. An alternative would be to copy the data to a > >>+ * buffer and relax the spinning requirement. The downside is > >>+ * we need to kmalloc a GFP_ATOMIC buffer each time the > >>+ * console driver writes something out. > >>*/ > >>while (!virtqueue_get_buf(out_vq,&len)) > >>cpu_relax(); > >>@@ -626,6 +629,14 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf, > >>goto free_buf; > >>} > >> > >>+ /* > >>+ * We now ask send_buf() to not spin for generic ports -- we > >>+ * can re-use the same code path that non-blocking file > >>+ * descriptors take for blocking file descriptors since the > >>+ * wait is already done and we're certain the write will go > >>+ * through to the host. > >>+ */ > >>+ nonblock = true; > >>ret = send_buf(port, buf, count, nonblock); > >> > >>if (nonblock&& ret> 0) > > > >1) Hmm, this changes the code to kfree the buffer, but only if the send_buf > >succeeded (which it always should given we did a will_block check first). > > > >I cannot help but notice that the data was not freed on a blocking fd > >before this patch, but is freed now. And I see nothing in send_buf to make > >it take ownership of the buffer / free it in the blocking case, and not take > >ownership in the blocking case. > > This part still stands. > > > More over if anything I would expect send_buf > >to take ownership in the non blocking case (as the data is not directly > >consumed there), and not take owner ship in the blocking case, but the check > >is the reverse. Also why is the buffer not freed if the write failed, that > >makes no sense. > > > > This part is wrong the: > > if (nonblock && ret> 0) > > Check make it jump (goto) over the free, so it does make sense, but is coded > rather convolutedly. This means that: a) we're not going to wait for the host to tell us it used the buffer, so keep the buffer around. b) we actually managed to send the buffer to the host. I don't see why that's convoluted :-) Amit _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/virtualization
