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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.03.13 at 15:32, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 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@xxxxxxxxxx>
> 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@xxxxxxxxxx>
>     Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>     Cc: xen-devel@xxxxxxxxxxxxx 
>     [v1: s/buf/offset/]
>     Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxx 
> http://lists.xen.org/xen-devel 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel