Re: [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer

[Amit Shah <amit.shah@redhat.com>]

On (Wed) 26 Sep 2012 [09:48:12], Sjur BRENDELAND wrote:
> > > 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.

Not necessary; see my comments in another mail.

> > > -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. 

Not really -- one thing I've picked up from Rusty is to use kmallocs
and explicitly initialise values.  Then, missing such initialisations
(like in this case) will cause tools like valgrind to show the error
pretty quickly.

> > >  	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().

But we're dropping the memset/kzalloc anyway.

		Amit