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

[Kavanagh, Mark B]

>From: Stokes, Ian
>Sent: Thursday, January 18, 2018 5:35 PM
>To: Kavanagh, Mark B <mark.b.kavanagh at intel.com>; dev at openvswitch.org
>Subject: RE: [RFC PATCH v1 1/1] netdev-dpdk: Fix requested MTU size
>validation.
>
>> 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
>
>Thanks for the feedback Mark, v2 based on it has been posted.
>
>https://mail.openvswitch.org/pipermail/ovs-dev/2018-January/343390.html

Cheers Ian - I'll try to get to this tomorrow.
-Mark

>
>Thanks
>Ian
>> >
>> >>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