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