> -----Original Message----- > From: Kavanagh, Mark B > Sent: Monday, January 22, 2018 2:59 PM > To: Stokes, Ian <[email protected]>; [email protected] > Subject: RE: [PATCH v3 1/1] netdev-dpdk: Fix requested MTU size > validation. > > Hey Ian, > > Sorry to be a stickler, but there are a few areas where the difference > between 'frame length' and 'MTU' still needs to be clarified, so as to > avoid confusion. > > I mentioned this as a general comment last time - apologies if I wasn't > super-clear. I'll try and point out those areas now. > > Thanks, > Mark > > >-----Original Message----- > >From: Stokes, Ian > >Sent: Monday, January 22, 2018 2:18 PM > >To: [email protected] > >Cc: Stokes, Ian <[email protected]>; Kavanagh, Mark B > ><[email protected]> > >Subject: [PATCH v3 1/1] netdev-dpdk: Fix requested MTU size validation. > > > >This commit replaces MTU_TO_FRAME_LEN(mtu) with > >MTU_TO_MAX_FRAME_LEN(mtu) when validating if an MTU will exceed > > It's not actually the MTU that exceeds METDEV_DPDK_MAX_PKT_LEN; rather, > it's the resultant frame size that's calculated, based on the provided MTU > value. > > >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 > > I'd avoid using the term "MTU frame size" anywhere, since it's a bit > confusing/misleading (i.e. mixes up L2 and L3).
Ok, but the macros being called are MTU_TO_FRAME_LEN and MTU_TO_MAX_FRAME_LEN. I had followed this originally when writing the commit hence the frame size entry above. I can change above and throughout the code to reference MTU to frame length calculation if that helps? If it doesn't then and it's a sticking point then I'd suggest a separate patch to re-name the macros to be less confusing and mixing the layers. Ian > > >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_eth_dev_init() using rte_eth_dev_set_mtu(mtu). > > > >However when using rte_eth_dev_set_mtu(mtu) the calculation used to > >check that the MTU does not exceed the max frame length for that device > >varies > > Again, it's not the MTU that exceeds the max frame length, but the length > of the overall frame, as determined by the DPDK driver, based on the L3 > MTU. > Taking this into account, I'd suggest refactoring the rest of the > doc/commit message accordingly. > > >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 > > > >mtu + ETHER_HDR_LEN + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE * 2 > > > >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_eth_dev_init() will fail before the queues have > >been created for the 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(). > >MTU_TO_MAX_FRAME_LEN(mtu) is equivalent to the following: > > > >mtu + ETHER_HDR_LEN + ETHER_CRC_LEN + (2 * VLAN_HEADER_LEN) > > > >Its use accounts for the potential of up to two vlan headers being > >included in the overhead required by some devices in MTU frame length > >at the netdev_dpdk_set_mtu() stage. This allows OVS to flag an error > >rather than the DPDK driver if the MTU exceeds the max DPDK frame > >length. OVS can fail gracefully at this point and use the default MTU > >of 1500 to continue to configure the port. > > > >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. > > > >CC: Mark Kavanagh <[email protected]> > >Fixes: 0072e931 ("netdev-dpdk: add support for jumbo frames") > >Signed-off-by: Ian Stokes <[email protected]> > >Acked-by: Flavio Leitner <[email protected]> > > > >--- > >v2->v3 > >* Remove RFC status. > >* Remove extra white space in documentation. > >* Correct dpdk_eth_dev_init() & rte_eth_dev_set_mtu() in commit message > > and comments. > >* Use l2 frame calculation in limitation notes. > >* Misc minor grammar corrections. > >* Use 'default' instead of 'fallback' in commit message regarding MTU > > 1500. > >* Rephrase opening comment in netdev-dpdk.c set mtu code. > >* Add acked from Flavio Leitner. > > > >v1->v2 > >* Add note to limitations in DPDK documentation regarding MTU. > >* Correct MTU calculation in commit message. > >* Flag that we provision 8 bytes (2 x vlan header) by using > > MTU_TO_MAX_FRAME_LEN in commit message and code comments. > >--- > > Documentation/intro/install/dpdk.rst | 12 ++++++++++++ > > lib/netdev-dpdk.c | 13 ++++++++++++- > > 2 files changed, 24 insertions(+), 1 deletions(-) > > > >diff --git a/Documentation/intro/install/dpdk.rst > >b/Documentation/intro/install/dpdk.rst > >index 3fecb5c..f071cc9 100644 > >--- a/Documentation/intro/install/dpdk.rst > >+++ b/Documentation/intro/install/dpdk.rst > >@@ -585,6 +585,18 @@ Limitations > > > > .. _DPDK release notes: > >http://dpdk.org/doc/guides/rel_notes/release_17_11.html > > > >+- Upper bound MTU: DPDK device drivers differ in how the L2 overhead > >+for a > >+ given MTU value is calculated e.g. i40e driver includes 2 x vlan > >+headers > >in > >+ MTU overhead, em driver includes 1 x vlan header, ixgbe driver does > >+ not include a vlan header in overhead. Currently it is not possible > >+ for OVS DPDK to know what upper bound MTU value is supported for a > given device. > >+ As such OVS DPDK must provision for the case where the maximum MTU > >+ value includes 2 x vlan headers. This reduces the upper bound MTU > >+ value for devices that do not include vlan headers in their overhead > >+ by 8 bytes > >e.g. > >+ ixgbe devices upper bound MTU is reduced from 9710 to 9702. This > >+ work around is temporary and is expected to be removed once a method > >+ is > >provided > >+ by DPDK to query the maximum MTU for a given device. > >+ > > Reporting Bugs > > -------------- > > > >diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index > >e32c7f6..37ce6a0 100644 > >--- a/lib/netdev-dpdk.c > >+++ b/lib/netdev-dpdk.c > >@@ -2134,7 +2134,18 @@ 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: Ensure that the overall frame length of the requested MTU > >+ does > >not > >+ * surpass the NETDEV_DPDK_MAX_PKT_LEN. DPDK device drivers differ > >+ * in how the MTU frame length is calculated when > >rte_eth_dev_set_mtu(mtu) > >+ * is called e.g. i40e driver includes 2 x vlan headers in MTU > overhead > >+ * the em driver includes 1 x vlan tag in MTU overhead, the ixgbe > >driver > >+ * does not include vlan tags in MTU overhead. As such we should use > >+ * MTU_TO_MAX_FRAME_LEN(mtu) which includes an additional 2 x vlan > >headers > >+ * (8 bytes) for comparison. This avoids a failure later with > >+ * rte_eth_dev_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
