Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758202Ab2JXKpW (ORCPT ); Wed, 24 Oct 2012 06:45:22 -0400 Received: from smtp.eu.citrix.com ([62.200.22.115]:11106 "EHLO SMTP.EU.CITRIX.COM" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758111Ab2JXKpT (ORCPT ); Wed, 24 Oct 2012 06:45:19 -0400 X-IronPort-AV: E=Sophos;i="4.80,639,1344211200"; d="scan'208";a="15355111" Message-ID: <5087C6BC.7070109@citrix.com> Date: Wed, 24 Oct 2012 12:45:16 +0200 From: =?ISO-8859-1?Q?Roger_Pau_Monn=E9?= User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:16.0) Gecko/20121010 Thunderbird/16.0.1 MIME-Version: 1.0 To: Konrad Rzeszutek Wilk CC: Konrad Rzeszutek Wilk , "linux-kernel@vger.kernel.org" , "xen-devel@lists.xen.org" Subject: Re: [Xen-devel] [PATCH RFC] Persistent grant maps for xen blk drivers References: <1350559321-19066-1-git-send-email-roger.pau@citrix.com> <20121022134708.GA13832@konrad-lan.dumpdata.com> <5086C0C8.5000306@citrix.com> <20121023172008.GB11787@phenom.dumpdata.com> <5086DD57.4000206@citrix.com> <20121023185049.GB20350@phenom.dumpdata.com> In-Reply-To: <20121023185049.GB20350@phenom.dumpdata.com> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4149 Lines: 106 On 23/10/12 20:50, Konrad Rzeszutek Wilk wrote: > On Tue, Oct 23, 2012 at 08:09:27PM +0200, Roger Pau Monn? wrote: >> On 23/10/12 19:20, Konrad Rzeszutek Wilk wrote: >>>>>> diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c >>>>>> index c6decb9..2b982b2 100644 >>>>>> --- a/drivers/block/xen-blkback/blkback.c >>>>>> +++ b/drivers/block/xen-blkback/blkback.c >>>>>> @@ -78,6 +78,7 @@ struct pending_req { >>>>>> unsigned short operation; >>>>>> int status; >>>>>> struct list_head free_list; >>>>>> + unsigned int unmap_seg[BLKIF_MAX_SEGMENTS_PER_REQUEST]; >> >> Should I change this to a bool? Since we are only setting it to 0 or 1. > > I would just keep it as 'int'. Eventually we can replace this with a > bit-map, but that can be done later. I've already changed it to a bitmap. >>>>> Perhaps there should be a #define for that array.. >>>> >>>> Do you mean something like: >>>> >>>> #define unmap(req, i) req->unmap_seg[i] >>> >>> I was thinking that you just check for req->unamp_seg[i] to >>> have an non-zero value. But since that array is just used as an check >>> to see whether the functionality is enabled (or not), you might want >>> to declerare the right values so: >>> #define UNMAP_SG_ON 1 >>> #define UNMAP_SG_OFF 0 >>> >>> or so. >> >> Agreed, will add the defines. >> >>>>>> + if (persistent_gnts[i]) { >>>>>> + if (!persistent_gnts[i]->handle) { >>>>>> + /* >>>>>> + * If this is a new persistent grant >>>>>> + * save the handler >>>>>> + */ >>>>>> + persistent_gnts[i]->handle = map[j].handle; >>>>>> + persistent_gnts[i]->dev_bus_addr = >>>>>> + map[j++].dev_bus_addr; >>>>>> + } >>>>>> + pending_handle(pending_req, i) = >>>>>> + persistent_gnts[i]->handle; >>>>>> + pending_req->unmap_seg[i] = 0; >>>>> >>>>> Could we have a #define for that? >>>> >>>> Sure. >> >> I've used the previous macro, so it looks like: >> >> unmap(req, i) = UNMAP_SG_OFF; >> >> I'm not sure if this is what you meant, or if you where interested in >> defining a set of macros like: >> >> #define check_unmap(req, i) req->unmap_seg[i] >> #define unset_unmap(req, i) req->unmap_seg[i] = UNMAP_SG_OFF >> #define set_unmap(req, i) req->unmap_seg[i] = UNMAP_SG_ON >> >> I would go for the first option (the unmap macro that can be used here >> and in xen_blkbk_unmap). > > I was just thinking something as simple as > > if (reg->unmap_seg[i] == UNMAP_SG_OFF) > continue; > > And the #defines are just for the hard-coded values of 0 or 1. > >> >>>>> HA! By default, eh? >>>> >>>> Yes, you caught me, there's a paragraph in the commit message that >>>> explains that we are using persistent grants in the frontend >>>> unconditionally, since the protocol is compatible (you can have a >>>> persistent blkfront and a non-persistent blkback). It simplifies the >>>> logic in blkfront. Are you OK with it? >>> >>> It is OK, but you should be checking whether the backend supports it. >>> I don't see it checking the info->feature_persistent_grant to print >>> that. >> >> I don't understand why blkfront needs to check if the backend supports >> persisten grants, blkfront is going to use persistent grants anyway. > > What if it does not (say this guest runs on an older xen-blkback?)? > Then you would be still printing 'persistent grants' in the blkfront. Ok, I get your point. Now blkfront will only report the use of persistent grants if the backend supports it. If there are no further comments I will send v2 after doing some tests, thanks for the reviews. -- 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/