From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH 1/5] xen-blkback: don't store dev_bus_addr Date: Tue, 19 Mar 2013 11:10:00 -0400 Message-ID: <20130319151000.GA8550__47013.1663642407$1363706108$gmane$org@phenom.dumpdata.com> References: <1363625376-35612-1-git-send-email-roger.pau@citrix.com> <1363625376-35612-2-git-send-email-roger.pau@citrix.com> <20130319143217.GC32432@phenom.dumpdata.com> <51488A8C02000078000C6C20@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: <51488A8C02000078000C6C20@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel@lists.xen.org, linux-kernel@vger.kernel.org, Roger Pau Monne List-Id: xen-devel@lists.xenproject.org On Tue, Mar 19, 2013 at 02:55:56PM +0000, Jan Beulich wrote: > >>> On 19.03.13 at 15:32, Konrad Rzeszutek Wilk = wrote: > > On Mon, Mar 18, 2013 at 05:49:32PM +0100, Roger Pau Monne wrote: > >> dev_bus_addr returned in the grant ref map operation is the mfn of the > >> passed page, there's no need to store it in the persistent grant > >> entry, since we can always get it provided that we have the page. > >> = > >> This reduces the memory overhead of persistent grants in blkback. > > = > > I took this patch, but I redid it a bit: > > = > > commit 1d4cb410befdb8b373c6fad604b39e0200e0bee0 > > Author: Roger Pau Monne > > Date: Mon Mar 18 17:49:32 2013 +0100 > > = > > xen-blkback: don't store dev_bus_addr > > = > > dev_bus_addr returned in the grant ref map operation is the mfn of = the > > passed page, there's no need to store it in the persistent grant > > entry, since we can always get it provided that we have the page. > > = > > This reduces the memory overhead of persistent grants in blkback. > > = > > While at it, rename the 'seg[i].buf' to be 'seg[i].offset' as > > it makes much more sense - as we use that value in bio_add_page > > which as the fourth argument expects the offset. > > = > > We hadn't used the physical address as part of this at all. > > = > > Signed-off-by: Roger Pau Monn=E9 > > Cc: Konrad Rzeszutek Wilk > > Cc: xen-devel@lists.xen.org = > > [v1: s/buf/offset/] > > Signed-off-by: Konrad Rzeszutek Wilk > > = > > diff --git a/drivers/block/xen-blkback/blkback.c = > > b/drivers/block/xen-blkback/blkback.c > > index 2cf8381..061c202 100644 > > --- a/drivers/block/xen-blkback/blkback.c > > +++ b/drivers/block/xen-blkback/blkback.c > > @@ -442,7 +442,7 @@ int xen_blkif_schedule(void *arg) > > } > > = > > struct seg_buf { > > - unsigned long buf; > > + unsigned long offset; > = > If you touch this anyway, why don't you reduce the type to > "unsigned int", halving the overall structure size? > = > Even more, the field seems pointless to me altogether, since ... > = > > unsigned int nsec; > > }; > > /* > > @@ -621,30 +621,21 @@ static int xen_blkbk_map(struct blkif_request *re= q, > > * If this is a new persistent grant > > * save the handler > > */ > > - persistent_gnts[i]->handle =3D map[j].handle; > > - persistent_gnts[i]->dev_bus_addr =3D > > - map[j++].dev_bus_addr; > > + persistent_gnts[i]->handle =3D map[j++].handle; > > } > > pending_handle(pending_req, i) =3D > > persistent_gnts[i]->handle; > > = > > if (ret) > > continue; > > - > > - seg[i].buf =3D persistent_gnts[i]->dev_bus_addr | > > - (req->u.rw.seg[i].first_sect << 9); > > } else { > > - pending_handle(pending_req, i) =3D map[j].handle; > > + pending_handle(pending_req, i) =3D map[j++].handle; > > bitmap_set(pending_req->unmap_seg, i, 1); > > = > > - if (ret) { > > - j++; > > + if (ret) > > continue; > > - } > > - > > - seg[i].buf =3D map[j++].dev_bus_addr | > > - (req->u.rw.seg[i].first_sect << 9); > > } > > + seg[i].offset =3D (req->u.rw.seg[i].first_sect << 9); > = > ... this uses "i" as index on both sides, so ... > = > > } > > return ret; > > } > > @@ -971,7 +962,7 @@ static int dispatch_rw_block_io(struct xen_blkif *b= lkif, > > (bio_add_page(bio, > > pages[i], > > seg[i].nsec << 9, > > - seg[i].buf & ~PAGE_MASK) =3D=3D 0)) { > > + seg[i].offset & ~PAGE_MASK) =3D=3D 0)) { > = > ... this one could as well use the original field. > = > And the masking with ~PAGE_MASK is not pointless in any case. Good point. In which might as well make the 'struct seg_buf' be an simple array of unsigned int. > = > Jan > = > > = > > bio =3D bio_alloc(GFP_KERNEL, nseg-i); > > if (unlikely(bio =3D=3D NULL)) > > diff --git a/drivers/block/xen-blkback/common.h = > > b/drivers/block/xen-blkback/common.h > > index da78346..60103e2 100644 > > --- a/drivers/block/xen-blkback/common.h > > +++ b/drivers/block/xen-blkback/common.h > > @@ -187,7 +187,6 @@ struct persistent_gnt { > > struct page *page; > > grant_ref_t gnt; > > grant_handle_t handle; > > - uint64_t dev_bus_addr; > > struct rb_node node; > > }; > > = > > = > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xen.org = > > http://lists.xen.org/xen-devel = > = > =