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

[KY Srinivasan]

> -----Original Message-----
> From: David Miller [mailto:davem@xxxxxxxxxxxxx]
> Sent: Saturday, March 8, 2014 12:07 PM
> To: KY Srinivasan
> Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; apw@xxxxxxxxxxxxx;
> jasowang@xxxxxxxxxx
> Subject: Re: [PATCH V2 1/6] Drivers: net: hyperv: Enable scatter gather I/O
> 
> From: KY Srinivasan <kys@xxxxxxxxxxxxx>
> Date: Sat, 8 Mar 2014 04:12:01 +0000
> 
> >
> >
> >> -----Original Message-----
> >> From: David Miller [mailto:davem@xxxxxxxxxxxxx]
> >> Sent: Saturday, March 8, 2014 3:18 AM
> >> To: KY Srinivasan
> >> Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> >> devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; apw@xxxxxxxxxxxxx;
> >> jasowang@xxxxxxxxxx
> >> Subject: Re: [PATCH V2 1/6] Drivers: net: hyperv: Enable scatter
> >> gather I/O
> >>
> >> From: "K. Y. Srinivasan" <kys@xxxxxxxxxxxxx>
> >> 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.
> 
> That's not what happens, you execute the loop code at least once, and
> hit:
> 
> >> > +		offset += bytes;
> >> > +		len -= bytes;
> >> > +		if (offset == PAGE_SIZE && len) {
> >> > +			page++;
> >> > +			offset = 0;
> >> > +			j++;
> >> > +		}

> 
> thus incrementing j before the return statement.

I don't mean to be argumentative, but I still don't see the issue here. You are right, I
will execute the loop code at least once, but j gets incremented only if len is non-zero as well as
offset being == PAGE_SIZE. "len" would have gotten decremented prior to the check and in the case
we are talking about, "len" would be zero and so j would not get incremented.

Regards,

K. Y

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/