Page MenuHomeFreeBSD
Differential D18470

ix(4),ixv(4): Fix TSO offloads when TXCSUM is disabled
ClosedPublic
Actions

Authored by piotr.pietruszewski_intel.com on Dec 7 2018, 1:28 PM.
Tags
None
Referenced Files
F81651660: D18470.id51901.diff
Fri, Apr 19, 11:54 AM
F81625509: D18470.id52216.diff
Fri, Apr 19, 4:33 AM
Unknown Object (File)
Thu, Apr 18, 4:22 PM
Unknown Object (File)
Thu, Apr 18, 7:59 AM
Unknown Object (File)
Tue, Apr 16, 3:03 PM
Unknown Object (File)
Sat, Apr 6, 3:57 PM
Unknown Object (File)
Jan 13 2024, 1:30 PM
Unknown Object (File)
Jan 8 2024, 2:29 AM
View All 37 Files
Subscribers
ae
erj
imp
jacob.e.keller_intel.com
jeffrey.e.pieper_intel.com

Details

Reviewers
sbruno
jeffrey.e.pieper_intel.com
erj
Group Reviewers
Intel Networking
Commits
rS344101: MFC: r343622
rS343622: ix(4),ixv(4): Fix TSO offloads when TXCSUM is disabled
Summary

The iflib stack does not disable TSO automatically when TXCSUM is
disabled, instead assuming that the driver will correctly handle TSOs
even when CSUM_IP is not set.

This results in iflib calling ixgbe_isc_txd_encap with packets which have
CSUM_IP_TSO, but do not have CSUM_IP or CSUM_IP_TCP set. Because of
this, ixgbe_tx_ctx_setup will not setup the IPv4 checksum offloading.

This results in bad TSO packets being sent if a user disables TXCSUM
without disabling TSO.

Fix this by updating the ixgbe_tx_ctx_setup function to check both
CSUM_IP and CSUM_IP_TSO when deciding whether to enable checksums.

Once this is corrected, another issue for TSO packets is revealed. The
driver sets IFLIB_NEED_ZERO_CSUM in order to enable a work around that
causes the ip->sum field to be zero'd. This is necessary for ix
hardware to correctly perform TSOs.

However, if TXCSUM is disabled, then the work around is not enabled, as
CSUM_IP will not be set when the iflib stack checks to see if it should
clear the sum field.

Fix this by adding IFLIB_TSO_INIT_IP to the iflib flags for the ix and
ixv interface files.

Once both of these changes are made, the ix and ixv drivers should
correctly offload TSO packets when TSO offload is enabled, regardless
of whether TXCSUM is enabled or disabled.

This patch and commit message is based on rS340256
created by Jacob Keller.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 21426
Build 20748: arc lint + arc unit

Event Timeline

piotr.pietruszewski_intel.com created this revision.Dec 7 2018, 1:28 PM
Herald added a reviewer: sbruno. · View Herald TranscriptDec 7 2018, 1:28 PM
Herald added a subscriber: ae. · View Herald Transcript
piotr.pietruszewski_intel.com added a reviewer: Intel Networking.Dec 7 2018, 1:29 PM
Harbormaster completed remote builds in B21426: Diff 51713.Dec 7 2018, 1:29 PM
sbruno accepted this revision.Dec 10 2018, 3:42 PM
sbruno added a subscriber: jeffrey.e.pieper_intel.com.

I guess this will wait for @jeffrey.e.pieper_intel.com to test or approve? This looks correct to my eye.

This revision is now accepted and ready to land.Dec 10 2018, 3:42 PM
krzysztof.galazka_intel.com added a reviewer: jeffrey.e.pieper_intel.com.Dec 11 2018, 2:01 PM
This revision now requires review to proceed.Dec 11 2018, 2:01 PM
erj added a subscriber: erj.Dec 12 2018, 1:35 AM

The description should mention that this also affects/fixes ixv(4), too.

piotr.pietruszewski_intel.com updated this revision to Diff 51901.Dec 12 2018, 11:28 AM

Change commit message to mention also ixv driver.

piotr.pietruszewski_intel.com retitled this revision from ix(4): Fix TSO offloads when TXCSUM is disabled to ix(4),ixv(4): Fix TSO offloads when TXCSUM is disabled.Dec 12 2018, 11:30 AM
piotr.pietruszewski_intel.com edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B21511: Diff 51901.Dec 12 2018, 11:30 AM
jeffrey.e.pieper_intel.com requested changes to this revision.EditedDec 18 2018, 6:17 PM

We are seeing tx hangs with UDP using multiple clients when offloads are disabled. Cannot reproduce on 12.0-RELEASE. Reproducible on X520 and X550.

