lkml.org 
[lkml]   [2014]   [Mar]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH V2 1/6] Drivers: net: hyperv: Enable scatter gather I/O
Date


> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Saturday, March 8, 2014 12:07 PM
> 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: KY Srinivasan <kys@microsoft.com>
> Date: Sat, 8 Mar 2014 04:12:01 +0000
>
> >
> >
> >> -----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.
>
> 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



\
 
 \ /
  Last update: 2014-03-08 12:01    [W:0.087 / U:0.076 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site