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