>-----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. 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
