[net 1/2] ixgbe: Fix issues with SR-IOV loopback when flow control is disabled

From: Alexander Duyck <redacted>

This patch allows us to avoid a Tx hang when SR-IOV is enabled.  This hang
can be triggered by sending small packets at a rate that was triggering Rx
missed errors from the adapter while the internal Tx switch and at least
one VF are enabled.

This was all due to the fact that under heavy stress the Rx FIFO never
drained below the flow control high water mark.  This resulted in the Tx
FIFO being head of line blocked due to the fact that it relies on the flow
control high water mark to determine when it is acceptable for the Tx to
place a packet in the Rx FIFO.

The resolution for this is to set the FCRTH value to the RXPBSIZE - 32 so
that even if the ring is almost completely full we can still place Tx
packets on the Rx ring and drop incoming Rx traffic if we do not have
sufficient space available in the Rx FIFO.

Signed-off-by: Alexander Duyck <redacted>
Trested-by: Sibai Li [off-list ref]
Signed-off-by: Jeff Kirsher <redacted>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_common.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
index 383b941..7d81bee 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c

@@ -2011,13 +2011,20 @@ s32 ixgbe_fc_enable_generic(struct ixgbe_hw *hw, s32 packetbuf_num)
 	IXGBE_WRITE_REG(hw, IXGBE_MFLCN, mflcn_reg);
 	IXGBE_WRITE_REG(hw, IXGBE_FCCFG, fccfg_reg);
 
-	fcrth = hw->fc.high_water[packetbuf_num] << 10;
 	fcrtl = hw->fc.low_water << 10;
 
 	if (hw->fc.current_mode & ixgbe_fc_tx_pause) {
+		fcrth = hw->fc.high_water[packetbuf_num] << 10;
 		fcrth |= IXGBE_FCRTH_FCEN;
 		if (hw->fc.send_xon)
 			fcrtl |= IXGBE_FCRTL_XONE;
+	} else {
+		/*
+		 * If Tx flow control is disabled, set our high water mark
+		 * to Rx FIFO size minus 32 in order prevent Tx switch
+		 * loopback from stalling on DMA.
+		 */
+		fcrth = IXGBE_READ_REG(hw, IXGBE_RXPBSIZE(packetbuf_num)) - 32;
 	}
 
 	IXGBE_WRITE_REG(hw, IXGBE_FCRTH_82599(packetbuf_num), fcrth);

-- 
1.7.7.6

From: Yi Zou <redacted>

Fix a bug when using 'ethtool -K ethx tx off' to turn off tx ip checksum,
FCoE CRC offload should not be impacte. The skb_checksum_help() is needed
only if it's not FCoE traffic for ip checksum, regardless of ethtool toggling
the tx ip checksum on or off.

Signed-off-by: Yi Zou <redacted>
Tested-by: Ross Brattain <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
 net/core/dev.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 6ca32f6..9e378009 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -1913,6 +1913,9 @@ int skb_checksum_help(struct sk_buff *skb)
 	__wsum csum;
 	int ret = 0, offset;
 
+	if (skb->protocol == htons(ETH_P_FCOE))
+		goto out;
+
 	if (skb->ip_summed == CHECKSUM_COMPLETE)
 		goto out_set_summed;
 

-- 
1.7.7.6

On Wed, 14 Mar 2012 00:01:58 -0700
Jeff Kirsher [off-list ref] wrote:

Shouldn't this be done in the offending driver rather than the core?

On Wed, 14 Mar 2012 00:01:58 -0700
Jeff Kirsher [off-list ref] wrote:

quoted

From: Yi Zou <redacted>

Fix a bug when using 'ethtool -K ethx tx off' to turn off tx ip

checksum,

quoted

FCoE CRC offload should not be impacte. The skb_checksum_help() is

needed

quoted

only if it's not FCoE traffic for ip checksum, regardless of ethtool

toggling

quoted

the tx ip checksum on or off.

Signed-off-by: Yi Zou <redacted>
Tested-by: Ross Brattain <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
 net/core/dev.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 6ca32f6..9e378009 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -1913,6 +1913,9 @@ int skb_checksum_help(struct sk_buff *skb)
 	__wsum csum;
 	int ret = 0, offset;

+	if (skb->protocol == htons(ETH_P_FCOE))
+		goto out;
+
 	if (skb->ip_summed == CHECKSUM_COMPLETE)
 		goto out_set_summed;

Shouldn't this be done in the offending driver rather than the core?

Well, yes and no, the driver is already using NETIF_F_FCOE_CRC to indicate
the FCoE CRC offload capability, and can_checksum_protocol() indicates that.
The fcoe protocol stack then uses CHECKSUM_PARTIAL and NETIF_F_FCOE_CRC to
pass through netdev w/o doing skb_checksum(), which is not captured here
in dev_hard_start_xmit() as it looks at NETIF_F_ALL_CSUM only before calling
skb_checksum_help().

