From mboxrd@z Thu Jan 1 00:00:00 1970 From: Annie Subject: Re: [PATCH v2 1/1] xen/netback: correctly calculate required slots of skb. Date: Thu, 11 Jul 2013 18:59:26 +0800 Message-ID: <3038108B-5529-452A-A488-4E616F7C1B8E@oracle.com> References: <1373447711-31303-1-git-send-email-annie.li@oracle.com> <1373530274.5453.148.camel@hastur.hellion.org.uk> <51DE6DFD.9080007@oracle.com> <1373536078.5453.159.camel@hastur.hellion.org.uk> <4A59CC63-9B57-49FA-A2FD-6903C86E110E@oracle.com> Mime-Version: 1.0 (1.0) Content-Type: multipart/mixed; boundary="===============3882206869844808958==" Cc: "netdev@vger.kernel.org" , "xen-devel@lists.xensource.com" , "wei.liu2@citrix.com" , Ian Campbell , "msw@amazon.com" To: Annie Return-path: In-Reply-To: <4A59CC63-9B57-49FA-A2FD-6903C86E110E@oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org List-Id: netdev.vger.kernel.org --===============3882206869844808958== Content-Transfer-Encoding: 7bit Content-Type: multipart/alternative; boundary=Apple-Mail-C91A0154-5A96-43B0-B5CF-3F727826BE99 --Apple-Mail-C91A0154-5A96-43B0-B5CF-3F727826BE99 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=GB2312 =B7=A2=D7=D4=CE=D2=B5=C4 iPhone =D4=DA 2013-7-11=A3=AC=CF=C2=CE=E76:46=A3=ACAnnie =D0=B4= =B5=C0=A3=BA >=20 >=20 > =B7=A2=D7=D4=CE=D2=B5=C4 iPhone >=20 > =D4=DA 2013-7-11=A3=AC=CF=C2=CE=E75:47=A3=ACIan Campbell =D0=B4=B5=C0=A3=BA >=20 >> On Thu, 2013-07-11 at 16:34 +0800, annie li wrote: >>> On 2013-7-11 16:11, Ian Campbell wrote: >>>> On Wed, 2013-07-10 at 17:15 +0800, Annie Li wrote: >>>>> +static int netbk_count_slots(struct xenvif *vif, struct sk_buff *skb,= >>>>> + int *copy_off, unsigned long size, >>>>> + unsigned long offset, int *head) >>>>> { >>>>> - unsigned int count; >>>>> - int i, copy_off; >>>>> + unsigned long bytes; >>>>> + int count =3D 0; >>>>>=20 >>>>> - count =3D DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE); >>>>> + offset &=3D ~PAGE_MASK; >>>>>=20 >>>>> - copy_off =3D skb_headlen(skb) % PAGE_SIZE; >>>>> + while (size > 0) { >>>>> + BUG_ON(offset >=3D PAGE_SIZE); >>>>> + BUG_ON(*copy_off > MAX_BUFFER_OFFSET); >>>>>=20 >>>>> - if (skb_shinfo(skb)->gso_size) >>>>> - count++; >>>>> + bytes =3D PAGE_SIZE - offset; >>>>>=20 >>>>> - for (i =3D 0; i < skb_shinfo(skb)->nr_frags; i++) { >>>>> - unsigned long size =3D skb_frag_size(&skb_shinfo(skb)->frags[= i]); >>>>> - unsigned long offset =3D skb_shinfo(skb)->frags[i].page_offse= t; >>>>> - unsigned long bytes; >>>>> + if (bytes > size) >>>>> + bytes =3D size; >>>>>=20 >>>>> - offset &=3D ~PAGE_MASK; >>>>> + if (start_new_rx_buffer(*copy_off, bytes, *head)) { >>>>> + count++; >>>>> + *copy_off =3D 0; >>>>> + } >>>>>=20 >>>>> - while (size > 0) { >>>>> - BUG_ON(offset >=3D PAGE_SIZE); >>>>> - BUG_ON(copy_off > MAX_BUFFER_OFFSET); >>>>> + if (*copy_off + bytes > MAX_BUFFER_OFFSET) >>>>> + bytes =3D MAX_BUFFER_OFFSET - *copy_off; >>>>>=20 >>>>> - bytes =3D PAGE_SIZE - offset; >>>>> + *copy_off +=3D bytes; >>>>>=20 >>>>> - if (bytes > size) >>>>> - bytes =3D size; >>>>> + offset +=3D bytes; >>>>> + size -=3D bytes; >>>>>=20 >>>>> - if (start_new_rx_buffer(copy_off, bytes, 0)) { >>>>> - count++; >>>>> - copy_off =3D 0; >>>>> - } >>>>> + /* Next frame */ >>>>> + if (offset =3D=3D PAGE_SIZE && size) >>>>> + offset =3D 0; >>>>> + >>>>> + if (*head) >>>>> + count++; >>>> This little bit corresponds to the "/* Leave a gap for the GSO >>>> descriptor. */" in gop_frag_copy? >>>=20 >>> No, it does not correspond to this in gop_frag_copy. >>=20 >> So what does it correspond to? >=20 > It corresponds to following code in gop_frag_copy, > req =3D RING_GET_REQUEST(&vif->rx, vif->rx.req_cons++); Hit the button and Send it wrongly last time, it is in netbk_gop_skb. >=20 >>=20 >>> The code here only=20 >>> increase count for the first time. I thought to initialize the count in=20= >>> xen_netbk_count_skb_slots with 1 to avoid this. But thinking of the=20 >>> extreme case when the header size is zero(not sure whether this case=20 >>> could be true), I increase the count here to keep safe in case header=20= >>> size is zero. >>=20 >> netfront requires that the first slot always contains some data, >> gop_frag_copy will BUG if that's not the case. In gop_frag_copy, we can not go into the while if the size is 0. Which BUG_O= N do you mean here? Thanks Annie >>=20 >>>=20 >>> There is code correspond to that in gop_frag_copy in=20 >>> xen_netbk_count_skb_slots, see following, >>>=20 >>> + if (skb_shinfo(skb)->gso_size && !vif->gso_prefix) >>> + count++; >>> (The original code does not have gso_prefix, I added it in this patch >>> too based on Wei's suggestion) >>=20 >> OK. >>=20 >> It's be nice if the count and copy variants of this logic could be as >> similar as possible but they already differ quite a bit. I have >> considered whether we could combine the two by adding "dry-run" >> functionality to gop_frag_copy but that seems like it would just ugly up >> both versions. >>=20 >> Ian. >>=20 >>=20 >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xen.org >> http://lists.xen.org/xen-devel >=20 > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel --Apple-Mail-C91A0154-5A96-43B0-B5CF-3F727826BE99 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8


=E5=8F=91=E8=87=AA= =E6=88=91=E7=9A=84 iPhone

=E5=9C=A8 2013-7-11=EF=BC=8C=E4=B8=8B= =E5=8D=886:46=EF=BC=8CAnnie <annie= .li@oracle.com> =E5=86=99=E9=81=93=EF=BC=9A

<= /span>


=E5=8F=91=E8=87= =AA=E6=88=91=E7=9A=84 iPhone

=E5=9C=A8 2013-7-11=EF=BC=8C=E4=B8= =8B=E5=8D=885:47=EF=BC=8CIan Campbell <ian.campbell@citrix.com> =E5=86=99=E9=81=93=EF=BC=9A

=
On Thu, 2013-07-11 at 1= 6:34 +0800, annie li wrote:
On 201= 3-7-11 16:11, Ian Campbell wrote:
On Wed, 2013-07-10 at 17:15 +0800, An= nie Li wrote:
=
+static int netbk_= count_slots(struct xenvif *vif, struct sk_buff *skb,
=
+            = ;    int *copy_off, unsigned long size,
+           &nb= sp;    unsigned long offset, int *head)
 {
-    unsigned int count;
-    int i, copy_off;
+    unsign= ed long bytes;
+ &nb= sp;  int count =3D 0;
=

-   &n= bsp;count =3D DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
+    offset &=3D ~PAGE_M= ASK;

-    copy_off =3D skb_= headlen(skb) % PAGE_SIZE;
<= blockquote type=3D"cite">
+    while (size > 0) {
+        BUG_ON(offset >=3D P= AGE_SIZE);
+  =      BUG_ON(*copy_off > MAX_BUFFER_OFFSET);

-    if (skb_shinfo(skb)->gso_size)
-     &nbs= p;  count++;
+=        bytes =3D PAGE_SIZE - offset;

=
-    for (i =3D 0; i < skb_shinfo(skb)->= ;nr_frags; i++) {
-=        unsigned long size =3D skb_frag_size(&skb_sh= info(skb)->frags[i]);
= -        unsigned long offset =3D skb_shinfo(skb)-= >frags[i].page_offset;
<= blockquote type=3D"cite">
-        unsigned long bytes;
+        if (bytes= > size)
+  = ;          bytes =3D size;
<= /blockquote>

-        offset &=3D ~PAGE_MASK;=
+       &n= bsp;if (start_new_rx_buffer(*copy_off, bytes, *head)) {
+           &n= bsp;count++;
+ &nbs= p;          *copy_off =3D 0;
+        }
=

-        while (size > 0= ) {
-    =        BUG_ON(offset >=3D PAGE_SIZE);
-         &nb= sp;  BUG_ON(copy_off > MAX_BUFFER_OFFSET);
=
+        if (*copy_off += bytes > MAX_BUFFER_OFFSET)
+            bytes =3D MAX_BUFFER_= OFFSET - *copy_off;

-     &= nbsp;      bytes =3D PAGE_SIZE - offset;
+        *copy_off += =3D bytes;
<= br>
-       &nb= sp;    if (bytes > size)
-                = bytes =3D size;
+ &= nbsp;      offset +=3D bytes;
+        size -=3D bytes;

-           &nbs= p;if (start_new_rx_buffer(copy_off, bytes, 0)) {
-             &n= bsp;  count++;
-                copy_off =3D 0;
-      = ;      }
<= span>+        /* Next frame */
=
+        if (offset =3D=3D= PAGE_SIZE && size)
+            offset =3D 0;
=
+
+        if (*head)=
+       &n= bsp;    count++;
=
This little bit co= rresponds to the "/* Leave a gap for the GSO
descriptor. *= /" in gop_frag_copy?

No, it d= oes not correspond to this in gop_frag_copy.

So what does it correspond to?
<= div>
It corresponds to following code in gop_frag_copy,
req =3D RING_GET_REQUEST(&vif->rx, vif->rx.req_cons++);

Hit the button and Send it wrong= ly last time, it is in netbk_gop_skb.



The code here only
increase count for the first time. I thought to ini= tialize the count in
xen_netbk_count_skb_slots with 1 to avoid this. But thinking of the =
extreme case when the heade= r size is zero(not sure whether this case
could be true), I increase the count here to keep safe= in case header
size= is zero.

netfront requires th= at the first slot always contains some data,
gop_frag_copy w= ill BUG if that's not the case.

In gop_frag_copy, we can not go into the while if t= he size is 0. Which BUG_ON do you mean here?

Thanks=
Annie


There is code correspond to that in g= op_frag_copy in
xen_= netbk_count_skb_slots, see following,

+ &= nbsp;  if (skb_shinfo(skb)->gso_size && !vif->gso_prefix)=
+     &nbs= p;  count++;
(Th= e original code does not have gso_prefix, I added it in this patch
too based on Wei's suggestion)=

OK.
It's be nice if the count and copy variants of this logic could be a= s
similar as possible but they already differ quite a bit. I= have
considered whether we could combine the two by adding "= dry-run"
functionality to gop_frag_copy but that seems like i= t would just ugly up
both versions.
<= br>Ian.


____________= ___________________________________
Xen-devel mailing list
Xen-devel@lists.xe= n.org
http://= lists.xen.org/xen-devel
_________= ______________________________________
Xen-devel mailing lis= t
Xen-devel@lists= .xen.org
http= ://lists.xen.org/xen-devel
= --Apple-Mail-C91A0154-5A96-43B0-B5CF-3F727826BE99-- --===============3882206869844808958== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============3882206869844808958==--