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.
--
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
> >
> >
>
