linux-kernel - Re: [Xen-devel] [PATCH 1/5] xen-blkback: don't store dev_bus

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <51488A8C02000078000C6C20@nat28.tlf.novell.com>
Date:	Tue, 19 Mar 2013 14:55:56 +0000
From:	"Jan Beulich" <JBeulich@...e.com>
To:	"Roger Pau Monne" <roger.pau@...rix.com>,
	"Konrad Rzeszutek Wilk" <konrad.wilk@...cle.com>
Cc:	<xen-devel@...ts.xen.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [Xen-devel] [PATCH 1/5] xen-blkback: don't store
 dev_bus_addr

>>> On 19.03.13 at 15:32, Konrad Rzeszutek Wilk <konrad.wilk@...cle.com> 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 <roger.pau@...rix.com>
> 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é <roger.pau@...rix.com>
>     Cc: Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
>     Cc: xen-devel@...ts.xen.org 
>     [v1: s/buf/offset/]
>     Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
> 
> 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 *req,
>  				 * 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;
> +				persistent_gnts[i]->handle = map[j++].handle;
>  			}
>  			pending_handle(pending_req, i) =
>  				persistent_gnts[i]->handle;
>  
>  			if (ret)
>  				continue;
> -
> -			seg[i].buf = persistent_gnts[i]->dev_bus_addr |
> -				(req->u.rw.seg[i].first_sect << 9);
>  		} else {
> -			pending_handle(pending_req, i) = map[j].handle;
> +			pending_handle(pending_req, i) = map[j++].handle;
>  			bitmap_set(pending_req->unmap_seg, i, 1);
>  
> -			if (ret) {
> -				j++;
> +			if (ret)
>  				continue;
> -			}
> -
> -			seg[i].buf = map[j++].dev_bus_addr |
> -				(req->u.rw.seg[i].first_sect << 9);
>  		}
> +		seg[i].offset = (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 *blkif,
>  		       (bio_add_page(bio,
>  				     pages[i],
>  				     seg[i].nsec << 9,
> -				     seg[i].buf & ~PAGE_MASK) == 0)) {
> +				     seg[i].offset & ~PAGE_MASK) == 0)) {

... this one could as well use the original field.

And the masking with ~PAGE_MASK is not pointless in any case.

Jan

>  
>  			bio = bio_alloc(GFP_KERNEL, nseg-i);
>  			if (unlikely(bio == 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@...ts.xen.org 
> http://lists.xen.org/xen-devel 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/