Messages in this thread | | | From | Sjur BRENDELAND <> | Date | Wed, 26 Sep 2012 09:48:12 +0200 | Subject | RE: [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer |
| |
> > This merge reduces code size by unifying the approach for > > sending scatter-lists and regular buffers. Any type of > > write operation (splice, write, put_chars) will now allocate > > a port_buffer and send_buf() and free_buf() can always be used. > > Thanks! > This looks much nicer and simpler. I just have some comments below.
OK, good to hear that you agree to this kind of change. I'll do a respin of this patch fixing the issues you have pointed out.
> > static void free_buf(struct port_buffer *buf) > > { > > + int i; > > + > > kfree(buf->buf); > > this should be done only when !buf->sgpages, or (see below)
Agree, I'll put this statement in an else branch to the if-statement below.
> > > + > > + if (buf->sgpages) > > + for (i = 0; i < buf->sgpages; i++) { > > + struct page *page = sg_page(&buf->sg[i]); > > + if (!page) > > + break; > > + put_page(page); > > + } > > + > > kfree(buf); > > } > > > > -static struct port_buffer *alloc_buf(size_t buf_size) > > +static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size, > > + int nrbufs) > > { > > struct port_buffer *buf; > > + size_t alloc_size; > > > > - buf = kmalloc(sizeof(*buf), GFP_KERNEL); > > + /* Allocate buffer and the scatter list */ > > + alloc_size = sizeof(*buf) + sizeof(struct scatterlist) * nrbufs; > > This allocates one redundant sg entry when nrbuf > 0, > but I think it is OK. (just a comment)
I did this on purpose for the sake of simplicity, but I can change this to something like: alloc_size = sizeof(*buf) + sizeof(buf->sg) * max(nrbufs - 1, 1);
> > + buf = kmalloc(alloc_size, GFP_ATOMIC); > > This should be kzalloc(), or buf->buf and others are not initialized, > which will cause unexpected kfree bug at kfree(buf->buf) in free_buf.
Agree, kzalloc() is better in this case.
> > if (!buf) > > goto fail; > > - buf->buf = kzalloc(buf_size, GFP_KERNEL); > > + > > + buf->sgpages = nrbufs; > > + if (nrbufs > 0) > > + return buf; > > + > > + buf->buf = kmalloc(buf_size, GFP_ATOMIC); > > You can also use kzalloc here as previous code does. > But if the reason why using kzalloc comes from the security, > I think kmalloc is enough here, since the host can access > all the guest pages anyway.
With this new patch alloc_buf() is used both for both RX and TX. The out_vq did previously use malloc(). But I have preserved the legacy behavior for the in_vq by calling memset() in function fill_queue().
Thanks, Sjur
| |