Yi

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, 14 Mar 2012 18:54:32 +0000
"Zou, Yi" [off-list ref] wrote:

quoted

On Wed, 14 Mar 2012 00:01:58 -0700
Jeff Kirsher [off-list ref] wrote:

quoted

From: Yi Zou <redacted>

Fix a bug when using 'ethtool -K ethx tx off' to turn off tx ip

checksum,

quoted

FCoE CRC offload should not be impacte. The skb_checksum_help() is

needed

quoted

only if it's not FCoE traffic for ip checksum, regardless of ethtool

toggling

quoted

the tx ip checksum on or off.

Signed-off-by: Yi Zou <redacted>
Tested-by: Ross Brattain <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
 net/core/dev.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 6ca32f6..9e378009 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -1913,6 +1913,9 @@ int skb_checksum_help(struct sk_buff *skb)
 	__wsum csum;
 	int ret = 0, offset;

+	if (skb->protocol == htons(ETH_P_FCOE))
+		goto out;
+
 	if (skb->ip_summed == CHECKSUM_COMPLETE)
 		goto out_set_summed;

Shouldn't this be done in the offending driver rather than the core?

Well, yes and no, the driver is already using NETIF_F_FCOE_CRC to indicate
the FCoE CRC offload capability, and can_checksum_protocol() indicates that.
The fcoe protocol stack then uses CHECKSUM_PARTIAL and NETIF_F_FCOE_CRC to
pass through netdev w/o doing skb_checksum(), which is not captured here
in dev_hard_start_xmit() as it looks at NETIF_F_ALL_CSUM only before calling
skb_checksum_help().

But your change would break any code (if there is any) generating this
kind of packet and going through firewall, etc. For example a bridge or
router for FCOE packets.

On Wed, 2012-03-14 at 00:01 -0700, Jeff Kirsher wrote:

From: Yi Zou <redacted>

Fix a bug when using 'ethtool -K ethx tx off' to turn off tx ip checksum,
FCoE CRC offload should not be impacte. The skb_checksum_help() is needed
only if it's not FCoE traffic for ip checksum, regardless of ethtool toggling
the tx ip checksum on or off.

[...]

I think the bug is more fundamental, and it's not just a problem for
FCoE.  For the transmit path, the ip_summed values are specified as:

[forwarding]
 *	COMPLETE: the most generic way. Device supplied checksum of _all_
 *	    the packet as seen by netif_rx in skb->csum.
 *	    NOTE: Even if device supports only some protocols, but
 *	    is able to produce some skb->csum, it MUST use COMPLETE,
 *	    not UNNECESSARY.

[locally-generated]
 *	NONE: skb is checksummed by protocol or csum is not required.
 *
 *	PARTIAL: device is required to csum packet as seen by hard_start_xmit
 *	from skb->csum_start to the end and to record the checksum
 *	at skb->csum_start + skb->csum_offset.

It's implicit that the checksum algorithm for CHECKSUM_PARTIAL is as
specified for TCP/IP.  So none of those is correct when a different
algorithm is to be used.

It seems like we may need another ip_summed value for FCoE, SCTP or any
other protocol with a different checksum algorithm that will be
offloaded.  Maybe allow CHECKSUM_UNNECESSARY to be used on output in
that case?

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

On Wed, 14 Mar 2012 18:54:32 +0000
"Zou, Yi" [off-list ref] wrote:

quoted

quoted

On Wed, 14 Mar 2012 00:01:58 -0700
Jeff Kirsher [off-list ref] wrote:

quoted

From: Yi Zou <redacted>

Fix a bug when using 'ethtool -K ethx tx off' to turn off tx ip

checksum,

quoted

FCoE CRC offload should not be impacte. The skb_checksum_help() is

needed

quoted

only if it's not FCoE traffic for ip checksum, regardless of

ethtool

quoted

quoted

toggling

quoted

the tx ip checksum on or off.

Signed-off-by: Yi Zou <redacted>
Tested-by: Ross Brattain <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
 net/core/dev.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 6ca32f6..9e378009 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -1913,6 +1913,9 @@ int skb_checksum_help(struct sk_buff *skb)
 	__wsum csum;
 	int ret = 0, offset;

+	if (skb->protocol == htons(ETH_P_FCOE))
+		goto out;
+
 	if (skb->ip_summed == CHECKSUM_COMPLETE)
 		goto out_set_summed;

Shouldn't this be done in the offending driver rather than the core?

Well, yes and no, the driver is already using NETIF_F_FCOE_CRC to

indicate

quoted

the FCoE CRC offload capability, and can_checksum_protocol() indicates

that.

quoted

The fcoe protocol stack then uses CHECKSUM_PARTIAL and NETIF_F_FCOE_CRC

