[ovs-dev,RFC,v3,1/8] netdev-dpdk: simplify mbuf sizing

When calculating the mbuf data_room_size (i.e. the size of actual
packet data that an mbuf can accomodate), it is possible to simply
use the value calculated by dpdk_buf_size() as a parameter to
rte_pktmbuf_pool_create(). This simplifies mbuf sizing considerably.
This patch removes the related size conversions and macros, which
are no longer needed.

The benefits of this approach are threefold:
- the mbuf sizing code is much simpler, and more readable.
- mbuf size will always be cache-aligned [1], satisfying that
  requirement of specific PMDs (vNIC thunderx, for example).
- the maximum amount of data that each mbuf contains may now be
  calculated as mbuf->buf_len - mbuf->data_off. This is important
  in the case of multi-segment jumbo frames.

[1] (this is true since mbuf size is now always a multiple
     of 1024, + 128B RTE_PKTMBUF_HEADROOM + 704B dp_packet).

Fixes: 4be4d22 ("netdev-dpdk: clean up mbuf initialization")
Fixes: 31b88c9 ("netdev-dpdk: round up mbuf_size to cache_line_size")
Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
---
 lib/netdev-dpdk.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

+ Santosh

>From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org]
>On Behalf Of Mark Kavanagh
>Sent: Tuesday, November 21, 2017 6:29 PM
>To: dev@openvswitch.org; qiudayu@chinac.com
>Subject: [ovs-dev] [RFC PATCH v3 1/8] netdev-dpdk: simplify mbuf sizing
>
>When calculating the mbuf data_room_size (i.e. the size of actual
>packet data that an mbuf can accomodate), it is possible to simply
>use the value calculated by dpdk_buf_size() as a parameter to
>rte_pktmbuf_pool_create(). This simplifies mbuf sizing considerably.
>This patch removes the related size conversions and macros, which
>are no longer needed.
>
>The benefits of this approach are threefold:
>- the mbuf sizing code is much simpler, and more readable.
>- mbuf size will always be cache-aligned [1], satisfying that
>  requirement of specific PMDs (vNIC thunderx, for example).
>- the maximum amount of data that each mbuf contains may now be
>  calculated as mbuf->buf_len - mbuf->data_off. This is important
>  in the case of multi-segment jumbo frames.
>
>[1] (this is true since mbuf size is now always a multiple
>     of 1024, + 128B RTE_PKTMBUF_HEADROOM + 704B dp_packet).

Santosh, I've just spotted that the cacheline size for thunderx is defined as 128B, as opposed to 64.
	
	==> config/defconfig_arm64-thunderx-linuxapp-gcc:36:CONFIG_RTE_CACHE_LINE_SIZE=128

Apologies for the oversight - I'd be very interested to hear your thoughts on this patch.

Thanks in advance,
Mark


