Re: [PATCH V2 1/6] Drivers: net: hyperv: Enable scatter gather I/O
From: KY Srinivasan Date: Sat, 8 Mar 2014 10:27:54 + > "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. You are correct, this is the part of the logic in this case which I missed. Thanks. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH V2 1/6] Drivers: net: hyperv: Enable scatter gather I/O
> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> Sent: Saturday, March 8, 2014 12:07 PM
> To: KY Srinivasan
> Cc: net...@vger.kernel.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com;
> jasow...@redhat.com
> Subject: Re: [PATCH V2 1/6] Drivers: net: hyperv: Enable scatter gather I/O
>
> From: KY Srinivasan
> Date: Sat, 8 Mar 2014 04:12:01 +
>
> >
> >
> >> -Original Message-
> >> From: David Miller [mailto:da...@davemloft.net]
> >> Sent: Saturday, March 8, 2014 3:18 AM
> >> To: KY Srinivasan
> >> Cc: net...@vger.kernel.org; linux-ker...@vger.kernel.org;
> >> de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com;
> >> jasow...@redhat.com
> >> Subject: Re: [PATCH V2 1/6] Drivers: net: hyperv: Enable scatter
> >> gather I/O
> >>
> >> From: "K. Y. Srinivasan"
> >> 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V2 1/6] Drivers: net: hyperv: Enable scatter gather I/O
From: KY Srinivasan
Date: Sat, 8 Mar 2014 04:12:01 +
>
>
>> -Original Message-
>> From: David Miller [mailto:da...@davemloft.net]
>> Sent: Saturday, March 8, 2014 3:18 AM
>> To: KY Srinivasan
>> Cc: net...@vger.kernel.org; linux-ker...@vger.kernel.org;
>> de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com;
>> jasow...@redhat.com
>> Subject: Re: [PATCH V2 1/6] Drivers: net: hyperv: Enable scatter gather I/O
>>
>> From: "K. Y. Srinivasan"
>> 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:
>> > + if (offset == PAGE_SIZE && len) {
>> > + page++;
>> > + offset = 0;
>> > + j++;
>> > + }
thus incrementing j before the return statement.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH V2 1/6] Drivers: net: hyperv: Enable scatter gather I/O
> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> Sent: Saturday, March 8, 2014 3:18 AM
> To: KY Srinivasan
> Cc: net...@vger.kernel.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com;
> jasow...@redhat.com
> Subject: Re: [PATCH V2 1/6] Drivers: net: hyperv: Enable scatter gather I/O
>
> From: "K. Y. Srinivasan"
> 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".
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V2 1/6] Drivers: net: hyperv: Enable scatter gather I/O
From: "K. Y. Srinivasan"
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.
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".
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
