On Wed, Jan 7, 2015 at 12:28 AM, Bill Fischofer <bill.fischo...@linaro.org> wrote: > > > 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.
The line in question is inside clone_pkts, which is on the TX path, it's called when it's needed to transmit a packet that is not an ODP buffer (from a tap port for example). I don't remember exactly why I was parsing the packet at that point, but it doesn't seem to make sense to keep it now. The odp-generator for example doesn't parse anything on the TX side. All other things needed seem to have been covered by Bill's comments, can you send version 2 now? Also can you did the deb packages build for, you, Mike told me he had some problems with that. Regular build worked though. > > > 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 > _______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org http://lists.linaro.org/mailman/listinfo/lng-odp