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

Fri Jan 12 17:03:16 UTC 2018

Quick follow-up inline.

>-----Original Message-----
>From: Kavanagh, Mark B
>Sent: Friday, January 12, 2018 5:00 PM
>To: Stokes, Ian <ian.stokes at intel.com>; dev at openvswitch.org
>Subject: RE: [RFC PATCH v1 1/1] netdev-dpdk: Fix requested MTU size
>validation.
>
>
>
>>-----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.

Scratch that last comment - I just realized that values 9703-9710 will be rejected when attempting to set the MTU.
Thanks,
Mark

>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