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

Mon Jan 22 15:22:22 UTC 2018

> -----Original Message-----
> From: Kavanagh, Mark B
> Sent: Monday, January 22, 2018 2:59 PM
> To: Stokes, Ian <ian.stokes at intel.com>; dev at openvswitch.org
> 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: dev at openvswitch.org
> >Cc: Stokes, Ian <ian.stokes at intel.com>; Kavanagh, Mark B
> ><mark.b.kavanagh at intel.com>
> >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 <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>
> >Acked-by: Flavio Leitner <fbl at sysclose.org>
> >
> >---
> >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