Quick follow-up inline. >-----Original Message----- >From: Kavanagh, Mark B >Sent: Friday, January 12, 2018 5:00 PM >To: Stokes, Ian <[email protected]>; [email protected] >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: [email protected] >>Cc: Stokes, Ian <[email protected]>; Kavanagh, Mark B >><[email protected]> >>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 <[email protected]> >>Fixes: 0072e931 ("netdev-dpdk: add support for jumbo frames") >>Signed-off-by: Ian Stokes <[email protected]> >>--- >> 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