to

quoted

pass through netdev w/o doing skb_checksum(), which is not captured

here

quoted

in dev_hard_start_xmit() as it looks at NETIF_F_ALL_CSUM only before

calling

quoted

skb_checksum_help().

But your change would break any code (if there is any) generating this
kind of packet and going through firewall, etc. For example a bridge or
router for FCOE packets.

I don't know anything like that exists right now but in that case for fcoe,
you dan't want to use CHECKSUM_PARTIAL on output. Still, the point is the 
skb_checksum_help() only makes sense for ip checksum.

yi

On Wed, 2012-03-14 at 00:01 -0700, Jeff Kirsher wrote:

quoted

From: Yi Zou <redacted>

Fix a bug when using 'ethtool -K ethx tx off' to turn off tx ip

checksum,

quoted

FCoE CRC offload should not be impacte. The skb_checksum_help() is

needed

quoted

only if it's not FCoE traffic for ip checksum, regardless of ethtool

toggling

quoted

the tx ip checksum on or off.

[...]

I think the bug is more fundamental, and it's not just a problem for
FCoE.  For the transmit path, the ip_summed values are specified as:

[forwarding]
 *	COMPLETE: the most generic way. Device supplied checksum of _all_
 *	    the packet as seen by netif_rx in skb->csum.
 *	    NOTE: Even if device supports only some protocols, but
 *	    is able to produce some skb->csum, it MUST use COMPLETE,
 *	    not UNNECESSARY.

[locally-generated]
 *	NONE: skb is checksummed by protocol or csum is not required.
 *
 *	PARTIAL: device is required to csum packet as seen by
hard_start_xmit
 *	from skb->csum_start to the end and to record the checksum
 *	at skb->csum_start + skb->csum_offset.

It's implicit that the checksum algorithm for CHECKSUM_PARTIAL is as
specified for TCP/IP.  So none of those is correct when a different
algorithm is to be used.

It seems like we may need another ip_summed value for FCoE, SCTP or any
other protocol with a different checksum algorithm that will be
offloaded.  Maybe allow CHECKSUM_UNNECESSARY to be used on output in
that case?

Ben.

CHECKSUM_UNNECESSARY sounds good to me, if it's ok to be used on the tx path
as well, I think so but I am not 100% sure, it'd resolve this for fcoe and sctp
like, downside is it's a bigger change that requires corresponding changes in
these protocol driver stacks as well.

yi

--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

From: "Zou, Yi" <redacted>
Date: Wed, 14 Mar 2012 20:48:03 +0000

CHECKSUM_UNNECESSARY sounds good to me, if it's ok to be used on the tx path
as well, I think so but I am not 100% sure, it'd resolve this for fcoe and sctp
like, downside is it's a bigger change that requires corresponding changes in
these protocol driver stacks as well.

This is definitely the way this bug should be fixed, CHECKSUM_PARTIAL should
never be set for packets outside of the domain that value was designed for.

From: "Zou, Yi" <redacted>
Date: Wed, 14 Mar 2012 20:48:03 +0000

quoted

CHECKSUM_UNNECESSARY sounds good to me, if it's ok to be used on the tx

path 

quoted

as well, I think so but I am not 100% sure, it'd resolve this for fcoe

and sctp

quoted

like, downside is it's a bigger change that requires corresponding

changes in

quoted

these protocol driver stacks as well.

This is definitely the way this bug should be fixed, CHECKSUM_PARTIAL should
never be set for packets outside of the domain that value was designed for.

Cool, then please drop this patch and I will fix this by changing the fcoe 
protocol stack driver to use CHECKSUM_UNNECESSARY instead.

yi

From: "Zou, Yi" <redacted>
Date: Wed, 14 Mar 2012 22:04:06 +0000

quoted

From: "Zou, Yi" <redacted>
Date: Wed, 14 Mar 2012 20:48:03 +0000

quoted

CHECKSUM_UNNECESSARY sounds good to me, if it's ok to be used on the tx

path 

quoted

as well, I think so but I am not 100% sure, it'd resolve this for fcoe

and sctp

quoted

like, downside is it's a bigger change that requires corresponding

changes in

quoted

these protocol driver stacks as well.

This is definitely the way this bug should be fixed, CHECKSUM_PARTIAL should
never be set for packets outside of the domain that value was designed for.

Cool, then please drop this patch and I will fix this by changing the fcoe 
protocol stack driver to use CHECKSUM_UNNECESSARY instead.

I've dropped both patches.

On Wed, 2012-03-14 at 20:48 +0000, Zou, Yi wrote:

quoted

On Wed, 2012-03-14 at 00:01 -0700, Jeff Kirsher wrote:

quoted

From: Yi Zou <redacted>

Fix a bug when using 'ethtool -K ethx tx off' to turn off tx ip

checksum,

quoted

