Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751836AbbKYRkx (ORCPT ); Wed, 25 Nov 2015 12:40:53 -0500 Received: from aserp1040.oracle.com ([141.146.126.69]:26877 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751061AbbKYRkq (ORCPT ); Wed, 25 Nov 2015 12:40:46 -0500 Date: Wed, 25 Nov 2015 12:40:34 -0500 From: Konrad Rzeszutek Wilk To: Bob Liu Cc: xen-devel@lists.xen.org, linux-kernel@vger.kernel.org, roger.pau@citrix.com, felipe.franciosi@citrix.com, axboe@fb.com, avanzini.arianna@gmail.com, rafal.mielniczuk@citrix.com, jonathan.davies@citrix.com, david.vrabel@citrix.com Subject: Re: [PATCH v5 07/10] xen/blkback: pseudo support for multi hardware queues/rings Message-ID: <20151125174034.GB19188@x230.dumpdata.com> References: <1447470739-18136-1-git-send-email-bob.liu@oracle.com> <1447470739-18136-8-git-send-email-bob.liu@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1447470739-18136-8-git-send-email-bob.liu@oracle.com> User-Agent: Mutt/1.5.24 (2015-08-30) X-Source-IP: userv0021.oracle.com [156.151.31.71] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4586 Lines: 135 > @@ -113,19 +115,55 @@ static void xen_update_blkif_status(struct xen_blkif *blkif) > } > invalidate_inode_pages2(blkif->vbd.bdev->bd_inode->i_mapping); > > - blkif->ring.xenblkd = kthread_run(xen_blkif_schedule, &blkif->ring, "%s", name); > - if (IS_ERR(blkif->ring.xenblkd)) { > - err = PTR_ERR(blkif->ring.xenblkd); > - blkif->ring.xenblkd = NULL; > - xenbus_dev_error(blkif->be->dev, err, "start xenblkd"); > - return; > + for (i = 0; i < blkif->nr_rings; i++) { > + ring = &blkif->rings[i]; > + ring->xenblkd = kthread_run(xen_blkif_schedule, ring, "%s-%d", name, i); > + if (IS_ERR(ring->xenblkd)) { > + err = PTR_ERR(ring->xenblkd); > + ring->xenblkd = NULL; > + xenbus_dev_fatal(blkif->be->dev, err, > + "start %s-%d xenblkd", name, i); > + goto out; > + } > + } > + return; > + > +out: > + while (--i >= 0) { > + ring = &blkif->rings[i]; > + kthread_stop(ring->xenblkd); That won't work. Imagine us failing at the start of the loop above, so i==0. We get here and decrement and unsigned int by one, and loop back to 0xffffffffff. Naturally 0xffff.. >= 0 so we will just continue one going over the blkif->rings[0xffffff].. and BOOM! This worked when 'i' was 'int', but now it is unsigned int. Let me make it 'int' and then this works, or we can swap the loop around and use 'i-1' to use the previous entry. [Fixed it up in my tree] > } > + return; > +} > + .. snip.. > +static int connect_ring(struct backend_info *be) > +{ > + struct xenbus_device *dev = be->dev; > + unsigned int pers_grants; > + char protocol[64] = ""; > + int err, i; > + char *xspath; > + size_t xspathsize; > + const size_t xenstore_path_ext_size = 11; /* sufficient for "/queue-NNN" */ > + > + pr_debug("%s %s\n", __func__, dev->otherend); > + > + be->blkif->blk_protocol = BLKIF_PROTOCOL_DEFAULT; > + err = xenbus_gather(XBT_NIL, dev->otherend, "protocol", > + "%63s", protocol, NULL); > + if (err) > + strcpy(protocol, "unspecified, assuming default"); > + else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_NATIVE)) > + be->blkif->blk_protocol = BLKIF_PROTOCOL_NATIVE; > + else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_32)) > + be->blkif->blk_protocol = BLKIF_PROTOCOL_X86_32; > + else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_64)) > + be->blkif->blk_protocol = BLKIF_PROTOCOL_X86_64; > + else { > + xenbus_dev_fatal(dev, err, "unknown fe protocol %s", protocol); > + return -1; > + } > + err = xenbus_gather(XBT_NIL, dev->otherend, > + "feature-persistent", "%u", > + &pers_grants, NULL); > + if (err) > + pers_grants = 0; > + > + be->blkif->vbd.feature_gnt_persistent = pers_grants; > + be->blkif->vbd.overflow_max_grants = 0; > + > + pr_info("%s: using %d queues, protocol %d (%s) %s\n", dev->nodename, > + be->blkif->nr_rings, be->blkif->blk_protocol, protocol, > + pers_grants ? "persistent grants" : ""); > + > + if (be->blkif->nr_rings == 1) > + return read_per_ring_refs(&be->blkif->rings[0], dev->otherend); > + else { > + xspathsize = strlen(dev->otherend) + xenstore_path_ext_size; > + xspath = kmalloc(xspathsize, GFP_KERNEL); > + if (!xspath) { > + xenbus_dev_fatal(dev, -ENOMEM, "reading ring references"); > + return -ENOMEM; > + } > + > + for (i = 0; i < be->blkif->nr_rings; i++) { > + memset(xspath, 0, xspathsize); > + snprintf(xspath, xspathsize, "%s/queue-%u", dev->otherend, i); > + err = read_per_ring_refs(&be->blkif->rings[i], xspath); Say nr_rings is 4 and this fails at the last one. That means be->blkif->rings[0..2].pending_free has a bunch of pages and also ring->blk_ring are set. We return out of this function and end back in (frontend_changed): 752 err = connect_ring(be); 753 if (err) 754 break; Great. So we have a memory leak until the device goes in XenbusStateConnected (where we end up calling xen_blkif_disconnect and free ring[0..2].. But that may take a while if the guest is not nice. Perhaps we should add in frontend_changed(..) an call to xen_blkif_disconnect in case we fail at 'connect_ring' to clear the memory faster. I will prep a patch for that. > + if (err) { > + kfree(xspath); > + return err; > + } > + } > + kfree(xspath); > + } > + return 0; > } > > static const struct xenbus_device_id xen_blkbk_ids[] = { > -- > 1.7.10.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/