[ovs-dev] [PATCH] dpdk: add support for v2.1.0

Puha, TimoX timox.puha at intel.com
Fri Aug 28 11:18:56 UTC 2015


Hi,

Thanks for your comments. Some details inline.

> From: Daniele Di Proietto [mailto:diproiettod at vmware.com]
> >diff --git a/INSTALL.DPDK.md b/INSTALL.DPDK.md
> >index 35dd9a0..39e1b5d 100644
> >--- a/INSTALL.DPDK.md
> >+++ b/INSTALL.DPDK.md
> >@@ -16,7 +16,7 @@ OVS needs a system with 1GB hugepages support.
> > Building and Installing:
> > ------------------------
> >
> >-Required: DPDK 2.0
> >+Required: DPDK 2.1
> > Optional (if building with vhost-cuse): `fuse`, `fuse-devel`
> >(`libfuse-dev`
> > on Debian/Ubuntu)
> >
> >@@ -24,7 +24,7 @@ on Debian/Ubuntu)
> >   1. Set `$DPDK_DIR`
> >
> >      ```
> >-     export DPDK_DIR=/usr/src/dpdk-2.0
> >+     export DPDK_DIR=/usr/src/dpdk-2.1
> >      cd $DPDK_DIR
> >      ```
> 
> Should we also remove the requirement to change
> CONFIG_RTE_LIBRTE_VHOST?
> It is set by default in DPDK 2.1
Ok. I had missed this part getting out of date.

> >diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> >index 3444bb1..5798a93 100644
> >--- a/lib/netdev-dpdk.c
> >+++ b/lib/netdev-dpdk.c
> >@@ -68,7 +68,8 @@ static struct vlog_rate_limit rl =
> >VLOG_RATE_LIMIT_INIT(5, 20);
> >  */
> >
> > #define MTU_TO_MAX_LEN(mtu)  ((mtu) + ETHER_HDR_LEN +
> ETHER_CRC_LEN)
> >-#define MBUF_SIZE(mtu)       (MTU_TO_MAX_LEN(mtu) + (512) + \
> >+#define MBUF_PADDING         16
> >+#define MBUF_SIZE(mtu)       (MTU_TO_MAX_LEN(mtu) + (512) +
> >(MBUF_PADDING) + \
> >                              sizeof(struct rte_mbuf) +
> >RTE_PKTMBUF_HEADROOM)
> 
> This change was not immediately clear to me. Then I tried to remove it and
> I noticed a performance regression.
> 
> If the goal is just to make sure that the data room is at least
> 2048 + RTE_PKTMBUF_HEADROOM bytes, I would rewrite the macro like this:
> 
> #define MBUF_SIZE_MTU(mtu)   (MTU_TO_MAX_LEN(mtu) \
>                               + sizeof(struct dp_packet) \
>                               + RTE_PKTMBUF_HEADROOM)
> #define MBUF_SIZE_DRIVER     (2048 \
>                               + sizeof (struct rte_mbuf) \
>                               + RTE_PKTMBUF_HEADROOM)
> #define MBUF_SIZE(mtu)       MAX(MBUF_SIZE_MTU(mtu), MBUF_SIZE_DRIVER)
> 
> 
> and add a comment to explain the logic in the code.
> 
> What do you think?
As you suggest, the purpose is to ensure the minimum size is big enough to avoid scattering with normal Ethernet MTU.
We actually had a patch version similar to your suggestion at one time, but dropped it for the even simpler alternative because the jumbo frame support is not really there anyway. Anyway, I agree that this MTU sensitive code makes the intention clearer.

I will send a V2 soon.

-Timo



More information about the dev mailing list