[v2,4/6] netdev-odp: allocate packet metadata in the same buffer as the packet itself

Allocating it after every packet receive gives a big performance penalty. So
move it into the same buffer pool, right after the packet itself.

Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org>
---
v2:
- fix style of for loops
- delete SHM_OFPBUF_NUM_BUFS and odp_ofpbuf
- no need to DPDK offset hack anymore, it was a DPDK error
- init ofbuf at startup time

 lib/netdev-odp.c | 65 +++++++++++++++++++++++++++++---------------------------
 lib/ofpbuf.c     |  1 -
 lib/ofpbuf.h     |  1 -
 3 files changed, 34 insertions(+), 33 deletions(-)

On Thu, Mar 26, 2015 at 9:23 PM, Zoltan Kiss <zoltan.kiss@linaro.org> wrote:
> Allocating it after every packet receive gives a big performance penalty. So
> move it into the same buffer pool, right after the packet itself.
>
> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org>
> ---
> v2:
> - fix style of for loops
> - delete SHM_OFPBUF_NUM_BUFS and odp_ofpbuf
> - no need to DPDK offset hack anymore, it was a DPDK error
> - init ofbuf at startup time
>
>  lib/netdev-odp.c | 65 +++++++++++++++++++++++++++++---------------------------
>  lib/ofpbuf.c     |  1 -
>  lib/ofpbuf.h     |  1 -
>  3 files changed, 34 insertions(+), 33 deletions(-)
>
> diff --git a/lib/netdev-odp.c b/lib/netdev-odp.c
> index ae944f7..b77eea0 100644
> --- a/lib/netdev-odp.c
> +++ b/lib/netdev-odp.c
> @@ -52,13 +52,11 @@ VLOG_DEFINE_THIS_MODULE(odp);
>  #define SHM_PKT_POOL_NUM_BUFS  32768 - 1
>  #define SHM_PKT_POOL_BUF_SIZE  1856
>
> -#define SHM_OFPBUF_NUM_BUFS      512
>  #define SHM_OFPBUF_POOL_BUF_SIZE  sizeof(struct dpif_packet)
>
>  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
>
>  static odp_buffer_pool_t pool;
> -static odp_buffer_pool_t ofpbuf_pool;
>  static odp_buffer_pool_t struct_pool;
>
>  static int odp_initialized = 0;
> @@ -95,7 +93,6 @@ void
>  free_odp_buf(struct ofpbuf *b)
>  {
>      odp_packet_free(b->odp_pkt);
> -    odp_buffer_free(b->odp_ofpbuf);
>  }
>
>  int
> @@ -130,11 +127,12 @@ static int
>  odp_class_init(void)
>  {
>      odp_buffer_pool_param_t params;
> -    int result = 0;
> +    int result = 0, i;
> +    odp_packet_t* pkts;
>
> -    /* create packet pool */
> +    /* create packet & ofpbuf pool */
>
> -    params.buf_size = SHM_PKT_POOL_BUF_SIZE;
> +    params.buf_size = SHM_PKT_POOL_BUF_SIZE + SHM_OFPBUF_POOL_BUF_SIZE;
>      params.buf_align = ODP_CACHE_LINE_SIZE;
>      params.num_bufs = SHM_PKT_POOL_NUM_BUFS;
>      params.buf_type = ODP_BUFFER_TYPE_PACKET;
> @@ -147,19 +145,32 @@ odp_class_init(void)
>      }
>      odp_buffer_pool_print(pool);
>
> -    /* create ofpbuf pool */
> -
> -    params.buf_size = SHM_OFPBUF_POOL_BUF_SIZE;
> -    params.num_bufs = SHM_OFPBUF_POOL_BUF_SIZE;
> -    params.buf_type = ODP_BUFFER_TYPE_RAW;
> -
> -    ofpbuf_pool = odp_buffer_pool_create("ofpbuf_pool", ODP_SHM_NULL, &params);
> -
> -    if (ofpbuf_pool == ODP_BUFFER_POOL_INVALID) {
> -            VLOG_ERR("Error: ofpbuf pool create failed.\n");
> +    /* Allocate all the packets from the pool and initialize ofpbuf part */
> +    pkts = malloc(sizeof(odp_packet_t) * params.num_bufs);
> +    for (i = 0; i < params.num_bufs; ++i) {
> +        struct dpif_packet *packet;
> +        char *start;
> +        pkts[i] = odp_packet_alloc(pool, 0);
> +        if (pkts[i] == ODP_PACKET_INVALID) {
> +            VLOG_ERR("Error: packet allocation failed.\n");
>              return -1;

Sorrry, I wasn't clear last time. This whole piece of code in
odp_class_init is not portable, ODP does not guarantee you that
buffers / packets will always be at the same address (even though for
odp-keystone2 this might actually be true), so adding the ofpbuf
struct at the end of the buffer here does not necessarily mean it will
be at the same offset when you allocate a packet.

In addition the KS2 implementation will not allocate pools with more
than 1024 buffers, even if you request 32767, so the for above will
cause vswitchd to crash. This is a problem too I think, but I can deal
with that separately. Maybe Taras can comment on this, I think
odp_pool_create should fail if it cannot provide the requested number
of buffers.

The portable solution would be to initialize the ofpbuf part in the
receive call, although this might mean additional performance penalty
compared to how you can do it with odp-dpdk, but at least we're using
the same buffer.

> -    }
> -    odp_buffer_pool_print(ofpbuf_pool);
> +        }
> +        start = (char *)odp_buffer_addr(odp_packet_to_buffer(pkts[i]));
> +        packet = (struct dpif_packet*) (start + SHM_PKT_POOL_BUF_SIZE);
> +        packet->ofpbuf.odp_pkt = pkts[i];
> +        ofpbuf_init_odp(&packet->ofpbuf, SHM_PKT_POOL_BUF_SIZE);
> +        /* TODO: odp_packet_alloc reset this to ODP_PACKET_OFFSET_INVALID, and
> +         * ODP-DPDK doesn't set it later on, which causes a crash in
> +         * emc_processing. Workaround this problem here for the moment */
> +        odp_packet_l2_offset_set(pkts[i], 0);
> +     }
> +
> +    /* Free our packets up */
> +    for (i = 0; i < params.num_bufs; i++)
> +        odp_packet_free(pkts[i]);
> +    free(pkts);
> +
> +    odp_buffer_pool_print(pool);
>
>      /* create pool for structures */
>
> @@ -359,10 +370,8 @@ netdev_odp_send(struct netdev *netdev, int qid OVS_UNUSED,
>              }
>          }
>      } else {
> -        for (i = 0; i < cnt; i++) {
> +        for (i = 0; i < cnt; i++)
>              odp_pkts[i] = pkts[i]->ofpbuf.odp_pkt;
> -            odp_packet_free(pkts[i]->ofpbuf.odp_ofpbuf);
> -        }
>          pkts_ok = cnt;
>      }
>
> @@ -605,17 +614,11 @@ netdev_odp_rxq_recv(struct netdev_rxq *rxq_, struct dpif_packet **packets,
>          return EINVAL;
>      }
>
> -    /* Allocate an ofpbuf for each valid packet */
> +    /* Build the array of dpif_packet pointers */
>      for (i = 0; i < pkts_ok; i++) {
> -        odp_buffer_t buf;
> -        buf = odp_buffer_alloc(ofpbuf_pool);
> -        if (buf == ODP_BUFFER_INVALID) {
> -            out_of_memory();
> -        }
> -        packets[i] = (struct dpif_packet*) odp_buffer_addr(buf);
> -        ofpbuf_init_odp(&packets[i]->ofpbuf, odp_packet_buf_len(pkt_tbl[i]));
> -        packets[i]->ofpbuf.odp_pkt = pkt_tbl[i];
> -        packets[i]->ofpbuf.odp_ofpbuf = buf;
> +        char *start;
> +        start = (char*)odp_buffer_addr(odp_packet_to_buffer(pkt_tbl[i]));
> +        packets[i] = (struct dpif_packet*) (start + SHM_PKT_POOL_BUF_SIZE);
>          rx_bytes += odp_packet_len(pkt_tbl[i]);
>      }
>
> diff --git a/lib/ofpbuf.c b/lib/ofpbuf.c
> index 6f27b47..06a8c1c 100644
> --- a/lib/ofpbuf.c
> +++ b/lib/ofpbuf.c
> @@ -155,7 +155,6 @@ ofpbuf_uninit(struct ofpbuf *b)
>          } else if (b->source == OFPBUF_ODP) {
>  #ifdef ODP_NETDEV
>              odp_packet_free(b->odp_pkt);
> -            odp_buffer_free(b->odp_ofpbuf);
>  #else
>              ovs_assert(b->source != OFPBUF_ODP);
>  #endif
> diff --git a/lib/ofpbuf.h b/lib/ofpbuf.h
> index 47cee29..33f3d6a 100644
> --- a/lib/ofpbuf.h
> +++ b/lib/ofpbuf.h
> @@ -64,7 +64,6 @@ struct ofpbuf {
>      struct rte_mbuf mbuf;       /* DPDK mbuf */
>  #else
>  # ifdef ODP_NETDEV
> -    odp_buffer_t odp_ofpbuf;    /* ODP buffer containig this struct ofpbuf */
>      odp_packet_t odp_pkt;       /* ODP packet containing actual payload */
>  # endif
>      void *base_;                 /* First byte of allocated space. */
> --
> 1.9.1
>