On Mon, Jan 5, 2015 at 1:19 PM, Zoltan Kiss <zoltan.k...@linaro.org> wrote:
> This is only compile tested, and there are a few questions in the patch.
>
> Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
> ---
> diff --git a/lib/netdev-odp.c b/lib/netdev-odp.c
> index 96126a1..571b66e 100644
> --- a/lib/netdev-odp.c
> +++ b/lib/netdev-odp.c
> @@ -45,7 +45,7 @@
> #include "unaligned.h"
> #include "timeval.h"
> #include "unixctl.h"
> -#include "vlog.h"
> +#include "openvswitch/vlog.h"
>
> VLOG_DEFINE_THIS_MODULE(odp);
>
> @@ -94,7 +94,7 @@ struct netdev_rxq_odp {
> void
> free_odp_buf(struct ofpbuf *b)
> {
> - odph_packet_free(b->odp_pkt);
> + odp_packet_free(b->odp_pkt);
> odp_buffer_free(b->odp_ofpbuf);
> }
>
> @@ -129,26 +129,26 @@ odp_init(int argc, char *argv[])
> static int
> odp_class_init(void)
> {
> - void *pool_base;
> odp_shm_t shm;
> + odp_buffer_pool_param_t params;
> int result = 0;
>
> /* create packet pool */
> shm = odp_shm_reserve("shm_packet_pool", SHM_PKT_POOL_SIZE,
> ODP_CACHE_LINE_SIZE, 0);
> - pool_base = odp_shm_addr(shm);
>
> - if (odp_unlikely(pool_base == NULL)) {
> + if (odp_unlikely(shm == ODP_SHM_INVALID)) {
> VLOG_ERR("Error: ODP packet pool mem alloc failed\n");
> out_of_memory();
> return -1;
> }
>
> - pool = odp_buffer_pool_create("packet_pool", pool_base,
> - SHM_PKT_POOL_SIZE,
> - SHM_PKT_POOL_BUF_SIZE,
> - ODP_CACHE_LINE_SIZE,
> - ODP_BUFFER_TYPE_PACKET);
> + params.buf_size = SHM_PKT_POOL_BUF_SIZE;
> + params.buf_align = ODP_CACHE_LINE_SIZE;
> + params.num_bufs = SHM_PKT_POOL_SIZE / SHM_PKT_POOL_BUF_SIZE;
> + params.buf_type = ODP_BUFFER_TYPE_PACKET;
> +
> + pool = odp_buffer_pool_create("packet_pool", shm, ¶ms);
>
>
You really shouldn't be attempting to allocate your own SHM here. Instead,
delete the odp_shm_reserve() calls and pass ODP_SHM_NULL as the second
parameter to odp_buffer_pool_create() and ODP will allocate the memory for
you. There is no architected way to determine how large a SHM you need to
allocate to successfully do it the other way, so this provision of the API
is of somewhat questionable utility.
> if (pool == ODP_BUFFER_POOL_INVALID) {
> VLOG_ERR("Error: packet pool create failed.\n");
> @@ -159,19 +159,18 @@ odp_class_init(void)
> /* create ofpbuf pool */
> shm = odp_shm_reserve("shm_ofpbuf_pool", SHM_OFPBUF_POOL_SIZE,
> ODP_CACHE_LINE_SIZE, 0);
> - pool_base = odp_shm_addr(shm);
>
> - if (odp_unlikely(pool_base == NULL)) {
> + if (odp_unlikely(shm == ODP_SHM_INVALID)) {
> VLOG_ERR("Error: ODP packet pool mem alloc failed\n");
> out_of_memory();
> return -1;
> }
>
> - ofpbuf_pool = odp_buffer_pool_create("ofpbuf_pool", pool_base,
> - SHM_OFPBUF_POOL_SIZE,
> - SHM_OFPBUF_POOL_BUF_SIZE,
> - ODP_CACHE_LINE_SIZE,
> - ODP_BUFFER_TYPE_RAW);
> + params.buf_size = SHM_OFPBUF_POOL_BUF_SIZE;
> + params.num_bufs = SHM_OFPBUF_POOL_SIZE / SHM_OFPBUF_POOL_BUF_SIZE;
> + params.buf_type = ODP_BUFFER_TYPE_RAW;
> +
> + ofpbuf_pool = odp_buffer_pool_create("ofpbuf_pool", shm, ¶ms);
>
Same comment here. Let ODP allocate the memory.
>
> if (ofpbuf_pool == ODP_BUFFER_POOL_INVALID) {
> VLOG_ERR("Error: ofpbuf pool create failed.\n");
> @@ -182,19 +181,17 @@ odp_class_init(void)
> /* create pool for structures */
> shm = odp_shm_reserve("shm_struct_pool", SHM_STRUCT_POOL_SIZE,
> ODP_CACHE_LINE_SIZE, 0);
> - pool_base = odp_shm_addr(shm);
>
> - if (odp_unlikely(pool_base == NULL)) {
> + if (odp_unlikely(shm == ODP_SHM_INVALID)) {
> VLOG_ERR("Error: ODP packet pool mem alloc failed\n");
> out_of_memory();
> return -1;
> }
>
> - struct_pool = odp_buffer_pool_create("struct_pool", pool_base,
> - SHM_STRUCT_POOL_SIZE,
> - SHM_STRUCT_POOL_BUF_SIZE,
> - ODP_CACHE_LINE_SIZE,
> - ODP_BUFFER_TYPE_RAW);
> + params.buf_size = SHM_STRUCT_POOL_BUF_SIZE;
> + params.num_bufs = SHM_STRUCT_POOL_SIZE / SHM_STRUCT_POOL_BUF_SIZE;
> +
> + struct_pool = odp_buffer_pool_create("struct_pool", shm, ¶ms);
>
And again here, same comment.
>
> if (struct_pool == ODP_BUFFER_POOL_INVALID) {
> VLOG_ERR("Error: packet pool create failed.\n");
> @@ -247,15 +244,15 @@ netdev_odp_construct(struct netdev *netdev_)
> }
>
> netdev->pkt_pool = pool;
> - pkt = odph_packet_alloc(netdev->pkt_pool);
> - if (!odph_packet_is_valid(pkt)) {
> + pkt = odp_packet_alloc(netdev->pkt_pool, 0);
> + if (!odp_packet_is_valid(pkt)) {
> out_of_memory();
> goto out_err;
> }
>
It's more efficient to say if (pkt == ODP_PACKET_INVALID) here for this
test.
>
> - netdev->max_frame_len = odph_packet_buf_size(pkt);
> + netdev->max_frame_len = odp_buffer_size(odp_packet_to_buffer(pkt));
>
Need more context as to what you're trying to determine here. If you want
to know what's the largest size packet you can receive that's
ODP_CONFIG_PACKET_BUF_LEN_MAX
> - odph_packet_free(pkt);
> + odp_packet_free(pkt);
>
> ovs_mutex_init(&netdev->mutex);
>
> @@ -304,7 +301,7 @@ static int drop_err_pkts(odp_packet_t pkt_tbl[],
> unsigned len)
> pkt = pkt_tbl[i];
>
> if (odp_unlikely(odp_packet_error(pkt))) {
> - odph_packet_free(pkt); /* Drop */
> + odp_packet_free(pkt); /* Drop */
> pkt_cnt--;
> } else if (odp_unlikely(i != j++)) {
> pkt_tbl[j-1] = pkt;
> @@ -334,19 +331,21 @@ clone_pkts(struct netdev_odp *dev, struct
> dpif_packet **pkts,
> dropped++;
> continue;
> }
> - pkt = odph_packet_alloc(dev->pkt_pool);
> + pkt = odp_packet_alloc(dev->pkt_pool, size);
>
> - if (OVS_UNLIKELY(!odph_packet_is_valid(pkt))) {
> + if (OVS_UNLIKELY(pkt == ODP_PACKET_INVALID)) {
> VLOG_WARN_RL(&rl, "Could not allocate packet");
> dropped += cnt -i;
> break;
> }
>
> - odp_packet_init(pkt);
> - odp_packet_set_l2_offset(pkt, 0);
> + odp_packet_reset(pkt, 0);
> + odp_packet_l2_offset_set(pkt, 0);
>
> - memcpy(odp_packet_l2(pkt), ofpbuf_data(&pkts[i]->ofpbuf), size);
> - odp_packet_parse(pkt, size, 0);
> + memcpy(odp_packet_l2_ptr(pkt, NULL),
> ofpbuf_data(&pkts[i]->ofpbuf),
> + size);
> + /* TODO: is this OK? */
>
You shouldn't use memcpy on packets. Instead, to set the length and copy
the packet this should be:
odp_packet_push_tail(pkt, size);
odp_packet_copydata_in(pkt, 0, size, ofpbuf_data(&pkts[i]->ofpbuf));
> + _odp_packet_parse(pkt);
>
This is an internal API and should not be called by an ODP app. It should
be an ODP API but Petri hasn't approved it as such, hence it can't be used.
>
> odp_pkts[newcnt] = pkt;
> newcnt++;
> @@ -386,7 +385,7 @@ netdev_odp_send(struct netdev *netdev, struct
> dpif_packet **pkts, int cnt,
> } else {
> for (i = 0; i < cnt; i++) {
> odp_pkts[i] = pkts[i]->ofpbuf.odp_pkt;
> - odph_packet_free(pkts[i]->ofpbuf.odp_ofpbuf);
> + odp_packet_free(pkts[i]->ofpbuf.odp_ofpbuf);
> }
> pkts_ok = cnt;
> }
> @@ -396,7 +395,7 @@ netdev_odp_send(struct netdev *netdev, struct
> dpif_packet **pkts, int cnt,
> ovs_mutex_lock(&dev->mutex);
> dev->stats.tx_packets += pkts_ok;
> for (i = 0; i < pkts_ok; i++) {
> - dev->stats.tx_bytes += odp_packet_get_len(odp_pkts[i]);
> + dev->stats.tx_bytes += odp_packet_len(odp_pkts[i]);
> }
> ovs_mutex_unlock(&dev->mutex);
>
> @@ -649,10 +648,12 @@ netdev_odp_rxq_recv(struct netdev_rxq *rxq_, struct
> dpif_packet **packets,
> out_of_memory();
> }
> packets[i] = (struct dpif_packet*) odp_buffer_addr(buf);
> - ofpbuf_init_odp(&packets[i]->ofpbuf,
> odph_packet_buf_size(pkt_tbl[i]));
> + ofpbuf_init_odp(&packets[i]->ofpbuf,
> + odp_buffer_size(odp_packet_to_buffer(pkt_tbl[i])));
> + /* ^^^ Is
> this OK?*/
>
I'm not sure what you're trying to do here to say what these calls should
be. Can you clarify?
> packets[i]->ofpbuf.odp_pkt = pkt_tbl[i];
> packets[i]->ofpbuf.odp_ofpbuf = buf;
> - rx_bytes += odp_packet_get_len(pkt_tbl[i]);
> + rx_bytes += odp_packet_len(pkt_tbl[i]);
> }
>
> *c = pkts_ok;
> diff --git a/lib/netdev-odp.h b/lib/netdev-odp.h
> index 9f521da..6162dd4 100644
> --- a/lib/netdev-odp.h
> +++ b/lib/netdev-odp.h
> @@ -9,7 +9,7 @@
> #include <odp.h>
> #include <odph_eth.h>
> #include <odph_ip.h>
> -#include <odph_packet.h>
> +#include <odp_packet.h>
>
> /* This function is not exported, we need another way to deal with
> creating a packet from an ofpbuf */
> diff --git a/lib/ofpbuf.c b/lib/ofpbuf.c
> index d216917..764a799 100644
> --- a/lib/ofpbuf.c
> +++ b/lib/ofpbuf.c
> @@ -154,7 +154,7 @@ ofpbuf_uninit(struct ofpbuf *b)
> #endif
> } else if (b->source == OFPBUF_ODP) {
> #ifdef ODP_NETDEV
> - odph_packet_free(b->odp_pkt);
> + odp_packet_free(b->odp_pkt);
> odp_buffer_free(b->odp_ofpbuf);
> #else
> ovs_assert(b->source != OFPBUF_ODP);
> diff --git a/lib/ofpbuf.h b/lib/ofpbuf.h
> index 1c5166f..47cee29 100644
> --- a/lib/ofpbuf.h
> +++ b/lib/ofpbuf.h
> @@ -429,7 +429,7 @@ static inline void * ofpbuf_data(const struct ofpbuf
> *b)
> {
> #ifdef ODP_NETDEV
> if (b->source == OFPBUF_ODP)
> - return odp_packet_l2(b->odp_pkt);
> + return odp_packet_l2_ptr(b->odp_pkt, NULL);
> #endif
>
> return b->data_;
> @@ -439,8 +439,7 @@ static inline void ofpbuf_set_data(struct ofpbuf *b,
> void *d)
> {
> #ifdef ODP_NETDEV
> if (b->source == OFPBUF_ODP) {
> - ODP_ERR("ODP: Invalid use of ofpbuf_set_data\n");
> - ovs_abort(0, "Invalid function call\n");
> + ovs_abort(0, "ODP: Invalid use of ofpbuf_set_data\n");
> }
> #endif
>
> @@ -451,8 +450,7 @@ static inline void * ofpbuf_base(const struct ofpbuf
> *b)
> {
> #ifdef ODP_NETDEV
> if (b->source == OFPBUF_ODP) {
> - ODP_ERR("ODP: Invalid use of ofpbuf_base\n");
> - ovs_abort(0, "Invalid function call\n");
> + ovs_abort(0, "ODP: Invalid use of ofpbuf_base\n");
> }
> #endif
>
> @@ -463,8 +461,7 @@ static inline void ofpbuf_set_base(struct ofpbuf *b,
> void *d)
> {
> #ifdef ODP_NETDEV
> if (b->source == OFPBUF_ODP) {
> - ODP_ERR("ODP: Invalid use of ofpbuf_set_base\n");
> - ovs_abort(0, "Invalid function call\n");
> + ovs_abort(0, "ODP: Invalid use of ofpbuf_set_base\n\n");
> }
> #endif
>
> @@ -475,7 +472,7 @@ static inline uint32_t ofpbuf_size(const struct ofpbuf
> *b)
> {
> #ifdef ODP_NETDEV
> if (b->source == OFPBUF_ODP)
> - return odp_packet_get_len(b->odp_pkt);
> + return odp_packet_len(b->odp_pkt);
> #endif
>
> return b->size_;
> @@ -485,8 +482,7 @@ static inline void ofpbuf_set_size(struct ofpbuf *b,
> uint32_t v)
> {
> #ifdef ODP_NETDEV
> if (b->source == OFPBUF_ODP) {
> - ODP_ERR("ODP: Invalid use of ofpbuf_set_size\n");
> - ovs_abort(0, "Invalid function call\n");
> + ovs_abort(0, "ODP: Invalid use of ofpbuf_set_size\n");
> }
> #endif
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
_______________________________________________
lng-odp mailing list
lng-odp@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/lng-odp
