Re: [Xen-devel] [PATCH RFC] Persistent grant maps for xen blk drivers

[Konrad Rzeszutek Wilk <konrad@kernel.org>]

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.

> 
> >>> 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.
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>