On Tue, Jan 6, 2015 at 12:27 PM, Zoltan Kiss <zoltan.k...@linaro.org> wrote:
> > > 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, ¶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. >> > > 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. > > Yes, odp_buffer_pool_info() will return the buf_size associated with the pool, which will tell you that. Note to Petri: Please note that applications need to know the packet size associated with a buffer pool, not the packet segment size. If you want to separately specify packet segment size on the odp_buffer_pool_param_t, we still need the packet size as well. > > > @@ -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. OK. > > > > >> @@ -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. > odp_packet_buf_len(pkt) tells you that info. > > Zoli >
_______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org http://lists.linaro.org/mailman/listinfo/lng-odp