>
>Fixes: 4be4d22 ("netdev-dpdk: clean up mbuf initialization")
>Fixes: 31b88c9 ("netdev-dpdk: round up mbuf_size to cache_line_size")
>Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
>---
> lib/netdev-dpdk.c | 22 ++++++++--------------
> 1 file changed, 8 insertions(+), 14 deletions(-)
>
>diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>index 8906423..c5eb851 100644
>--- a/lib/netdev-dpdk.c
>+++ b/lib/netdev-dpdk.c
>@@ -81,12 +81,6 @@ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
>20);
>                                      + (2 * VLAN_HEADER_LEN))
> #define MTU_TO_FRAME_LEN(mtu)       ((mtu) + ETHER_HDR_LEN + ETHER_CRC_LEN)
> #define MTU_TO_MAX_FRAME_LEN(mtu)   ((mtu) + ETHER_HDR_MAX_LEN)
>-#define FRAME_LEN_TO_MTU(frame_len) ((frame_len)                    \
>-                                     - ETHER_HDR_LEN - ETHER_CRC_LEN)
>-#define MBUF_SIZE(mtu)              ROUND_UP((MTU_TO_MAX_FRAME_LEN(mtu) \
>-                                             + sizeof(struct dp_packet) \
>-                                             + RTE_PKTMBUF_HEADROOM),   \
>-                                             RTE_CACHE_LINE_SIZE)
> #define NETDEV_DPDK_MBUF_ALIGN      1024
> #define NETDEV_DPDK_MAX_PKT_LEN     9728
>
>@@ -447,7 +441,7 @@ is_dpdk_class(const struct netdev_class *class)
>  * behaviour, which reduces performance. To prevent this, use a buffer size
>  * that is closest to 'mtu', but which satisfies the aforementioned criteria.
>  */
>-static uint32_t
>+static uint16_t
> dpdk_buf_size(int mtu)
> {
>     return ROUND_UP((MTU_TO_MAX_FRAME_LEN(mtu) + RTE_PKTMBUF_HEADROOM),
>@@ -486,7 +480,7 @@ ovs_rte_pktmbuf_init(struct rte_mempool *mp OVS_UNUSED,
>  *  - a new mempool was just created;
>  *  - a matching mempool already exists. */
> static struct rte_mempool *
>-dpdk_mp_create(struct netdev_dpdk *dev, int mtu)
>+dpdk_mp_create(struct netdev_dpdk *dev, uint16_t frame_len)
> {
>     char mp_name[RTE_MEMPOOL_NAMESIZE];
>     const char *netdev_name = netdev_get_name(&dev->up);
>@@ -513,12 +507,12 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu)
>          * longer than RTE_MEMPOOL_NAMESIZE. */
>         int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE,
>                            "ovs%08x%02d%05d%07u",
>-                           hash, socket_id, mtu, n_mbufs);
>+                           hash, socket_id, frame_len, n_mbufs);
>         if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {
>             VLOG_DBG("snprintf returned %d. "
>                      "Failed to generate a mempool name for \"%s\". "
>-                     "Hash:0x%x, socket_id: %d, mtu:%d, mbufs:%u.",
>-                     ret, netdev_name, hash, socket_id, mtu, n_mbufs);
>+                     "Hash:0x%x, socket_id: %d, frame length:%d, mbufs:%u.",
>+                     ret, netdev_name, hash, socket_id, frame_len, n_mbufs);
>             break;
>         }
>
>@@ -529,7 +523,7 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu)
>
>         mp = rte_pktmbuf_pool_create(mp_name, n_mbufs, MP_CACHE_SZ,
>                  sizeof (struct dp_packet) - sizeof (struct rte_mbuf),
>-                 MBUF_SIZE(mtu) - sizeof(struct dp_packet), socket_id);
>+                 frame_len + RTE_PKTMBUF_HEADROOM, socket_id);
>
>         if (mp) {
>             VLOG_DBG("Allocated \"%s\" mempool with %u mbufs",
>@@ -582,11 +576,11 @@ static int
> netdev_dpdk_mempool_configure(struct netdev_dpdk *dev)
>     OVS_REQUIRES(dev->mutex)
> {
>-    uint32_t buf_size = dpdk_buf_size(dev->requested_mtu);
>+    uint16_t buf_size = dpdk_buf_size(dev->requested_mtu);
>     struct rte_mempool *mp;
>     int ret = 0;
>
>-    mp = dpdk_mp_create(dev, FRAME_LEN_TO_MTU(buf_size));
>+    mp = dpdk_mp_create(dev, buf_size);
>     if (!mp) {
>         VLOG_ERR("Failed to create memory pool for netdev "
>                  "%s, with MTU %d on socket %d: %s\n",
>--
>1.9.3
>
>_______________________________________________
>dev mailing list
>dev@openvswitch.org
>https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On Wednesday 22 November 2017 06:54 PM, Kavanagh, Mark B wrote:
> + Santosh
>
>> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org]
>> On Behalf Of Mark Kavanagh
>> Sent: Tuesday, November 21, 2017 6:29 PM
>> To: dev@openvswitch.org; qiudayu@chinac.com
>> Subject: [ovs-dev] [RFC PATCH v3 1/8] netdev-dpdk: simplify mbuf sizing
>>
>> When calculating the mbuf data_room_size (i.e. the size of actual
>> packet data that an mbuf can accomodate), it is possible to simply
>> use the value calculated by dpdk_buf_size() as a parameter to
>> rte_pktmbuf_pool_create(). This simplifies mbuf sizing considerably.
>> This patch removes the related size conversions and macros, which
>> are no longer needed.
>>
>> The benefits of this approach are threefold:
>> - the mbuf sizing code is much simpler, and more readable.
>> - mbuf size will always be cache-aligned [1], satisfying that
>>  requirement of specific PMDs (vNIC thunderx, for example).
>> - the maximum amount of data that each mbuf contains may now be
>>  calculated as mbuf->buf_len - mbuf->data_off. This is important
>>  in the case of multi-segment jumbo frames.
>>
>> [1] (this is true since mbuf size is now always a multiple
>>     of 1024, + 128B RTE_PKTMBUF_HEADROOM + 704B dp_packet).
> Santosh, I've just spotted that the cacheline size for thunderx is defined as 128B, as opposed to 64.
> 	
> 	==> config/defconfig_arm64-thunderx-linuxapp-gcc:36:CONFIG_RTE_CACHE_LINE_SIZE=128
>
> Apologies for the oversight - I'd be very interested to hear your thoughts on this patch.
>
> Thanks in advance,
> Mark
>
Sorry for late reply.
I'm reviewing changeset now and Will share my feedback on series:
[PATCH v8 01/13] netdev-dpdk: fix mbuf sizing.

Thanks.