Re: [PATCH] Xen backend support for paged out grant targets.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Sep 12, 2012 at 09:20:24AM -0400, Andres Lagar-Cavilla wrote:
> On Sep 5, 2012, at 1:21 PM, Andres Lagar-Cavilla wrote:
> 
> > 
> > On Sep 5, 2012, at 12:27 PM, Konrad Rzeszutek Wilk wrote:
> > 
> >> On Fri, Aug 31, 2012 at 05:10:56PM +0100, David Vrabel wrote:
> >>> On 31/08/12 16:42, Andres Lagar-Cavilla wrote:
> >>>> Actually acted upon your feedback ipso facto:
> >>>> 
> >>>> commit d5fab912caa1f0cf6be0a6773f502d3417a207b6
> >>>> Author: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> >>>> Date:   Sun Aug 26 09:45:57 2012 -0400
> >>>> 
> >>>>   Xen backend support for paged out grant targets.
> >>> 
> >>> This looks mostly fine expect for the #define instead of inline functions.
> >>> 
> >>>> +#define gnttab_map_grant_no_eagain(_gop)                                    \
> >>> 
> >>> This name tripped me up previously. As I read this as:
> >>> 
> >>> gnttab_map_grant_no_[retries_for]_eagain().
> >>> 
> >>> Perhaps gnttab_map_grant_with_retries() ? Or similar?
> >> 
> >> gnttab_map_grant_retry ?
> >> 
> >> Besides that, it looks good to me. Ian Campbell needs to Ack so that
> >> David Miller (the network maintainer) can pick it up. Please CC them
> >> both and also LKML
> Konrad, Ian, can I get some insight on the question I pose below. I have otherwise cleaned up the patch for 3.7.

Can you re-send a cleanup patch and CC the relevant folks pls.

> 
> Thanks
> Andres
> >> .
> > 
> > Sure will do. However, I need to bring attention to the following hunk:
> > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> > index 682633b..5610fd8 100644
> > --- a/drivers/net/xen-netback/netback.c
> > +++ b/drivers/net/xen-netback/netback.c
> > @@ -635,9 +635,7 @@ static void xen_netbk_rx_action(struct xen_netbk *netbk)
> > 		return;
> > 
> > 	BUG_ON(npo.copy_prod > ARRAY_SIZE(netbk->grant_copy_op));
> > -	ret = HYPERVISOR_grant_table_op(GNTTABOP_copy, &netbk->grant_copy_op,
> > -					npo.copy_prod);
> > -	BUG_ON(ret != 0);
> > +	gnttab_batch_copy_no_eagain(netbk->grant_copy_op, npo.copy_prod);
> > 
> > It seems like the (extant) code passes a struct gnttab_copy ** to the grant table hypercall (&netbk->grant_copy_op, defined as struct gnttab_copy [])
> > 
> > I "fixed" it to pass a struct gnttab_copy * (netbk->grant_copy_op, no '&'). This matches the other invocation of the grant copy hyper call (in netbk_tx_action). Did I fix it, though? I am confused as to how this could have ever worked.
> > 
> > (cc'ing Ian Campbell for more insight) 
> > Andres
> >>> 
> >>> David
> >