Date:   Mon, 15 Jan 2018 14:58:03 +0000
From:   "Chopra, Manish" <Manish.Chopra@...ium.com>
To:     Daniel Axtens <dja@...ens.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     Thomas Falcon <tlfalcon@...ux.vnet.ibm.com>,
        "Mintz, Yuval" <Yuval.Mintz@...ium.com>
Subject: RE: [PATCH v2] bnx2x: disable GSO where gso_size is too big for
 hardware

> -----Original Message-----
> From: netdev-owner@...r.kernel.org [mailto:netdev-owner@...r.kernel.org]
> On Behalf Of Daniel Axtens
> Sent: Friday, January 12, 2018 5:29 AM
> To: netdev@...r.kernel.org
> Cc: Daniel Axtens <dja@...ens.net>; Thomas Falcon
> <tlfalcon@...ux.vnet.ibm.com>; Mintz, Yuval <Yuval.Mintz@...ium.com>
> Subject: [PATCH v2] bnx2x: disable GSO where gso_size is too big for hardware
> 
> If a bnx2x card is passed a GSO packet with a gso_size larger than
> ~9700 bytes, it will cause a firmware error that will bring the card
> down:
> 
> bnx2x: [bnx2x_attn_int_deasserted3:4323(enP24p1s0f0)]MC assert!
> bnx2x: [bnx2x_mc_assert:720(enP24p1s0f0)]XSTORM_ASSERT_LIST_INDEX 0x2
> bnx2x: [bnx2x_mc_assert:736(enP24p1s0f0)]XSTORM_ASSERT_INDEX 0x0 =
> 0x00000000 0x25e43e47 0x00463e01 0x00010052
> bnx2x: [bnx2x_mc_assert:750(enP24p1s0f0)]Chip Revision: everest3, FW
> Version: 7_13_1 ... (dump of values continues) ...
> 
> Detect when gso_size + header length is greater than the maximum packet size
> (9700 bytes) and disable GSO. For simplicity and speed this is approximated by
> comparing gso_size against 9200 and assuming no-one will have more than 500
> bytes of headers.
> 
> This raises the obvious question - how do we end up with a packet with a
> gso_size that's greater than 9700? This has been observed on an powerpc
> system when Open vSwitch is forwarding a packet from an ibmveth device.
> 
> ibmveth is a bit special. It's the driver for communication between virtual
> machines (aka 'partitions'/LPARs) running under IBM's proprietary hypervisor on
> ppc machines. It allows sending very large packets (up to 64kB) between LPARs.
> This involves some quite 'interesting' things: for example, when talking TCP, the
> MSS is stored the checksum field (see ibmveth_rx_mss_helper() in ibmveth.c).
> 
> Normally on a box like this, there would be a Virtual I/O Server
> (VIOS) partition that owns the physical network card. VIOS lets the AIX partitions
> know when they're talking to a real network and that they should drop their
> MSS. This works fine if VIOS owns the physical network card.
> 
> However, in this case, a Linux partition owns the card (this is known as a
> NovaLink setup). The negotiation between VIOS and AIX uses a non-standard
> TCP option, so Linux has never supported that.  Instead, Linux just supports
> receiving large packets. It doesn't support any form of messaging/MSS
> negotiation back to other LPARs.
> 
> To get some clarity about where the large MSS was coming from, I asked
> Thomas Falcon, the maintainer of ibmveth, for some background:
> 
> "In most cases, large segments are an aggregation of smaller packets by the
> Virtual I/O Server (VIOS) partition and then are forwarded to the Linux LPAR /
> ibmveth driver. These segments can be as large as 64KB. In this case, since the
> customer is using Novalink, I believe what is happening is pretty straightforward:
> the large segments are created by the AIX partition and then forwarded to the
> Linux partition, ... The ibmveth driver doesn't do any aggregation itself but just
> ensures the proper bits are set before sending the frame up to avoid giving the
> upper layers indigestion."
> 
> It is possible to stop AIX from sending these large segments, but it requires
> configuration on each LPAR. While ibmveth's behaviour is admittedly weird, we
> should fix this here: it shouldn't be possible for it to cause a firmware panic on
> another card.
> 
> Cc: Thomas Falcon <tlfalcon@...ux.vnet.ibm.com> # ibmveth
> Cc: Yuval Mintz <Yuval.Mintz@...ium.com> # bnx2x
> Thanks-to: Jay Vosburgh <jay.vosburgh@...onical.com> # veth info
> Signed-off-by: Daniel Axtens <dja@...ens.net>
> 
> ---
> v2: change to a feature check as suggested by Eric Dumazet.
> 
> ---
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> index 7b08323e3f3d..bab909b5d7a2 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> @@ -12934,6 +12934,17 @@ static netdev_features_t
> bnx2x_features_check(struct sk_buff *skb,
>  					      struct net_device *dev,
>  					      netdev_features_t features)
>  {
> +	/*
> +	 * A skb with gso_size + header length > 9700 will cause a
> +	 * firmware panic. Drop GSO support.
> +	 *
> +	 * To avoid costly calculations on all packets (and because
> +	 * super-jumbo frames are rare), allow 500 bytes of headers
> +	 * and just disable GSO if gso_size is greater than 9200.
> +	 */
> +	if (unlikely(skb_is_gso(skb) && skb_shinfo(skb)->gso_size > 9200))
> +		features &= ~NETIF_F_GSO_MASK;
> +
>  	features = vlan_features_check(skb, features);
>  	return vxlan_features_check(skb, features);  }
> --
> 2.14.1

This is merely a question on kernel stack, how kernel might end up sending such GSO packets to the device
considering MTUs of underlined device ? Not sure if this should be handled at driver level or at kernel stack level ?

In general, when I run iperf with MTU 1500, I get gso_size as 1448.
When running with 9600 MTU, I get gso_size as 9548.

With this change, considering Jumbo MTU, even if device is able to handle a packet with gso_size like 9548, it will cause it to disable GSO
Considering jumbo frames are rare, doesn't sound well. Since there are customers/people which might be utilizing jumbo MTUs.

Thanks,
Manish

Powered by blists - more mailing lists