Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751926AbbHJLjM (ORCPT ); Mon, 10 Aug 2015 07:39:12 -0400 Received: from smtp.citrix.com ([66.165.176.89]:43535 "EHLO SMTP.CITRIX.COM" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751811AbbHJLjJ (ORCPT ); Mon, 10 Aug 2015 07:39:09 -0400 X-IronPort-AV: E=Sophos;i="5.15,645,1432598400"; d="scan'208";a="289610940" Date: Mon, 10 Aug 2015 12:39:07 +0100 From: Wei Liu To: Julien Grall CC: Wei Liu , , , , , , Subject: Re: [PATCH v3 18/20] net/xen-netback: Make it running on 64KB page granularity Message-ID: <20150810113907.GH8857@zion.uk.xensource.com> References: <1438966019-19322-1-git-send-email-julien.grall@citrix.com> <1438966019-19322-19-git-send-email-julien.grall@citrix.com> <20150808145534.GB14214@zion.uk.xensource.com> <55C8759C.3010507@citrix.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <55C8759C.3010507@citrix.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-DLP: MIA2 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2563 Lines: 85 On Mon, Aug 10, 2015 at 10:57:48AM +0100, Julien Grall wrote: [...] > > >>+ info.page = page; > >>+ gnttab_foreach_grant_in_range(page, offset, bytes, > >>+ xenvif_gop_frag_copy_grant, > >>+ &info); > > > >Looks like I need to at least wait until the API is settle before giving > >my ack. > > > >> size -= bytes; > >>+ offset = 0; > > > >This looks wrong. Should be offset += bytes. > > With the new implementation of the loop, each iteration will be on a > different page. > So only the first page has an offset different than zero. > > > > >> > >>- /* Next frame */ > >>- if (offset == PAGE_SIZE && size) { > >>+ /* Next page */ > >>+ if (size) { > >> BUG_ON(!PageCompound(page)); > >> page++; > >>- offset = 0; > > > >And this should not be deleted, I think. > > > >What is the reason for changing offset calculation? I think there is > >still compound page when using 64K page. > > The compound pages are still working ... gnttab_foreach_grant_in_range is > called once per page. So the offset can be reset to 0 every time. No need to > add code which would make the result less clear. > > We only need to know if the size is not 0 to get the next page. > > The patch may not be clear enough to see it's working so I've copied the > result loop below: > > while (size > 0) { > BUG_ON(offset >= PAGE_SIZE); > > bytes = PAGE_SIZE - offset; > if (bytes > size) > bytes = size; > > info.page = page; > gnttab_foreach_grant_in_range(page, offset, bytes, > xenvif_gop_frag_copy_grant, > &info); > size -= bytes; > offset = 0; > > /* Next page */ > if (size) { > BUG_ON(!PageCompound(page)); > page++; > } > } > Right. That doesn't mean the original code was wrong or anything. But I don't want to bikeshed about this. Please add a comment saying that offset is always 0 starting from second iteration because the gnttab_foreach_grant_in_range makes sure we handle one page in one go. Wei. > Regards, > > -- > Julien Grall -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/