On Wed, Aug 24, 2016 at 11:10 AM, Oriol Arcas <or...@starflownetworks.com> wrote:
> Regarding the last question pointed out by Bill, max_queue_num = > tm_system->next_queue_num, which is always "current queues in the TM + 1". > > So the loop, as it is now, visits all the elements in the array. > Thanks. That point wasn't obvious just by looking at the patch. > > -- > Oriol Arcas > Software Engineer > Starflow Networks > > On Wed, Aug 24, 2016 at 2:11 PM, Bill Fischofer <bill.fischo...@linaro.org > > wrote: > >> On Tue, Aug 23, 2016 at 4:51 PM, Barry Spinney <spin...@mellanox.com> >> wrote: >> >> > Both odp_pkt_queue.c and odp_traffic_mngr.c has similar bugs where >> > an array was malloc'd but the valid indicies run from 1 to the number >> > of entries malloc'd. This is because queue_num == 0 is used as a >> special >> > value indicating an invalid queue_num. The value 0 is used for this >> > same special purpose throughout the traffic mngr. >> > The fix involved using "queue_num - 1" as the array index, but only >> > after checking that queue_num is not 0. Also in a few cases a common >> > function used to get and check the queue_num was used in a few more >> > cases. >> > >> >> If the root issue is that 0 is being used as the invalid indicator, >> wouldn't another (and potentially simpler solution, as I believe Maxim >> suggested) to simply change the invalid distinguished value to 0xFFFFFFFF >> ? That's what linux-generic does for most of its ODP_xxx_INVALID symbols >> (and for this same reason to avoid indexing confusion). >> >> >> > >> > Signed-off-by: Barry Spinney <spin...@mellanox.com> >> > --- >> > platform/linux-generic/odp_pkt_queue.c | 12 ++++++------ >> > platform/linux-generic/odp_traffic_mngr.c | 26 >> +++++++++++--------------- >> > 2 files changed, 17 insertions(+), 21 deletions(-) >> > >> > diff --git a/platform/linux-generic/odp_pkt_queue.c >> > b/platform/linux-generic/odp_pkt_queue.c >> > index 7734ee9..7c6cd87 100644 >> > --- a/platform/linux-generic/odp_pkt_queue.c >> > +++ b/platform/linux-generic/odp_pkt_queue.c >> > @@ -92,7 +92,7 @@ static int pkt_queue_free_list_add(queue_pool_t >> *pool, >> > queue_blks_t *queue_blks; >> > queue_blk_t *queue_blk; >> > uint32_t which_region, blks_added, num_blks, start_idx; >> > - uint32_t malloc_len, blks_to_add, cnt; >> > + uint32_t malloc_len, blks_to_add, cnt, i; >> > >> > which_region = pool->current_region; >> > blks_added = 0; >> > @@ -102,7 +102,6 @@ static int pkt_queue_free_list_add(queue_pool_t >> *pool, >> > num_blks = region_desc->num_blks; >> > queue_blks = region_desc->queue_blks; >> > if (!queue_blks) { >> > - uint32_t i; >> > malloc_len = num_blks * sizeof(queue_blk_t); >> > queue_blks = malloc(malloc_len); >> > >> > @@ -263,13 +262,13 @@ int _odp_pkt_queue_append(_odp_int_queue_pool_t >> > queue_pool, >> > return -3; >> > >> > pool->total_pkt_appends++; >> > - first_blk_idx = pool->queue_num_tbl[queue_num]; >> > + first_blk_idx = pool->queue_num_tbl[queue_num - 1]; >> > if (first_blk_idx == 0) { >> > first_blk = queue_blk_alloc(pool, &first_blk_idx); >> > if (!first_blk) >> > return -1; >> > >> > - pool->queue_num_tbl[queue_num] = first_blk_idx; >> > + pool->queue_num_tbl[queue_num - 1] = first_blk_idx; >> > init_queue_blk(first_blk); >> > first_blk->pkts[0] = pkt; >> > return 0; >> > @@ -316,7 +315,7 @@ int _odp_pkt_queue_remove(_odp_int_queue_pool_t >> > queue_pool, >> > if ((queue_num == 0) || (pool->max_queue_num < queue_num)) >> > return -2; >> > >> > - first_blk_idx = pool->queue_num_tbl[queue_num]; >> > + first_blk_idx = pool->queue_num_tbl[queue_num - 1]; >> > if (first_blk_idx == 0) >> > return 0; /* pkt queue is empty. */ >> > >> > @@ -344,7 +343,8 @@ int _odp_pkt_queue_remove(_odp_int_queue_pool_t >> > queue_pool, >> > >> > first_blk->tail_queue_blk_idx; >> > } >> > >> > - pool->queue_num_tbl[queue_num] = >> > next_blk_idx; >> > + pool->queue_num_tbl[queue_num - 1] = >> > + next_blk_idx; >> > queue_blk_free(pool, first_blk, >> > first_blk_idx); >> > } >> > >> > diff --git a/platform/linux-generic/odp_traffic_mngr.c >> > b/platform/linux-generic/odp_traffic_mngr.c >> > index a5271ed..4fe07ef 100644 >> > --- a/platform/linux-generic/odp_traffic_mngr.c >> > +++ b/platform/linux-generic/odp_traffic_mngr.c >> > @@ -109,7 +109,7 @@ static tm_queue_obj_t *get_tm_queue_obj(tm_system_t >> > *tm_system, >> > return NULL; >> > >> > queue_num = pkt_desc->queue_num; >> > - tm_queue_obj = tm_system->queue_num_tbl[queue_num]; >> > + tm_queue_obj = tm_system->queue_num_tbl[queue_num - 1]; >> > return tm_queue_obj; >> > } >> > >> > @@ -1596,11 +1596,10 @@ static odp_bool_t tm_consume_sent_pkt(tm_system_ >> t >> > *tm_system, >> > tm_queue_obj_t *tm_queue_obj; >> > odp_packet_t pkt; >> > pkt_desc_t *new_pkt_desc; >> > - uint32_t queue_num, pkt_len; >> > + uint32_t pkt_len; >> > int rc; >> > >> > - queue_num = sent_pkt_desc->queue_num; >> > - tm_queue_obj = tm_system->queue_num_tbl[queue_num]; >> > + tm_queue_obj = get_tm_queue_obj(tm_system, sent_pkt_desc); >> > if (!tm_queue_obj) >> > return false; >> > >> > @@ -2088,15 +2087,11 @@ static void tm_send_pkt(tm_system_t *tm_system, >> > uint32_t max_sends) >> > tm_queue_obj_t *tm_queue_obj; >> > odp_packet_t odp_pkt; >> > pkt_desc_t *pkt_desc; >> > - uint32_t cnt, queue_num; >> > + uint32_t cnt; >> > >> > for (cnt = 1; cnt <= max_sends; cnt++) { >> > pkt_desc = &tm_system->egress_pkt_desc; >> > - queue_num = pkt_desc->queue_num; >> > - if (queue_num == 0) >> > - return; >> > - >> > - tm_queue_obj = tm_system->queue_num_tbl[queue_num]; >> > + tm_queue_obj = get_tm_queue_obj(tm_system, pkt_desc); >> > if (!tm_queue_obj) >> > return; >> > >> > @@ -2149,7 +2144,8 @@ static int tm_process_input_work_queue(tm >> _system_t >> > *tm_system, >> > return rc; >> > } >> > >> > - tm_queue_obj = tm_system->queue_num_tbl[work_ >> > item.queue_num]; >> > + tm_queue_obj = >> > + tm_system->queue_num_tbl[work_item.queue_num - >> 1]; >> > pkt = work_item.pkt; >> > if (!tm_queue_obj) { >> > odp_packet_free(pkt); >> > @@ -2204,7 +2200,7 @@ static int tm_process_expired_timers(tm_system_t >> > *tm_system, >> > >> > queue_num = (timer_context & 0xFFFFFFFF) >> 4; >> > timer_seq = timer_context >> 32; >> > - tm_queue_obj = tm_system->queue_num_tbl[queue_num]; >> > + tm_queue_obj = tm_system->queue_num_tbl[queue_num - 1]; >> > if (!tm_queue_obj) >> > return work_done; >> > >> > @@ -3904,7 +3900,7 @@ odp_tm_queue_t odp_tm_queue_create(odp_tm_t >> odp_tm, >> > tm_queue_obj->tm_qentry.s.enqueue = queue_tm_reenq; >> > tm_queue_obj->tm_qentry.s.enqueue_multi = queue_tm_reenq_multi; >> > >> > - tm_system->queue_num_tbl[tm_queue_obj->queue_num] = >> tm_queue_obj; >> > + tm_system->queue_num_tbl[tm_queue_obj->queue_num - 1] = >> > tm_queue_obj; >> > odp_ticketlock_lock(&tm_system->tm_system_lock); >> > if (params->shaper_profile != ODP_TM_INVALID) >> > tm_shaper_config_set(tm_system, params->shaper_profile, >> > @@ -3972,7 +3968,7 @@ int odp_tm_queue_destroy(odp_tm_queue_t tm_queue) >> > >> > /* Now that all of the checks are done, time to so some >> freeing. */ >> > odp_ticketlock_lock(&tm_system->tm_system_lock); >> > - tm_system->queue_num_tbl[tm_queue_obj->queue_num] = NULL; >> > + tm_system->queue_num_tbl[tm_queue_obj->queue_num - 1] = NULL; >> > >> > /* First delete any associated tm_wred_node and then the >> > tm_queue_obj >> > * itself */ >> > @@ -4634,7 +4630,7 @@ void odp_tm_stats_print(odp_tm_t odp_tm) >> > >> > max_queue_num = tm_system->next_queue_num; >> > for (queue_num = 1; queue_num < max_queue_num; queue_num++) { >> >> - tm_queue_obj = tm_system->queue_num_tbl[queue_num]; >> > + tm_queue_obj = tm_system->queue_num_tbl[queue_num - 1]; >> > >> >> This loop runs from 1 to max_queue_num - 1, but with this change doesn't >> this mean that it's now running from 0 to max_queue_num - 2? Can the last >> element of the array [max_queue_num - 1] ever be used? Shouldn't the test >> be changed to queue_num <= max_queue_num? >> >> >> > if (tm_queue_obj && tm_queue_obj->pkts_rcvd_cnt != 0) >> > ODP_DBG("queue_num=%u priority=%u rcvd=%u >> > enqueued=%u " >> > "dequeued=%u consumed=%u\n", >> > -- >> > 2.1.2 >> > >> > >> > >