stupid gmail... sent again to the list, sorry for duplicate (but
formatted better this time)
Hey Alex, this patch appears to have caused a regression (probably both
i40e/i40evf). Easily reproducible running rds-stress (see the thread titled
"i40e card Tx resets", the middle post by sowmini has the repro steps,
ignore the pktgen discussion in the thread.)
I've spent some time trying to figure out the right fix, but keep getting
stuck in the complicated logic of the function, so I'm sending this in the
hope you'll take a look.
This is (one example of) the skb that doesn't get linearized:
skb_print_bits: > headlen=114 datalen=33824 data=238 mac=238 nh=252
h=272 gso=1448 frag=17
skb_print_bits: frag[0]: len: 256
skb_print_bits: frag[1]: len: 48
skb_print_bits: frag[2]: len: 256
skb_print_bits: frag[3]: len: 48
skb_print_bits: frag[4]: len: 256
skb_print_bits: frag[5]: len: 48
skb_print_bits: frag[6]: len: 4096
This descriptor ^^^ is the 8th, I believe the hardware mechanism faults on
this input.
I added a print of the sum at each point it is subtracted or added to after
initially being set, sum7/8 are in the for loop.
skb_print_bits: frag[7]: len: 4096
skb_print_bits: frag[8]: len: 48
skb_print_bits: frag[9]: len: 4096
skb_print_bits: frag[10]: len: 4096
skb_print_bits: frag[11]: len: 48
skb_print_bits: frag[12]: len: 4096
skb_print_bits: frag[13]: len: 4096
skb_print_bits: frag[14]: len: 48
skb_print_bits: frag[15]: len: 4096
skb_print_bits: frag[16]: len: 4096
__i40e_chk_linearize: sum1: -1399
__i40e_chk_linearize: sum2: -1143
__i40e_chk_linearize: sum3: -1095
__i40e_chk_linearize: sum4: -839
__i40e_chk_linearize: sum5: -791
__i40e_chk_linearize: sum7: 3305
__i40e_chk_linearize: sum8: 3257
__i40e_chk_linearize: sum7: 7353
__i40e_chk_linearize: sum8: 7097
__i40e_chk_linearize: sum7: 7145
__i40e_chk_linearize: sum8: 7097
__i40e_chk_linearize: sum7: 11193
__i40e_chk_linearize: sum8: 10937
__i40e_chk_linearize: sum7: 15033
__i40e_chk_linearize: sum8: 14985
__i40e_chk_linearize: sum7: 15033
This was the descriptors generated from the above:
d[054] = 0x0000000000000000 0x16a0211400000011
d[055] = 0x0000000bfbaa40ee 0x000001ca02871640
d[056] = 0x0000000584bcd000 0x0000040202871640
d[057] = 0x0000000c0bfea9d0 0x000000c202871640
d[058] = 0x0000000584bcd100 0x0000040202871640
d[059] = 0x0000000c0bfeaa00 0x000000c202871640
d[05a] = 0x0000000584bcd200 0x0000040202871640
d[05b] = 0x0000000c0bfeaa30 0x000000c202871640
d[05c] = 0x000000056d5f0000 0x0000400202871640
d[05d] = 0x000000056d5f1000 0x0000400202871640
d[05e] = 0x0000000c0bfeaa60 0x000000c202871640
d[05f] = 0x00000005f2762000 0x0000400202871640
d[060] = 0x00000005f765e000 0x0000400202871640
d[061] = 0x0000000c0bfeaa90 0x000000c202871640
d[062] = 0x0000000574928000 0x0000400202871640
d[063] = 0x0000000568ba5000 0x0000400202871640
d[064] = 0x0000000c0bfeaac0 0x000000c202871640
d[065] = 0x00000005f68cd000 0x0000400202871640
d[066] = 0x0000000585a2a000 0x0000400202871670
On Fri, Feb 19, 2016 at 3:54 AM Jeff Kirsher [off-list ref]
wrote:
From: Alexander Duyck <redacted>
This patch is meant to rewrite the logic for how we determine if we can
transmit the frame or if it needs to be linearized.
+ /* Initialize size to the negative value of gso_size minus 1. We
+ * use this as the worst case scenerio in which the frag ahead
+ * of us only provides one byte which is why we are limited to 6
+ * descriptors for a single transmit as the header and previous
+ * fragment are already consuming 2 descriptors.
+ */
+ sum = 1 - gso_size;
+
+ /* Add size of frags 1 through 5 to create our initial sum */
+ sum += skb_frag_size(++frag);
I'm pretty sure this code skips frag[0] due to the pre-increment, the bug
seems to occur in the algorithm because skb->data contains L2/L3/L4 header
plus some data, and should counts as 1 descriptor for every subsequent sent
packet, and if the incoming skb has 7 chunks that add up to < MSS then we
get in trouble.
+ sum += skb_frag_size(++frag);
+ sum += skb_frag_size(++frag);
+ sum += skb_frag_size(++frag);
+ sum += skb_frag_size(++frag);
+
+ /* Walk through fragments adding latest fragment, testing it, and
+ * then removing stale fragments from the sum.
+ */
+ stale = &skb_shinfo(skb)->frags[0];
+ for (;;) {
+ sum += skb_frag_size(++frag);
+
+ /* if sum is negative we failed to make sufficient progress */
+ if (sum < 0)
+ return true;
+
+ /* use pre-decrement to avoid processing last fragment */
+ if (!--nr_frags)
+ break;
+
+ sum -= skb_frag_size(++stale);
I think this line also skips stale[0]
On Tue, Mar 29, 2016 at 5:34 PM, Jesse Brandeburg
[off-list ref] wrote:
stupid gmail... sent again to the list, sorry for duplicate (but
formatted better this time)
Hey Alex, this patch appears to have caused a regression (probably both
i40e/i40evf). Easily reproducible running rds-stress (see the thread titled
"i40e card Tx resets", the middle post by sowmini has the repro steps,
ignore the pktgen discussion in the thread.)
I'll see about setting up rds tomorrow. It should be pretty straight forward.
I've spent some time trying to figure out the right fix, but keep getting
stuck in the complicated logic of the function, so I'm sending this in the
hope you'll take a look.
This is (one example of) the skb that doesn't get linearized:
skb_print_bits: > headlen=114 datalen=33824 data=238 mac=238 nh=252
h=272 gso=1448 frag=17
So the code as I had written it up should be verifying we use no more
than 8 descriptors as that was what the data sheet states. You might
want to request a spec update if the hardware only supports 7 data
descriptors per segment for a TSO as that isn't what is in the
documentation.
skb_print_bits: frag[0]: len: 256
skb_print_bits: frag[1]: len: 48
skb_print_bits: frag[2]: len: 256
skb_print_bits: frag[3]: len: 48
skb_print_bits: frag[4]: len: 256
skb_print_bits: frag[5]: len: 48
skb_print_bits: frag[6]: len: 4096
This descriptor ^^^ is the 8th, I believe the hardware mechanism faults on
this input.
Is this something that was previously being linearized? It doesn't
seem like it would be. We are only 8 descriptors in and that 7th
fragment should have flushed the count back to 1 with a remainder.
If you are wanting to trigger a linearize with the updated code you
would just need to update 2 lines. Replace "I40E_MAX_BUFFER_TXD - 1"
with just I40E_MAX_BUFFER_TXD" in the line where we subtract that
value from nr_frags. Then remove one of the "sum +=
skb_frag_size(++frag);" lines. That should update the code so that
you only evaluate 5 buffers at a time instead of 6. It should force
things to coalesce if we would span 8 or more descriptors instead of 9
or more which is what the code currently does based on what was
required in the EAS.
I added a print of the sum at each point it is subtracted or added to after
initially being set, sum7/8 are in the for loop.
skb_print_bits: frag[7]: len: 4096
skb_print_bits: frag[8]: len: 48
skb_print_bits: frag[9]: len: 4096
skb_print_bits: frag[10]: len: 4096
skb_print_bits: frag[11]: len: 48
skb_print_bits: frag[12]: len: 4096
skb_print_bits: frag[13]: len: 4096
skb_print_bits: frag[14]: len: 48
skb_print_bits: frag[15]: len: 4096
skb_print_bits: frag[16]: len: 4096
__i40e_chk_linearize: sum1: -1399
__i40e_chk_linearize: sum2: -1143
__i40e_chk_linearize: sum3: -1095
__i40e_chk_linearize: sum4: -839
__i40e_chk_linearize: sum5: -791
__i40e_chk_linearize: sum7: 3305
__i40e_chk_linearize: sum8: 3257
__i40e_chk_linearize: sum7: 7353
__i40e_chk_linearize: sum8: 7097
__i40e_chk_linearize: sum7: 7145
__i40e_chk_linearize: sum8: 7097
__i40e_chk_linearize: sum7: 11193
__i40e_chk_linearize: sum8: 10937
__i40e_chk_linearize: sum7: 15033
__i40e_chk_linearize: sum8: 14985
__i40e_chk_linearize: sum7: 15033
This was the descriptors generated from the above:
d[054] = 0x0000000000000000 0x16a0211400000011
d[055] = 0x0000000bfbaa40ee 0x000001ca02871640
len 72
d[056] = 0x0000000584bcd000 0x0000040202871640
len 256
d[057] = 0x0000000c0bfea9d0 0x000000c202871640
len 48
d[058] = 0x0000000584bcd100 0x0000040202871640
len 256
d[059] = 0x0000000c0bfeaa00 0x000000c202871640
len 48
d[05a] = 0x0000000584bcd200 0x0000040202871640
len 256
d[05b] = 0x0000000c0bfeaa30 0x000000c202871640
len 48
d[05c] = 0x000000056d5f0000 0x0000400202871640
len 4096
From what I can see we are at 8 descriptors and have exceeded 1 MSS.
According to the EAS this is supposed to work.
d[05d] = 0x000000056d5f1000 0x0000400202871640
d[05e] = 0x0000000c0bfeaa60 0x000000c202871640
d[05f] = 0x00000005f2762000 0x0000400202871640
d[060] = 0x00000005f765e000 0x0000400202871640
d[061] = 0x0000000c0bfeaa90 0x000000c202871640
d[062] = 0x0000000574928000 0x0000400202871640
d[063] = 0x0000000568ba5000 0x0000400202871640
d[064] = 0x0000000c0bfeaac0 0x000000c202871640
d[065] = 0x00000005f68cd000 0x0000400202871640
d[066] = 0x0000000585a2a000 0x0000400202871670
On Fri, Feb 19, 2016 at 3:54 AM Jeff Kirsher [off-list ref]
wrote:
quoted
From: Alexander Duyck <redacted>
This patch is meant to rewrite the logic for how we determine if we can
transmit the frame or if it needs to be linearized.
+ /* Initialize size to the negative value of gso_size minus 1. We
+ * use this as the worst case scenerio in which the frag ahead
+ * of us only provides one byte which is why we are limited to 6
+ * descriptors for a single transmit as the header and previous
+ * fragment are already consuming 2 descriptors.
+ */
+ sum = 1 - gso_size;
+
+ /* Add size of frags 1 through 5 to create our initial sum */
+ sum += skb_frag_size(++frag);
I'm pretty sure this code skips frag[0] due to the pre-increment, the bug
seems to occur in the algorithm because skb->data contains L2/L3/L4 header
plus some data, and should counts as 1 descriptor for every subsequent sent
packet, and if the incoming skb has 7 chunks that add up to < MSS then we
get in trouble.
Yep we were skipping frag 0 on purpose. There is a comment earlier
that basically states that frag 0 cannot borrow from an earlier
descriptor. The assumption is that if we have a frag it contains at
least 1 byte. So if the 6 frags following the first one do not
provide at least gso_size - 1 in data the packet needs to be
linearized.
The code as written was meant to cap us at 8 data descriptors per
packet. It sounds like we are wanting to reduce that to 7 if I
understand what you are describing correctly.
quoted
+ sum += skb_frag_size(++frag);
+ sum += skb_frag_size(++frag);
+ sum += skb_frag_size(++frag);
+ sum += skb_frag_size(++frag);
+
+ /* Walk through fragments adding latest fragment, testing it, and
+ * then removing stale fragments from the sum.
+ */
+ stale = &skb_shinfo(skb)->frags[0];
+ for (;;) {
+ sum += skb_frag_size(++frag);
+
+ /* if sum is negative we failed to make sufficient progress */
+ if (sum < 0)
+ return true;
+
+ /* use pre-decrement to avoid processing last fragment */
+ if (!--nr_frags)
+ break;
+
+ sum -= skb_frag_size(++stale);
I think this line also skips stale[0]
Right we skip frag 0 and the last fragment in the packet. The general
idea was that we were preventing us from going over 8 descriptors per
packet so I was only checking 6 fragments starting at the second and
as long as we completed one MSS in those 6 descriptors we were
guaranteed to complete a packet in less than 8 total descriptors.
On Tue, Mar 29, 2016 at 7:20 PM, Alexander Duyck
[off-list ref] wrote:
On Tue, Mar 29, 2016 at 5:34 PM, Jesse Brandeburg
[off-list ref] wrote:
quoted
stupid gmail... sent again to the list, sorry for duplicate (but
formatted better this time)
Hey Alex, this patch appears to have caused a regression (probably both
i40e/i40evf). Easily reproducible running rds-stress (see the thread titled
"i40e card Tx resets", the middle post by sowmini has the repro steps,
ignore the pktgen discussion in the thread.)
I'll see about setting up rds tomorrow. It should be pretty straight forward.
quoted
I've spent some time trying to figure out the right fix, but keep getting
stuck in the complicated logic of the function, so I'm sending this in the
hope you'll take a look.
This is (one example of) the skb that doesn't get linearized:
skb_print_bits: > headlen=114 datalen=33824 data=238 mac=238 nh=252
h=272 gso=1448 frag=17
So the code as I had written it up should be verifying we use no more
than 8 descriptors as that was what the data sheet states. You might
want to request a spec update if the hardware only supports 7 data
descriptors per segment for a TSO as that isn't what is in the
documentation.
quoted
skb_print_bits: frag[0]: len: 256
skb_print_bits: frag[1]: len: 48
skb_print_bits: frag[2]: len: 256
skb_print_bits: frag[3]: len: 48
skb_print_bits: frag[4]: len: 256
skb_print_bits: frag[5]: len: 48
skb_print_bits: frag[6]: len: 4096
This descriptor ^^^ is the 8th, I believe the hardware mechanism faults on
this input.
Is this something that was previously being linearized? It doesn't
seem like it would be. We are only 8 descriptors in and that 7th
fragment should have flushed the count back to 1 with a remainder.
If you are wanting to trigger a linearize with the updated code you
would just need to update 2 lines. Replace "I40E_MAX_BUFFER_TXD - 1"
with just I40E_MAX_BUFFER_TXD" in the line where we subtract that
value from nr_frags. Then remove one of the "sum +=
skb_frag_size(++frag);" lines. That should update the code so that
you only evaluate 5 buffers at a time instead of 6. It should force
things to coalesce if we would span 8 or more descriptors instead of 9
or more which is what the code currently does based on what was
required in the EAS.
quoted
I added a print of the sum at each point it is subtracted or added to after
initially being set, sum7/8 are in the for loop.
skb_print_bits: frag[7]: len: 4096
skb_print_bits: frag[8]: len: 48
skb_print_bits: frag[9]: len: 4096
skb_print_bits: frag[10]: len: 4096
skb_print_bits: frag[11]: len: 48
skb_print_bits: frag[12]: len: 4096
skb_print_bits: frag[13]: len: 4096
skb_print_bits: frag[14]: len: 48
skb_print_bits: frag[15]: len: 4096
skb_print_bits: frag[16]: len: 4096
__i40e_chk_linearize: sum1: -1399
__i40e_chk_linearize: sum2: -1143
__i40e_chk_linearize: sum3: -1095
__i40e_chk_linearize: sum4: -839
__i40e_chk_linearize: sum5: -791
__i40e_chk_linearize: sum7: 3305
__i40e_chk_linearize: sum8: 3257
__i40e_chk_linearize: sum7: 7353
__i40e_chk_linearize: sum8: 7097
__i40e_chk_linearize: sum7: 7145
__i40e_chk_linearize: sum8: 7097
__i40e_chk_linearize: sum7: 11193
__i40e_chk_linearize: sum8: 10937
__i40e_chk_linearize: sum7: 15033
__i40e_chk_linearize: sum8: 14985
__i40e_chk_linearize: sum7: 15033
This was the descriptors generated from the above:
d[054] = 0x0000000000000000 0x16a0211400000011
d[055] = 0x0000000bfbaa40ee 0x000001ca02871640
len 72
Minor correction here. This is 72 hex, which is 114 in decimal. The
interesting bit I believe is the fact that we have both header and
payload data in the same descriptor.
You should probably check with your hardware team on this but I have a
working theory on the issue. I'm thinking that because both header
and payload are coming out of the same descriptor this must count as 2
descriptors and not 1 when we compute the usage for the first
descriptor. As such I do probably need to start testing at frag 0
because the first payload can actually come out of the header region
if the first descriptor includes both payload and data.
I'll try to submit a patch for it tonight. If you can test it
tomorrow I would appreciate it. Otherwise I will try setting up an
environment to try and reproduce the issue tomorrow.
- Alex
On Tue, 2016-03-29 at 22:19 -0700, Alexander Duyck wrote:
Minor correction here. This is 72 hex, which is 114 in decimal. The
interesting bit I believe is the fact that we have both header and
payload data in the same descriptor.
You should probably check with your hardware team on this but I have
a
working theory on the issue. I'm thinking that because both header
and payload are coming out of the same descriptor this must count as
2
descriptors and not 1 when we compute the usage for the first
descriptor. As such I do probably need to start testing at frag 0
because the first payload can actually come out of the header region
if the first descriptor includes both payload and data.
I'll try to submit a patch for it tonight. If you can test it
tomorrow I would appreciate it. Otherwise I will try setting up an
environment to try and reproduce the issue tomorrow.
If you get it submitted tonight, I can have it ready in my tree for
testing for Jesse and the team when they get into the office tomorrow.