From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andres Lagar-Cavilla Subject: Re: [PATCH] Xen backend support for paged out grant targets. Date: Wed, 5 Sep 2012 13:21:56 -0400 Message-ID: <4FAD394B-6A6C-44F1-AF1C-DC2F935639EE@gmail.com> References: <1346086287-17674-1-git-send-email-andres@lagarcavilla.org> <5040CAEA.7000600@citrix.com> <160CC375-2682-4CBF-B1EC-06A9F3E49A40@gridcentric.ca> <5040E210.2060702@citrix.com> <20120905162731.GD11949@phenom.dumpdata.com> Mime-Version: 1.0 (Apple Message framework v1278) Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20120905162731.GD11949@phenom.dumpdata.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Konrad Rzeszutek Wilk Cc: Andres Lagar-Cavilla , Ian Campbell , xen-devel@lists.xen.org, David Vrabel , Andres Lagar-Cavilla List-Id: xen-devel@lists.xenproject.org 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 >>> 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. 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