This revision now requires changes to proceed.Dec 18 2018, 6:17 PM
sbruno added a comment.Dec 18 2018, 6:20 PM
In D18470#396626, @jeffrey.e.pieper_intel.com wrote:

We are seeing tx hangs with UDP using multiple clients when offloads are disabled. Cannot reproduce on 12.0-RELEASE. Reproducible on X520 and X550.

Huh? Does this review break things for you? Or are you bringing up a new problem?

jeffrey.e.pieper_intel.com added a comment.Dec 18 2018, 6:23 PM
In D18470#396629, @sbruno wrote:
In D18470#396626, @jeffrey.e.pieper_intel.com wrote:

We are seeing tx hangs with UDP using multiple clients when offloads are disabled. Cannot reproduce on 12.0-RELEASE. Reproducible on X520 and X550.

Huh? Does this review break things for you? Or are you bringing up a new problem?

Just reporting issues/regressions we found while testing this :)

sbruno added a comment.Dec 19 2018, 7:12 PM
In D18470#396630, @jeffrey.e.pieper_intel.com wrote:
In D18470#396629, @sbruno wrote:
In D18470#396626, @jeffrey.e.pieper_intel.com wrote:

We are seeing tx hangs with UDP using multiple clients when offloads are disabled. Cannot reproduce on 12.0-RELEASE. Reproducible on X520 and X550.

Huh? Does this review break things for you? Or are you bringing up a new problem?

Just reporting issues/regressions we found while testing this :)

So, is UDP broken without this review?

jacob.e.keller_intel.com added a subscriber: jacob.e.keller_intel.com.Dec 19 2018, 7:40 PM

We are seeing tx hangs with UDP using multiple clients when offloads are disabled. Cannot reproduce on 12.0-RELEASE. Reproducible on X520 and X550.

This sounds like it can't reproduce on 12-release, meaning that it's a bug with this patch applied? Is that a correct interpretation of the statement?

Clarification would help here.

jeffrey.e.pieper_intel.com added a comment.Dec 19 2018, 8:24 PM
In D18470#396858, @jacob.e.keller_intel.com wrote:

We are seeing tx hangs with UDP using multiple clients when offloads are disabled. Cannot reproduce on 12.0-RELEASE. Reproducible on X520 and X550.

This sounds like it can't reproduce on 12-release, meaning that it's a bug with this patch applied? Is that a correct interpretation of the statement?

Clarification would help here.

That would be correct. We cannot repro on 13-CURRENT or 12.0-RELEASE, so this patch somehow breaks UDP with offloads disabled.

jacob.e.keller_intel.com added a comment.Dec 19 2018, 8:30 PM

I think I see what's wrong... we check CSUM_TSO for IPPROTO_UDP and IPPROTO_SCTP. That doesn't make sense, since TSO is only for TCP

jacob.e.keller_intel.com added a comment.Dec 19 2018, 8:33 PM

We probably should only check CSUM_TSO for TCP packets..

sys/dev/ixgbe/ix_txrx.c
123

In fact, we already do a check for enabling IP checksums when CSUM_TSO is on, but CSUM_IP is off, here.

134

This line is correct, but...

140

I don't think we want to enable UDP checksums if CSUM_TSO is on, but UDP checksums are off?

146

Same here, I think the check for CSUM_TSO here is incorrect.

piotr.pietruszewski_intel.com updated this revision to Diff 52216.Dec 21 2018, 11:42 AM

Implementing changes mentioned by Jake.

Harbormaster completed remote builds in B21674: Diff 52216.Dec 21 2018, 11:42 AM
erj added a comment.Jan 29 2019, 5:47 PM

@jeffrey.e.pieper_intel.com you should re-test this, now that Piotr has updated the patch.

jeffrey.e.pieper_intel.com accepted this revision.Jan 31 2019, 4:20 PM
This revision is now accepted and ready to land.Jan 31 2019, 4:20 PM
erj accepted this revision.Jan 31 2019, 5:42 PM
Closed by commit rS343622: ix(4),ixv(4): Fix TSO offloads when TXCSUM is disabled (authored by erj). · Explain WhyJan 31 2019, 9:53 PM
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: imp. · View Herald TranscriptJan 31 2019, 9:53 PM

Revision Contents
Changeset List

PathSize
sys/
dev/
ixgbe/
1 line
1 line
6 lines

Diff 51713

View Options

sys/dev/ixgbe/if_ix.c

Loading...
View Options

sys/dev/ixgbe/if_ixv.c

Loading...
View Options

sys/dev/ixgbe/ix_txrx.c

Loading...