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

[Stokes, Ian]

> 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

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