On Wed, Jan 7, 2015 at 4:02 PM, Ciprian Barbu <ciprian.ba...@linaro.org> wrote: > 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.
So the thing Mike had troubles with, might have been caused by configuring ovs --with-odp=<odp_dir> whereas it is supposed to be the directory where ODP was installed to. I usually configure ODP like this: ./configure --enable-shared=no --prefix=$PWD/install make make install and then point OVS to the install directory. I think I will send a patch with these instructions to make things clear. > >> >> >> 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
