[ovs-dev] [PATCH v4 1/1] netdev-dpdk: Fix requested MTU size validation.

Tue Jan 23 11:19:41 UTC 2018

> -----Original Message-----
> From: Stokes, Ian
> Sent: Monday, January 22, 2018 6:27 PM
> To: dev at openvswitch.org
> Cc: Stokes, Ian <ian.stokes at intel.com>; Kavanagh, Mark B
> <mark.b.kavanagh at intel.com>
> Subject: [PATCH v4 1/1] netdev-dpdk: Fix requested MTU size validation.
> 

Hi all, thanks for the work reviewing this,

I've applied it to the DPDK merge branch, I'll also apply it for backporting on OVS 2.9, 2.8 and 2.7 as the issue is present in those. We can also apply it to 2.6 but will have to submit a new patch as the documentation format changed between 2.6 and 2.7.

Thanks
Ian
> This commit replaces MTU_TO_FRAME_LEN(mtu) with MTU_TO_MAX_FRAME_LEN(mtu)
> in netdev_dpdk_set_mtu(), in order to determine if the total length of the
> L2 frame with an MTU of ’mtu’ exceeds NETDEV_DPDK_MAX_PKT_LEN.
> 
> When setting an MTU we first check if the requested total frame length
> (which includes associated L2 overhead) will exceed the maximum frame
> length supported in netdev_dpdk_set_mtu(). The frame length is calculated
> by MTU_TO_FRAME_LEN  as MTU + ETHER_HEADER + ETHER_CRC. The MTU for the
> device will be set at a later stage in dpdk_eth_dev_init() using
> rte_eth_dev_set_mtu(mtu).
> 
> However when using rte_eth_dev_set_mtu(mtu) the calculation used to check
> that the frame does not exceed the max frame length for that device varies
> between DPDK device drivers. For example ixgbe driver calculates the frame
> length for a given MTU as
> 
> mtu + ETHER_HDR_LEN + ETHER_CRC_LEN
> 
> i40e driver calculates it as
> 
> mtu + ETHER_HDR_LEN + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE * 2
> 
> em driver calculates it as
> 
> mtu + ETHER_HDR_LEN + ETHER_CRC_LEN + VLAN_TAG_SIZE
> 
> Currently it is possible to set an MTU for a netdev_dpdk device that
> exceeds the upper limit MTU for that devices DPDK driver. This leads to a
> segfault.
> This is because the frame length comparison as is, does not take into
> account the addition of the vlan tag overhead expected in the drivers. The
> netdev_dpdk_set_mtu() call will incorrectly succeed but the subsequent
> dpdk_eth_dev_init() will fail before the queues have been created for the
> DPDK device. This coupled with assumptions regarding reconfiguration
> requirements for the netdev will lead to a segfault when the rxq is polled
> for this device.
> 
> A simple way to avoid this is by using MTU_TO_MAX_FRAME_LEN(mtu) when
> validating a requested MTU in netdev_dpdk_set_mtu().
> MTU_TO_MAX_FRAME_LEN(mtu) is equivalent to the following:
> 
> mtu + ETHER_HDR_LEN + ETHER_CRC_LEN + (2 * VLAN_HEADER_LEN)
> 
> By using MTU_TO_MAX_FRAME_LEN at the netdev_dpdk_set_mtu() stage, OvS now
> takes into account the maximum L2 overhead that a DPDK driver could allow
> for in its frame size calculation. This allows OVS to flag an error rather
> than the DPDK driver if the frame length exceeds the max DPDK frame
> length. OVS can fail gracefully at this point and use the default MTU of
> 1500 to continue to configure the port.
> 
> Note: this fix is a work around, a better approach would be if DPDK
> devices could report the maximum MTU value that can be requested on a per
> device basis. This capability however is not currently available. A
> downside of this patch is that the MTU upper limit will be reduced by 8
> bytes for DPDK devices that do not need to account for vlan tags in the
> frame length driver calculations e.g. ixgbe devices upper MTU limit is
> reduced from the OVS point of view from 9710 to 9702.
> 
> CC: Mark Kavanagh <mark.b.kavanagh at intel.com>
> Fixes: 0072e931 ("netdev-dpdk: add support for jumbo frames")
> Signed-off-by: Ian Stokes <ian.stokes at intel.com>
> Co-authored-by: Mark Kavanagh <mark.b.kavanagh at intel.com>
> Signed-off-by: Mark Kavanagh <mark.b.kavanagh at intel.com>
> Acked-by: Flavio Leitner <fbl at sysclose.org>
> 
> ---
> v3->v4
> * Add co-authored tags for Mark Kavanagh for commit message rework.
> * Specify 'L2 frame length' throughout commit message, documentation and
>   code instead of 'framse size'.
> 
> v2->v3
> * Remove RFC status.
> * Remove extra white space in documentation.
> * Correct dpdk_eth_dev_init() & rte_eth_dev_set_mtu() in commit message
>   and comments.
> * Use l2 frame calculation in limitation notes.
> * Misc minor grammar corrections.
> * Use 'default' instead of 'fallback' in commit message regarding MTU
>   1500.
> * Rephrase opening comment in netdev-dpdk.c set mtu code.
> * Add acked from Flavio Leitner.
> 
> v1->v2
> * Add note to limitations in DPDK documentation regarding MTU.
> * Correct MTU calculation in commit message.
> * Flag that we provision 8 bytes (2 x vlan header) by using
>   MTU_TO_MAX_FRAME_LEN in commit message and code comments.
> ---
>  Documentation/intro/install/dpdk.rst |   12 ++++++++++++
>  lib/netdev-dpdk.c                    |   13 ++++++++++++-
>  2 files changed, 24 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/intro/install/dpdk.rst
> b/Documentation/intro/install/dpdk.rst
> index 3fecb5c..4f182af 100644
> --- a/Documentation/intro/install/dpdk.rst
> +++ b/Documentation/intro/install/dpdk.rst
> @@ -585,6 +585,18 @@ Limitations
> 
>  .. _DPDK release notes:
> http://dpdk.org/doc/guides/rel_notes/release_17_11.html
> 
> +- Upper bound MTU: DPDK device drivers differ in how the L2 frame for a
> +  given MTU value is calculated e.g. i40e driver includes 2 x vlan
> +headers in
> +  MTU overhead, em driver includes 1 x vlan header, ixgbe driver does
> +not
> +  include a vlan  header in overhead. Currently it is not possible for
> +OVS
> +  DPDK to know what upper bound MTU value is supported for a given
> device.
> +  As such OVS DPDK must provision for the case where the L2 frame for a
> +given
> +  MTU includes 2 x vlan headers. This reduces the upper bound MTU value
> +for
> +  devices that do not include vlan headers in their L2 frames by 8 bytes
> e.g.
> +  ixgbe devices upper bound MTU is reduced from 9710 to 9702. This work
> +  around is temporary and is expected to be removed once a method is
> +provided
> +  by DPDK to query the upper bound MTU value for a given device.
> +
>  Reporting Bugs
>  --------------
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index e32c7f6..d0f9d44
> 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -2134,7 +2134,18 @@ netdev_dpdk_set_mtu(struct netdev *netdev, int mtu)
> {
>      struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> 
> -    if (MTU_TO_FRAME_LEN(mtu) > NETDEV_DPDK_MAX_PKT_LEN
> +    /* XXX: Ensure that the overall frame length of the requested MTU
> does not
> +     * surpass the NETDEV_DPDK_MAX_PKT_LEN. DPDK device drivers differ in
> how
> +     * the L2 frame length is calculated for a given MTU when
> +     * rte_eth_dev_set_mtu(mtu) is called e.g. i40e driver includes 2 x
> vlan
> +     * headers, the em driver includes 1 x vlan header, the ixgbe driver
> does
> +     * not include vlan headers. As such we should use
> +     * MTU_TO_MAX_FRAME_LEN(mtu) which includes an additional 2 x vlan
> headers
> +     * (8 bytes) for comparison. This avoids a failure later with
> +     * rte_eth_dev_set_mtu(). This approach should be used until DPDK
> provides
> +     * a method to retrieve the upper bound MTU for a given device.
> +     */
> +    if (MTU_TO_MAX_FRAME_LEN(mtu) > NETDEV_DPDK_MAX_PKT_LEN
>          || mtu < ETHER_MIN_MTU) {
>          VLOG_WARN("%s: unsupported MTU %d\n", dev->up.name, mtu);
>          return EINVAL;
> --
> 1.7.0.7