Page MenuHomeFreeBSD

intel: correct initialization of tx_cidx_processed
ClosedPublic

Authored by jacob.e.keller_intel.com on Dec 14 2018, 12:00 AM.

Details

Summary

In r341156 ("Fix first-packet completion", 2018-11-28) a hack to work
around a delta calculation determining how many descriptors were used
was added to ixl_isc_tx_credits_update_dwb.

The same fix was also applied to the em and igb drivers in r340310, and
to ix in r341156.

The hack checked the case where prev and cur were equal, and then added
one. This works, because by the time we do the delta check, we already
know there is at least one packet available, so the delta should be at
least one.

However, it's not a complete fix, and as indicated by the comment is
really a hack to work around the real bug.

The real problem is that the first time that we transmit a packet,
tx_cidx_processed will be set to point to the start of the ring.
Ultimately, the credits_update function expects it to point to the
*last* descriptor that was processed. Since we haven't yet processed any
descriptors, pointing it to 0 results in this incorrect calculation.

Fix the initialization code to have it point to the end of the ring
instead. One way to think about this, is that we are setting the value
to be one prior to the first available descriptor.

Doing so, corrects the delta calculation in all cases. The original fix
only works if the first packet has exactly one descriptor. Otherwise, we
will report 1 less than the correct value.

As part of this fix, also update the MPASS assertions to match the real
expectations. First, ensure that prev is not equal to cur, since this
should never happen. Second, remove the assertion about prev==0 || delta
!= 0. It looks like that originated from when the em driver was
converted to iflib. It seems like it was supposed to ensure that delta
was non-zero. However, because we originally returned 0 delta for the
first calculation, the "prev == 0" was tacked on.

Instead, replace this with a check that delta is greater than zero,
after the correction necessary when the ring pointers wrap around.

This new solution should fix the same bug as r341156 did, but in a more
robust way.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Stephen Hurd, this is what I believe to be the correct fix for this issue. I'm not entirely certain how to reproduce the original setup that triggered the bug, so some help with that would be appreciated.

I do think this is helpful, because it corrects the returned count even when the number of descriptors is more than 1, which the hack fix does not do. Additionally, it removes an MPASS which seemed incorrect to me, since the delta should never be zero.

shurd added a comment.Dec 14 2018, 8:36 PM

I'm not entirely certain how to reproduce the original setup that triggered the bug, so some help with that would be appreciated.

The easiest way to trigger the issue is to use net/pkt-gen to send a fixed number of packets. With the bug, pkt-gen would never complete because it wasn't notified that all packets were sent.

I'm not entirely certain how to reproduce the original setup that triggered the bug, so some help with that would be appreciated.

The easiest way to trigger the issue is to use net/pkt-gen to send a fixed number of packets. With the bug, pkt-gen would never complete because it wasn't notified that all packets were sent.

Ok, I'll give it a try.

The following pkt-gen command:

pkt-gen -i ixl0 -s <src ip> -d <dst ip> -n 100

works with this patch applied. Without either the original fix or this patch, the command fails.

This revision is now accepted and ready to land.Dec 18 2018, 4:30 AM
erj added a subscriber: erj.Jan 18 2019, 7:08 PM

Should I be the one to commit this, then?

erj added inline comments.Jan 24 2019, 12:41 AM
sys/dev/ixgbe/if_ixv.c
1231 ↗(On Diff #51981)

Missing a semicolon at the end of this line, but I can take care of it.

This revision was automatically updated to reflect the committed changes.