RE: [PATCH V2 1/6] Drivers: net: hyperv: Enable scatter gather I/O

[KY Srinivasan <kys@microsoft.com>]

> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Saturday, March 8, 2014 3:18 AM
> To: KY Srinivasan
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> jasowang@redhat.com
> Subject: Re: [PATCH V2 1/6] Drivers: net: hyperv: Enable scatter gather I/O
> 
> From: "K. Y. Srinivasan" <kys@microsoft.com>
> Date: Thu,  6 Mar 2014 21:32:36 -0800
> 
> > +static u32 fill_pg_buf(struct page *page, u32 offset, u32 len,
> > +			struct hv_page_buffer *pb)
> > +{
> > +	int j = 0;
> > +
> > +	/* Deal with compund pages by ignoring unused part
> > +	 * of the page.
> > +	 */
> > +	page += offset  >> PAGE_SHIFT;
> 
> Please only one space between "offset" and ">>"
> 
> > +	offset &= ~PAGE_MASK;
> > +
> > +	while (len > 0) {
> > +		unsigned long bytes;
> > +
> > +		bytes = PAGE_SIZE - offset;
> > +		if (bytes > len)
> > +			bytes = len;
> > +		pb[j].pfn = page_to_pfn(page);
> > +		pb[j].offset = offset;
> > +		pb[j].len = bytes;
> > +
> > +		offset += bytes;
> > +		len -= bytes;
> > +
> > +		if (offset == PAGE_SIZE && len) {
> > +			page++;
> > +			offset = 0;
> > +			j++;
> > +		}
> > +	}
> > +
> > +	return j + 1;
> > +}
> 
> I think this function has some edge case errors.
> 
> As I understand it, this function returns how many page buffer entries were
> filled in.
> 
> But if we fill exactly the end of a page, we will report one too many in the
> return value.

I may be missing something here; but in the case you are describing we would return a value
of one as we should. When offset + len is exactly equal to the page size, we would not increment
the value of j, since at that point len == 0. Since the initial value of j was 0 and we return (j + 1),
we will return 1.

Regards,

K. Y
> 
> Consider an "offset" of X, where X is smaller than PAGE_SIZE.  And a "len"
> which is exactly "PAGE_SIZE - offset".
> 
> We will fill in exactly one page buffer entry, however we will erroneously
> return 2 from this function.
> 
> I believe the fix is to first replace the test at the end of the loop
> with:
> 
> 	page++;
> 	offset = 0;
> 	j++;
> 
> And subsequently change the function to just return plain "j".