On 05/01/15 20:26, Bill Fischofer wrote:


On Mon, Jan 5, 2015 at 1:19 PM, Zoltan Kiss <zoltan.k...@linaro.org
<mailto: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
    <mailto: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

    @@ -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, &params);

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.

Ok, I'll use it that way.



          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

This was written by Ciprian, but as far as I saw the only reason to allocate this packet is to determine the maximum packet size for this particular pool, not absolute maximum.
odp_buffer_pool_info is better for this goal.


    @@ -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.
If this came in on an ODP interface, then we don't need to parse it as Ciprian wrote in an another mail, because it was parsed during reception. But if it came from another kind of port (I'm not sure that's possible at the moment, but I think we will need that), we need to parse here.



    @@ -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?
I need to determine the allocated buffer size from a odp_packet_t. I don't know if there is a helper for that, I haven't found it, but maybe I missed it.

Zoli

_______________________________________________
lng-odp mailing list
lng-odp@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/lng-odp

Reply via email to