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

[Kavanagh, Mark B]

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

Hey Ian,

Thanks for the patch - some comments inline.

Cheers,
Mark

>This commit replaces MTU_TO_FRAME_LEN(mtu) with
>MTU_TO_MAX_FRAME_LEN(mtu) when validating if an MTU will exceed
>NETDEV_DPDK_MAX_PKT_LEN in netdev_dpdk_set_mtu().
>
>When setting an MTU we first check if the requested MTU frame
>size will exceed the maximum packet frame size supported in
>netdev_dpdk_set_mtu(). The MTU frame length is calculated as MTU +
>ETHER_HEADER + ETHER_CRC. The MTU for the device will be set at a later
>stage in dpdk_ethdev_init() using rte_ethdev_set_mtu(mtu).
>
>However when using rte_ethdev_set_mtu(mtu) the calculation used to check
>that the MTU does not exceed the max frame length for that device varies
>between DPDK device drivers. For example ixgbe driver calculates MTU
>frame length  as
>
>mtu + ETHER_HDR_LEN + ETHER_CRC_LEN
>
>i40e driver calculates it as
>
>ETHER_HDR_LEN + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE * 2

Missing text above, as you've already observed.

>
>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 MTU 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_ethdev_init() will fail before before queues have be

Typos in the line above: repetition of 'before'; also, s/be$/been/ .

>created for 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() to account for vlan
>overhead required by some devices in MTU frame length.

It would be worthwhile explaining why using MTU_TO_MAX_FRAME_LEN fixes the issue; i.e. MTU_TO_MAX_FRAME_LEN
takes into account 8 additional bytes of overhead when calculating the maximum frame size for a given MTU;
this corresponds to the upper bound of additional overhead factored in to the frame size calculation for
current DPDK drivers. 

>
>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 MTU frame length
>driver calculations e.g. ixgbe devices upper MTU limit is reduced from
>the ovs point of view from 9710 to 9702.

Is this case, how does OVSDB report the port's MTU - as 9710, or as 9702? If the latter, this could be problematic.
At the very least, this behavior should be documented in the errata (as you've outlined below), and also a warning should be issued to the user, to make them aware that MTU truncation has occurred.

>
>As this patch is RFC it has limited testing to i40e and ixgbe devices.
>If acceptable to the community documentation will also have to be
>updated under limitations I expect.
>
>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>
>---
> lib/netdev-dpdk.c |   11 ++++++++++-
> 1 files changed, 10 insertions(+), 1 deletions(-)
>
>diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>index 364f545..4da5292 100644
>--- a/lib/netdev-dpdk.c
>+++ b/lib/netdev-dpdk.c
>@@ -1999,7 +1999,16 @@ 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: We need to ensure the requested MTU frame length does not
>+     * surpass the NETDEV_DPDK_MAX_PKT_LEN. DPDK device drivers differ
>+     * in how the MTU frame length is calculated when
>rte_ethdev_set_mtu(mtu)
>+     * is called e.g. i40e and em drivers include vlan tags as part of the

As you've noted in the commit message, the em driver factors in the length of a single VLAN tag, but i40e (among others) factors in two.
It would be worth noting in the comment that the greater value (i.e. 8B) is taken here.

>+     * MTU frame length. As such we should use MTU_TO_MAX_FRAME_LEN(mtu)
>+     * for comparison here to avoid a failure later with
>rte_ethdev_set_mtu().
>+     * This approach should be used until DPDK provides a method to
>retrieve
>+     * maximum MTU frame length 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