[PATCHv7] linux-generic: fix odp_pktio_inq_remdef

Correctly remove queue from packet i/o and remove it from scheduler.

Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
---
 .../linux-generic/include/odp_queue_internal.h     | 11 ++++++++
 platform/linux-generic/odp_packet_io.c             | 29 +++++++++++++++++++++-
 platform/linux-generic/odp_schedule.c              |  7 +++++-
 3 files changed, 45 insertions(+), 2 deletions(-)

ping.

Maxim.

On 12/25/2014 05:25 PM, Maxim Uvarov wrote:
> Correctly remove queue from packet i/o and remove it from scheduler.
>
> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> ---
>   .../linux-generic/include/odp_queue_internal.h     | 11 ++++++++
>   platform/linux-generic/odp_packet_io.c             | 29 +++++++++++++++++++++-
>   platform/linux-generic/odp_schedule.c              |  7 +++++-
>   3 files changed, 45 insertions(+), 2 deletions(-)
>
> diff --git a/platform/linux-generic/include/odp_queue_internal.h b/platform/linux-generic/include/odp_queue_internal.h
> index d5c8e4e..16734ee 100644
> --- a/platform/linux-generic/include/odp_queue_internal.h
> +++ b/platform/linux-generic/include/odp_queue_internal.h
> @@ -129,6 +129,17 @@ static inline int queue_is_destroyed(odp_queue_t handle)
>   
>   	return queue->s.status == QUEUE_STATUS_DESTROYED;
>   }
> +
> +static inline int queue_is_sched(odp_queue_t handle)
> +{
> +	queue_entry_t *queue;
> +
> +	queue = queue_to_qentry(handle);
> +
> +	return ((queue->s.status == QUEUE_STATUS_DESTROYED) ||
> +		(queue->s.status == QUEUE_STATUS_NOTSCHED) ||
> +		 queue->s.pktin == ODP_PKTIO_INVALID) ? 0 : 1;
> +}
>   #ifdef __cplusplus
>   }
>   #endif
> diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c
> index 59590d2..dd069aa 100644
> --- a/platform/linux-generic/odp_packet_io.c
> +++ b/platform/linux-generic/odp_packet_io.c
> @@ -381,7 +381,34 @@ int odp_pktio_inq_setdef(odp_pktio_t id, odp_queue_t queue)
>   
>   int odp_pktio_inq_remdef(odp_pktio_t id)
>   {
> -	return odp_pktio_inq_setdef(id, ODP_QUEUE_INVALID);
> +	pktio_entry_t *pktio_entry = get_pktio_entry(id);
> +	odp_queue_t queue;
> +	queue_entry_t *qentry;
> +
> +	if (pktio_entry == NULL)
> +		return -1;
> +
> +	lock_entry(pktio_entry);
> +	queue = pktio_entry->s.inq_default;
> +	qentry = queue_to_qentry(queue);
> +
> +	queue_lock(qentry);
> +	if (qentry->s.status == QUEUE_STATUS_FREE) {
> +		queue_unlock(qentry);
> +		unlock_entry(pktio_entry);
> +		return -1;
> +	}
> +
> +	qentry->s.enqueue = queue_enq_dummy;
> +	qentry->s.enqueue_multi = queue_enq_multi_dummy;
> +	qentry->s.status = QUEUE_STATUS_NOTSCHED;
> +	qentry->s.pktin = ODP_PKTIO_INVALID;
> +	queue_unlock(qentry);
> +
> +	pktio_entry->s.inq_default = ODP_QUEUE_INVALID;
> +	unlock_entry(pktio_entry);
> +
> +	return 0;
>   }
>   
>   odp_queue_t odp_pktio_inq_getdef(odp_pktio_t id)
> diff --git a/platform/linux-generic/odp_schedule.c b/platform/linux-generic/odp_schedule.c
> index a14de4f..365dc98 100644
> --- a/platform/linux-generic/odp_schedule.c
> +++ b/platform/linux-generic/odp_schedule.c
> @@ -286,6 +286,11 @@ static int schedule(odp_queue_t *out_queue, odp_buffer_t out_buf[],
>   				desc  = odp_buffer_addr(desc_buf);
>   				queue = desc->queue;
>   
> +				if (odp_queue_type(queue) ==
> +					ODP_QUEUE_TYPE_PKTIN &&
> +					!queue_is_sched(queue))
> +					continue;
> +
>   				num = odp_queue_deq_multi(queue,
>   							  sched_local.buf,
>   							  max_deq);
> @@ -296,7 +301,7 @@ static int schedule(odp_queue_t *out_queue, odp_buffer_t out_buf[],
>   					 */
>   					if (odp_queue_type(queue) ==
>   					    ODP_QUEUE_TYPE_PKTIN &&
> -					    !queue_is_destroyed(queue))
> +					    queue_is_sched(queue))
>   						odp_queue_enq(pri_q, desc_buf);
>   
>   					continue;

On 01/12/2015 06:07 PM, Stuart Haslam wrote:
> On Thu, Dec 25, 2014 at 02:25:29PM +0000, Maxim Uvarov wrote:
>> Correctly remove queue from packet i/o and remove it from scheduler.
>>
>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
>> ---
>>   .../linux-generic/include/odp_queue_internal.h     | 11 ++++++++
>>   platform/linux-generic/odp_packet_io.c             | 29 +++++++++++++++++++++-
>>   platform/linux-generic/odp_schedule.c              |  7 +++++-
>>   3 files changed, 45 insertions(+), 2 deletions(-)
>>
>> diff --git a/platform/linux-generic/include/odp_queue_internal.h b/platform/linux-generic/include/odp_queue_internal.h
>> index d5c8e4e..16734ee 100644
>> --- a/platform/linux-generic/include/odp_queue_internal.h
>> +++ b/platform/linux-generic/include/odp_queue_internal.h
>> @@ -129,6 +129,17 @@ static inline int queue_is_destroyed(odp_queue_t handle)
>>   
>>   	return queue->s.status == QUEUE_STATUS_DESTROYED;
>>   }
>> +
>> +static inline int queue_is_sched(odp_queue_t handle)
>> +{
>> +	queue_entry_t *queue;
>> +
>> +	queue = queue_to_qentry(handle);
>> +
>> +	return ((queue->s.status == QUEUE_STATUS_DESTROYED) ||
>> +		(queue->s.status == QUEUE_STATUS_NOTSCHED) ||
>> +		 queue->s.pktin == ODP_PKTIO_INVALID) ? 0 : 1;
> This logic isn't right, it'll return 0 for any queue that has an invalid
> pktin, and return 1 for QUEUE_STATUS_FREE and QUEUE_STATUS_READY queues.
> I think you get away with it for now because you check the queue type
> before calling this function, but perhaps it should be;
>
> queue->s.status == QUEUE_STATUS_SCHED || queue->s.pktin != ODP_PKTIO_INVALID

in v8 I used &&. Because queue_is_sched if STATUS_SCHED AND pktion != 
INVALID, not ||.

Maxim.


>
>> +}
>>   #ifdef __cplusplus
>>   }
>>   #endif
>> diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c
>> index 59590d2..dd069aa 100644
>> --- a/platform/linux-generic/odp_packet_io.c
>> +++ b/platform/linux-generic/odp_packet_io.c
>> @@ -381,7 +381,34 @@ int odp_pktio_inq_setdef(odp_pktio_t id, odp_queue_t queue)
>>   
>>   int odp_pktio_inq_remdef(odp_pktio_t id)
>>   {
>> -	return odp_pktio_inq_setdef(id, ODP_QUEUE_INVALID);
>> +	pktio_entry_t *pktio_entry = get_pktio_entry(id);
>> +	odp_queue_t queue;
>> +	queue_entry_t *qentry;
>> +
>> +	if (pktio_entry == NULL)
>> +		return -1;
>> +
>> +	lock_entry(pktio_entry);
>> +	queue = pktio_entry->s.inq_default;
>> +	qentry = queue_to_qentry(queue);
>> +
>> +	queue_lock(qentry);
>> +	if (qentry->s.status == QUEUE_STATUS_FREE) {
>> +		queue_unlock(qentry);
>> +		unlock_entry(pktio_entry);
>> +		return -1;
>> +	}
>> +
>> +	qentry->s.enqueue = queue_enq_dummy;
>> +	qentry->s.enqueue_multi = queue_enq_multi_dummy;
>> +	qentry->s.status = QUEUE_STATUS_NOTSCHED;
>> +	qentry->s.pktin = ODP_PKTIO_INVALID;
>> +	queue_unlock(qentry);
>> +
>> +	pktio_entry->s.inq_default = ODP_QUEUE_INVALID;
>> +	unlock_entry(pktio_entry);
>> +
>> +	return 0;
>>   }
> This looks OK.
>
>>   
>>   odp_queue_t odp_pktio_inq_getdef(odp_pktio_t id)
>> diff --git a/platform/linux-generic/odp_schedule.c b/platform/linux-generic/odp_schedule.c
>> index a14de4f..365dc98 100644
>> --- a/platform/linux-generic/odp_schedule.c
>> +++ b/platform/linux-generic/odp_schedule.c
>> @@ -286,6 +286,11 @@ static int schedule(odp_queue_t *out_queue, odp_buffer_t out_buf[],
>>   				desc  = odp_buffer_addr(desc_buf);
>>   				queue = desc->queue;
>>   
>> +				if (odp_queue_type(queue) ==
>> +					ODP_QUEUE_TYPE_PKTIN &&
>> +					!queue_is_sched(queue))
>> +					continue;
>> +
> I don't fully understand why this hunk above is required, or at least
> why both this and the change below are. And what happens if remdef is
> called in another thread between these two queue_is_sched() checks?
>
>>   				num = odp_queue_deq_multi(queue,
>>   							  sched_local.buf,
>>   							  max_deq);
>> @@ -296,7 +301,7 @@ static int schedule(odp_queue_t *out_queue, odp_buffer_t out_buf[],
>>   					 */
>>   					if (odp_queue_type(queue) ==
>>   					    ODP_QUEUE_TYPE_PKTIN &&
>> -					    !queue_is_destroyed(queue))
>> +					    queue_is_sched(queue))
>>   						odp_queue_enq(pri_q, desc_buf);
>>   
>>   					continue;
>> -- 
>> 1.8.5.1.163.gd7aced9
>>
> --
> Stuart.
>