FCoE CRC offload should not be impacte. The skb_checksum_help() is

needed

quoted

only if it's not FCoE traffic for ip checksum, regardless of ethtool

toggling

quoted

the tx ip checksum on or off.

[...]

I think the bug is more fundamental, and it's not just a problem for
FCoE.  For the transmit path, the ip_summed values are specified as:

[forwarding]
 *	COMPLETE: the most generic way. Device supplied checksum of _all_
 *	    the packet as seen by netif_rx in skb->csum.
 *	    NOTE: Even if device supports only some protocols, but
 *	    is able to produce some skb->csum, it MUST use COMPLETE,
 *	    not UNNECESSARY.

[locally-generated]
 *	NONE: skb is checksummed by protocol or csum is not required.
 *
 *	PARTIAL: device is required to csum packet as seen by
hard_start_xmit
 *	from skb->csum_start to the end and to record the checksum
 *	at skb->csum_start + skb->csum_offset.

It's implicit that the checksum algorithm for CHECKSUM_PARTIAL is as
specified for TCP/IP.  So none of those is correct when a different
algorithm is to be used.

It seems like we may need another ip_summed value for FCoE, SCTP or any
other protocol with a different checksum algorithm that will be
offloaded.  Maybe allow CHECKSUM_UNNECESSARY to be used on output in
that case?

Ben.

CHECKSUM_UNNECESSARY sounds good to me, if it's ok to be used on the tx path
as well,

I don't believe it is yet.

I think so but I am not 100% sure, it'd resolve this for fcoe and sctp
like, downside is it's a bigger change that requires corresponding changes in
these protocol driver stacks as well.

Yes, but this should be done properly rather than patched up with a
bunch of checks for specific protocols.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

On Wed, 2012-03-14 at 20:48 +0000, Zou, Yi wrote:

quoted

quoted

On Wed, 2012-03-14 at 00:01 -0700, Jeff Kirsher wrote:

quoted

From: Yi Zou <redacted>

Fix a bug when using 'ethtool -K ethx tx off' to turn off tx ip

checksum,

quoted

FCoE CRC offload should not be impacte. The skb_checksum_help() is

needed

quoted

only if it's not FCoE traffic for ip checksum, regardless of

ethtool

quoted

quoted

toggling

quoted

the tx ip checksum on or off.

[...]

I think the bug is more fundamental, and it's not just a problem for
FCoE.  For the transmit path, the ip_summed values are specified as:

[forwarding]
 *	COMPLETE: the most generic way. Device supplied checksum of

_all_

quoted

quoted

 *	    the packet as seen by netif_rx in skb->csum.
 *	    NOTE: Even if device supports only some protocols, but
 *	    is able to produce some skb->csum, it MUST use COMPLETE,
 *	    not UNNECESSARY.

[locally-generated]
 *	NONE: skb is checksummed by protocol or csum is not required.
 *
 *	PARTIAL: device is required to csum packet as seen by
hard_start_xmit
 *	from skb->csum_start to the end and to record the checksum
 *	at skb->csum_start + skb->csum_offset.

It's implicit that the checksum algorithm for CHECKSUM_PARTIAL is as
specified for TCP/IP.  So none of those is correct when a different
algorithm is to be used.

It seems like we may need another ip_summed value for FCoE, SCTP or

any

quoted

quoted

other protocol with a different checksum algorithm that will be
offloaded.  Maybe allow CHECKSUM_UNNECESSARY to be used on output in
that case?

Ben.

CHECKSUM_UNNECESSARY sounds good to me, if it's ok to be used on the tx

path

quoted

as well,

I don't believe it is yet.

quoted

I think so but I am not 100% sure, it'd resolve this for fcoe and sctp
like, downside is it's a bigger change that requires corresponding

changes in

quoted

these protocol driver stacks as well.

Yes, but this should be done properly rather than patched up with a
bunch of checks for specific protocols.

Ben.

Agreed, however, I will still have to fix the netif_needs_gso() like below
to be able to use CHECKSUM_UNNECESSARY to not go down the dev_gso_segment()
path for FCoE, which is using SKB_GSO_FCOE.

So, anyone see this would break anything? i.e., using CHECKSUM_UNNECESSARY
but still want to do dev_gso_segment? If ok, I will resend the patch to
do the following.

Thanks,
yi

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 0eac07c..c1b2b5f 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h

@@ -2636,7 +2636,8 @@ static inline int netif_needs_gso(struct sk_buff *skb,
        netdev_features_t features)
 {
        return skb_is_gso(skb) && (!skb_gso_ok(skb, features) ||
-               unlikely(skb->ip_summed != CHECKSUM_PARTIAL));
+               unlikely((skb->ip_summed != CHECKSUM_PARTIAL) &&
+                        (skb->ip_summed != CHECKSUM_UNNECESSARY)));
 }

--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.