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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Tue, Mar 19, 2013 at 02:55:56PM +0000, Jan Beulich wrote:
> >>> 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.

Good point. In which might as well make the 'struct seg_buf' be an
simple array of unsigned int.

> 
> 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