From mboxrd@z Thu Jan 1 00:00:00 1970 From: Masami Hiramatsu Subject: Re: [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer Date: Wed, 26 Sep 2012 11:44:33 +0900 Message-ID: <50626C11.9040708@hitachi.com> References: <1348580837-10919-1-git-send-email-sjur.brandeland@stericsson.com> <1348580837-10919-2-git-send-email-sjur.brandeland@stericsson.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <1348580837-10919-2-git-send-email-sjur.brandeland@stericsson.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org To: sjur.brandeland@stericsson.com Cc: "Michael S. Tsirkin" , sjurbren@stericsson.com, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, yrl.pp-manager.tt@hitachi.com, Amit Shah , Linus Walleij List-Id: virtualization@lists.linuxfoundation.org KDIwMTIvMDkvMjUgMjI6NDcpLCBzanVyLmJyYW5kZWxhbmRAc3Rlcmljc3Nvbi5jb20gd3JvdGU6 Cj4gRnJvbTogU2p1ciBCcsOmbmRlbGFuZCA8c2p1ci5icmFuZGVsYW5kQHN0ZXJpY3Nzb24uY29t Pgo+IAo+IFRoaXMgbWVyZ2UgcmVkdWNlcyBjb2RlIHNpemUgYnkgdW5pZnlpbmcgdGhlIGFwcHJv YWNoIGZvcgo+IHNlbmRpbmcgc2NhdHRlci1saXN0cyBhbmQgcmVndWxhciBidWZmZXJzLiBBbnkg dHlwZSBvZgo+IHdyaXRlIG9wZXJhdGlvbiAoc3BsaWNlLCB3cml0ZSwgcHV0X2NoYXJzKSB3aWxs IG5vdyBhbGxvY2F0ZQo+IGEgcG9ydF9idWZmZXIgYW5kIHNlbmRfYnVmKCkgYW5kIGZyZWVfYnVm KCkgY2FuIGFsd2F5cyBiZSB1c2VkLgoKVGhhbmtzIQpUaGlzIGxvb2tzIG11Y2ggbmljZXIgYW5k IHNpbXBsZXIuIEkganVzdCBoYXZlIHNvbWUgY29tbWVudHMgYmVsb3cuCgo+IFNpZ25lZC1vZmYt Ynk6IFNqdXIgQnLDpm5kZWxhbmQgPHNqdXIuYnJhbmRlbGFuZEBzdGVyaWNzc29uLmNvbT4KPiBj YzogUnVzdHkgUnVzc2VsbCA8cnVzdHlAcnVzdGNvcnAuY29tLmF1Pgo+IGNjOiBNaWNoYWVsIFMu IFRzaXJraW4gPG1zdEByZWRoYXQuY29tPgo+IGNjOiBBbWl0IFNoYWggPGFtaXQuc2hhaEByZWRo YXQuY29tPgo+IGNjOiBMaW51cyBXYWxsZWlqIDxsaW51cy53YWxsZWlqQGxpbmFyby5vcmc+Cj4g Y2M6IE1hc2FtaSBIaXJhbWF0c3UgPG1hc2FtaS5oaXJhbWF0c3UucHRAaGl0YWNoaS5jb20+Cj4g LS0tCj4gIGRyaXZlcnMvY2hhci92aXJ0aW9fY29uc29sZS5jIHwgIDE0MSArKysrKysrKysrKysr KysrKystLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQo+ICAxIGZpbGVzIGNoYW5nZWQsIDYyIGluc2Vy dGlvbnMoKyksIDc5IGRlbGV0aW9ucygtKQo+IAo+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2NoYXIv dmlydGlvX2NvbnNvbGUuYyBiL2RyaXZlcnMvY2hhci92aXJ0aW9fY29uc29sZS5jCj4gaW5kZXgg OGFiOWMzZC4uZjRmN2IwNCAxMDA2NDQKPiAtLS0gYS9kcml2ZXJzL2NoYXIvdmlydGlvX2NvbnNv bGUuYwo+ICsrKyBiL2RyaXZlcnMvY2hhci92aXJ0aW9fY29uc29sZS5jCj4gQEAgLTExMSw2ICsx MTEsMTEgQEAgc3RydWN0IHBvcnRfYnVmZmVyIHsKPiAgCXNpemVfdCBsZW47Cj4gIAkvKiBvZmZz ZXQgaW4gdGhlIGJ1ZiBmcm9tIHdoaWNoIHRvIGNvbnN1bWUgZGF0YSAqLwo+ICAJc2l6ZV90IG9m ZnNldDsKPiArCj4gKwkvKiBJZiBzZ3BhZ2VzID09IDAgdGhlbiBidWYgaXMgdXNlZCwgZWxzZSBz ZyBpcyB1c2VkICovCj4gKwl1bnNpZ25lZCBpbnQgc2dwYWdlczsKPiArCj4gKwlzdHJ1Y3Qgc2Nh dHRlcmxpc3Qgc2dbMV07Cj4gIH07Cj4gIAo+ICAvKgo+IEBAIC0zMzgsMjMgKzM0Myw0NiBAQCBz dGF0aWMgaW5saW5lIGJvb2wgdXNlX211bHRpcG9ydChzdHJ1Y3QgcG9ydHNfZGV2aWNlICpwb3J0 ZGV2KQo+ICAKPiAgc3RhdGljIHZvaWQgZnJlZV9idWYoc3RydWN0IHBvcnRfYnVmZmVyICpidWYp Cj4gIHsKPiArCWludCBpOwo+ICsKPiAgCWtmcmVlKGJ1Zi0+YnVmKTsKCnRoaXMgc2hvdWxkIGJl IGRvbmUgb25seSB3aGVuICFidWYtPnNncGFnZXMsIG9yIChzZWUgYmVsb3cpCgo+ICsKPiArCWlm IChidWYtPnNncGFnZXMpCj4gKwkJZm9yIChpID0gMDsgaSA8IGJ1Zi0+c2dwYWdlczsgaSsrKSB7 Cj4gKwkJCXN0cnVjdCBwYWdlICpwYWdlID0gc2dfcGFnZSgmYnVmLT5zZ1tpXSk7Cj4gKwkJCWlm ICghcGFnZSkKPiArCQkJCWJyZWFrOwo+ICsJCQlwdXRfcGFnZShwYWdlKTsKPiArCQl9Cj4gKwo+ ICAJa2ZyZWUoYnVmKTsKPiAgfQo+ICAKPiAtc3RhdGljIHN0cnVjdCBwb3J0X2J1ZmZlciAqYWxs b2NfYnVmKHNpemVfdCBidWZfc2l6ZSkKPiArc3RhdGljIHN0cnVjdCBwb3J0X2J1ZmZlciAqYWxs b2NfYnVmKHN0cnVjdCB2aXJ0cXVldWUgKnZxLCBzaXplX3QgYnVmX3NpemUsCj4gKwkJCQkgICAg IGludCBucmJ1ZnMpCj4gIHsKPiAgCXN0cnVjdCBwb3J0X2J1ZmZlciAqYnVmOwo+ICsJc2l6ZV90 IGFsbG9jX3NpemU7Cj4gIAo+IC0JYnVmID0ga21hbGxvYyhzaXplb2YoKmJ1ZiksIEdGUF9LRVJO RUwpOwo+ICsJLyogQWxsb2NhdGUgYnVmZmVyIGFuZCB0aGUgc2NhdHRlciBsaXN0ICovCj4gKwlh bGxvY19zaXplID0gc2l6ZW9mKCpidWYpICsgc2l6ZW9mKHN0cnVjdCBzY2F0dGVybGlzdCkgKiBu cmJ1ZnM7CgpUaGlzIGFsbG9jYXRlcyBvbmUgcmVkdW5kYW50IHNnIGVudHJ5IHdoZW4gbnJidWYg PiAwLApidXQgSSB0aGluayBpdCBpcyBPSy4gKGp1c3QgYSBjb21tZW50KQoKPiArCWJ1ZiA9IGtt YWxsb2MoYWxsb2Nfc2l6ZSwgR0ZQX0FUT01JQyk7CgpUaGlzIHNob3VsZCBiZSBremFsbG9jKCks IG9yIGJ1Zi0+YnVmIGFuZCBvdGhlcnMgYXJlIG5vdCBpbml0aWFsaXplZCwKd2hpY2ggd2lsbCBj YXVzZSB1bmV4cGVjdGVkIGtmcmVlIGJ1ZyBhdCBrZnJlZShidWYtPmJ1ZikgaW4gZnJlZV9idWYu Cgo+ICAJaWYgKCFidWYpCj4gIAkJZ290byBmYWlsOwo+IC0JYnVmLT5idWYgPSBremFsbG9jKGJ1 Zl9zaXplLCBHRlBfS0VSTkVMKTsKPiArCj4gKwlidWYtPnNncGFnZXMgPSBucmJ1ZnM7Cj4gKwlp ZiAobnJidWZzID4gMCkKPiArCQlyZXR1cm4gYnVmOwo+ICsKPiArCWJ1Zi0+YnVmID0ga21hbGxv YyhidWZfc2l6ZSwgR0ZQX0FUT01JQyk7CgpZb3UgY2FuIGFsc28gdXNlIGt6YWxsb2MgaGVyZSBh cyBwcmV2aW91cyBjb2RlIGRvZXMuCkJ1dCBpZiB0aGUgcmVhc29uIHdoeSB1c2luZyBremFsbG9j IGNvbWVzIGZyb20gdGhlIHNlY3VyaXR5LApJIHRoaW5rIGttYWxsb2MgaXMgZW5vdWdoIGhlcmUs IHNpbmNlIHRoZSBob3N0IGNhbiBhY2Nlc3MKYWxsIHRoZSBndWVzdCBwYWdlcyBhbnl3YXkuCgo+ ICAJaWYgKCFidWYtPmJ1ZikKPiAgCQlnb3RvIGZyZWVfYnVmOwo+ICAJYnVmLT5sZW4gPSAwOwo+ ICAJYnVmLT5vZmZzZXQgPSAwOwo+ICAJYnVmLT5zaXplID0gYnVmX3NpemU7Cj4gKwo+ICsJLyog UHJlcGFyZSBzY2F0dGVyIGJ1ZmZlciBmb3Igc2VuZGluZyAqLwo+ICsJc2dfaW5pdF9vbmUoYnVm LT5zZywgYnVmLT5idWYsIGJ1Zl9zaXplKTsKPiAgCXJldHVybiBidWY7Cj4gIAo+ICBmcmVlX2J1 ZjoKClRoYW5rIHlvdSwKCgotLSAKTWFzYW1pIEhJUkFNQVRTVQpTb2Z0d2FyZSBQbGF0Zm9ybSBS ZXNlYXJjaCBEZXB0LiBMaW51eCBUZWNobm9sb2d5IENlbnRlcgpIaXRhY2hpLCBMdGQuLCBZb2tv aGFtYSBSZXNlYXJjaCBMYWJvcmF0b3J5CkUtbWFpbDogbWFzYW1pLmhpcmFtYXRzdS5wdEBoaXRh Y2hpLmNvbQoKCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f ClZpcnR1YWxpemF0aW9uIG1haWxpbmcgbGlzdApWaXJ0dWFsaXphdGlvbkBsaXN0cy5saW51eC1m b3VuZGF0aW9uLm9yZwpodHRwczovL2xpc3RzLmxpbnV4Zm91bmRhdGlvbi5vcmcvbWFpbG1hbi9s aXN0aW5mby92aXJ0dWFsaXphdGlvbg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751481Ab2IZCon (ORCPT ); Tue, 25 Sep 2012 22:44:43 -0400 Received: from mail4.hitachi.co.jp ([133.145.228.5]:57237 "EHLO mail4.hitachi.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751051Ab2IZCom (ORCPT ); Tue, 25 Sep 2012 22:44:42 -0400 X-AuditID: b753bd60-96850ba000002f78-04-50626c17ae58 X-AuditID: b753bd60-96850ba000002f78-04-50626c17ae58 Message-ID: <50626C11.9040708@hitachi.com> Date: Wed, 26 Sep 2012 11:44:33 +0900 From: Masami Hiramatsu Organization: Hitachi, Ltd., Japan User-Agent: Mozilla/5.0 (Windows NT 5.2; rv:13.0) Gecko/20120614 Thunderbird/13.0.1 MIME-Version: 1.0 To: sjur.brandeland@stericsson.com Cc: Amit Shah , linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, sjurbren@stericsson.com, Rusty Russell , "Michael S. Tsirkin" , Linus Walleij , yrl.pp-manager.tt@hitachi.com Subject: Re: [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer References: <1348580837-10919-1-git-send-email-sjur.brandeland@stericsson.com> <1348580837-10919-2-git-send-email-sjur.brandeland@stericsson.com> In-Reply-To: <1348580837-10919-2-git-send-email-sjur.brandeland@stericsson.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Brightmail-Tracker: AAAAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org (2012/09/25 22:47), sjur.brandeland@stericsson.com wrote: > From: Sjur Brændeland > > 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. > Signed-off-by: Sjur Brændeland > cc: Rusty Russell > cc: Michael S. Tsirkin > cc: Amit Shah > cc: Linus Walleij > cc: Masami Hiramatsu > --- > drivers/char/virtio_console.c | 141 ++++++++++++++++++----------------------- > 1 files changed, 62 insertions(+), 79 deletions(-) > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c > index 8ab9c3d..f4f7b04 100644 > --- a/drivers/char/virtio_console.c > +++ b/drivers/char/virtio_console.c > @@ -111,6 +111,11 @@ struct port_buffer { > size_t len; > /* offset in the buf from which to consume data */ > size_t offset; > + > + /* If sgpages == 0 then buf is used, else sg is used */ > + unsigned int sgpages; > + > + struct scatterlist sg[1]; > }; > > /* > @@ -338,23 +343,46 @@ static inline bool use_multiport(struct ports_device *portdev) > > static void free_buf(struct port_buffer *buf) > { > + int i; > + > kfree(buf->buf); this should be done only when !buf->sgpages, or (see 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) > + 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. > 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. > if (!buf->buf) > goto free_buf; > buf->len = 0; > buf->offset = 0; > buf->size = buf_size; > + > + /* Prepare scatter buffer for sending */ > + sg_init_one(buf->sg, buf->buf, buf_size); > return buf; > > free_buf: Thank you, -- Masami HIRAMATSU Software Platform Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu.pt@hitachi.com