diff mbox

queue: fix memory corruption in reorder_enq()

Message ID 1449509630-30932-1-git-send-email-zoltan.kiss@linaro.org
State Accepted
Commit 994d8d029fc804b7d5d22c503abb5049d70c3d86
Headers show

Commit Message

Zoltan Kiss Dec. 7, 2015, 5:33 p.m. UTC
reorder_prev is set to the address of the pointer origin_qe->s.reorder_head,
which is wrong. If the linked list was empty, that won't be corrected, and
reorder_prev->next points to the adjacent queue entry's status field. If that
entry is used, that queue's metadata will be corrupted.
This was found by running the chaos scheduler test with ODP-DPDK.

Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org>
---

Comments

Zoltan Kiss Dec. 7, 2015, 5:36 p.m. UTC | #1
Maxim, Bill, please check this ASAP, and if feasible commit it to 
master, it blocks ODP-DPDK upgrade to 1.5

Zoli

On 07/12/15 17:33, Zoltan Kiss wrote:
> reorder_prev is set to the address of the pointer origin_qe->s.reorder_head,
> which is wrong. If the linked list was empty, that won't be corrected, and
> reorder_prev->next points to the adjacent queue entry's status field. If that
> entry is used, that queue's metadata will be corrupted.
> This was found by running the chaos scheduler test with ODP-DPDK.
>
> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org>
> ---
> diff --git a/platform/linux-generic/include/odp_queue_internal.h b/platform/linux-generic/include/odp_queue_internal.h
> index a70044b..1cc0ed2 100644
> --- a/platform/linux-generic/include/odp_queue_internal.h
> +++ b/platform/linux-generic/include/odp_queue_internal.h
> @@ -212,8 +212,7 @@ static inline void reorder_enq(queue_entry_t *queue,
>   			       int sustain)
>   {
>   	odp_buffer_hdr_t *reorder_buf = origin_qe->s.reorder_head;
> -	odp_buffer_hdr_t *reorder_prev =
> -		(odp_buffer_hdr_t *)(void *)&origin_qe->s.reorder_head;
> +	odp_buffer_hdr_t *reorder_prev = NULL;
>
>   	while (reorder_buf && order >= reorder_buf->order) {
>   		reorder_prev = reorder_buf;
> @@ -221,7 +220,12 @@ static inline void reorder_enq(queue_entry_t *queue,
>   	}
>
>   	buf_hdr->next = reorder_buf;
> -	reorder_prev->next = buf_hdr;
> +
> +	if (reorder_prev)
> +		reorder_prev->next = buf_hdr;
> +	else
> +		origin_qe->s.reorder_head = buf_hdr;
> +
>
>   	if (!reorder_buf)
>   		origin_qe->s.reorder_tail = buf_hdr;
>
Bill Fischofer Dec. 7, 2015, 5:52 p.m. UTC | #2
You're seeing this because in linux-dpdk the next field of the
odp_buffer_hdr_t is not at offset 0.  In linux-generic next is at offset
zero so this code is slightly more efficient.  So this change really should
be in the linux-dpdk version of this code rather than linux-generic.

However the performance penalty is minor if you want to keep a common
linux-generic/linux-dpdk version of this file, so:

On Mon, Dec 7, 2015 at 11:36 AM, Zoltan Kiss <zoltan.kiss@linaro.org> wrote:

> Maxim, Bill, please check this ASAP, and if feasible commit it to master,

> it blocks ODP-DPDK upgrade to 1.5

>

> Zoli

>

>

> On 07/12/15 17:33, Zoltan Kiss wrote:

>

>> reorder_prev is set to the address of the pointer

>> origin_qe->s.reorder_head,

>> which is wrong. If the linked list was empty, that won't be corrected, and

>> reorder_prev->next points to the adjacent queue entry's status field. If

>> that

>> entry is used, that queue's metadata will be corrupted.

>> This was found by running the chaos scheduler test with ODP-DPDK.

>>

>> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org>

>>

>

Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org>



> ---

>> diff --git a/platform/linux-generic/include/odp_queue_internal.h

>> b/platform/linux-generic/include/odp_queue_internal.h

>> index a70044b..1cc0ed2 100644

>> --- a/platform/linux-generic/include/odp_queue_internal.h

>> +++ b/platform/linux-generic/include/odp_queue_internal.h

>> @@ -212,8 +212,7 @@ static inline void reorder_enq(queue_entry_t *queue,

>>                                int sustain)

>>   {

>>         odp_buffer_hdr_t *reorder_buf = origin_qe->s.reorder_head;

>> -       odp_buffer_hdr_t *reorder_prev =

>> -               (odp_buffer_hdr_t *)(void *)&origin_qe->s.reorder_head;

>> +       odp_buffer_hdr_t *reorder_prev = NULL;

>>

>>         while (reorder_buf && order >= reorder_buf->order) {

>>                 reorder_prev = reorder_buf;

>> @@ -221,7 +220,12 @@ static inline void reorder_enq(queue_entry_t *queue,

>>         }

>>

>>         buf_hdr->next = reorder_buf;

>> -       reorder_prev->next = buf_hdr;

>> +

>> +       if (reorder_prev)

>> +               reorder_prev->next = buf_hdr;

>> +       else

>> +               origin_qe->s.reorder_head = buf_hdr;

>> +

>>

>>         if (!reorder_buf)

>>                 origin_qe->s.reorder_tail = buf_hdr;

>>

>>
Bill Fischofer Dec. 7, 2015, 5:54 p.m. UTC | #3
BTW, this is why you see in the linux-generic version of
odp_buffer_internal.h the comment:

/* Common buffer header */
struct odp_buffer_hdr_t {
struct odp_buffer_hdr_t *next;       /* next buf in a list--keep 1st */

Since you've split this file for linux-dpdk to be consistent you should
also split include/odp_queue_internal.h as well so the code is isolated to
the platform that has this problem.

On Mon, Dec 7, 2015 at 11:52 AM, Bill Fischofer <bill.fischofer@linaro.org>
wrote:

> You're seeing this because in linux-dpdk the next field of the

> odp_buffer_hdr_t is not at offset 0.  In linux-generic next is at offset

> zero so this code is slightly more efficient.  So this change really should

> be in the linux-dpdk version of this code rather than linux-generic.

>

> However the performance penalty is minor if you want to keep a common

> linux-generic/linux-dpdk version of this file, so:

>

> On Mon, Dec 7, 2015 at 11:36 AM, Zoltan Kiss <zoltan.kiss@linaro.org>

> wrote:

>

>> Maxim, Bill, please check this ASAP, and if feasible commit it to master,

>> it blocks ODP-DPDK upgrade to 1.5

>>

>> Zoli

>>

>>

>> On 07/12/15 17:33, Zoltan Kiss wrote:

>>

>>> reorder_prev is set to the address of the pointer

>>> origin_qe->s.reorder_head,

>>> which is wrong. If the linked list was empty, that won't be corrected,

>>> and

>>> reorder_prev->next points to the adjacent queue entry's status field. If

>>> that

>>> entry is used, that queue's metadata will be corrupted.

>>> This was found by running the chaos scheduler test with ODP-DPDK.

>>>

>>> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org>

>>>

>>

> Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org>

>

>

>> ---

>>> diff --git a/platform/linux-generic/include/odp_queue_internal.h

>>> b/platform/linux-generic/include/odp_queue_internal.h

>>> index a70044b..1cc0ed2 100644

>>> --- a/platform/linux-generic/include/odp_queue_internal.h

>>> +++ b/platform/linux-generic/include/odp_queue_internal.h

>>> @@ -212,8 +212,7 @@ static inline void reorder_enq(queue_entry_t *queue,

>>>                                int sustain)

>>>   {

>>>         odp_buffer_hdr_t *reorder_buf = origin_qe->s.reorder_head;

>>> -       odp_buffer_hdr_t *reorder_prev =

>>> -               (odp_buffer_hdr_t *)(void *)&origin_qe->s.reorder_head;

>>> +       odp_buffer_hdr_t *reorder_prev = NULL;

>>>

>>>         while (reorder_buf && order >= reorder_buf->order) {

>>>                 reorder_prev = reorder_buf;

>>> @@ -221,7 +220,12 @@ static inline void reorder_enq(queue_entry_t *queue,

>>>         }

>>>

>>>         buf_hdr->next = reorder_buf;

>>> -       reorder_prev->next = buf_hdr;

>>> +

>>> +       if (reorder_prev)

>>> +               reorder_prev->next = buf_hdr;

>>> +       else

>>> +               origin_qe->s.reorder_head = buf_hdr;

>>> +

>>>

>>>         if (!reorder_buf)

>>>                 origin_qe->s.reorder_tail = buf_hdr;

>>>

>>>

>
Zoltan Kiss Dec. 7, 2015, 6:44 p.m. UTC | #4
On 07/12/15 17:52, Bill Fischofer wrote:
> You're seeing this because in linux-dpdk the next field of the
> odp_buffer_hdr_t is not at offset 0.  In linux-generic next is at offset
> zero so this code is slightly more efficient.  So this change really
> should be in the linux-dpdk version of this code rather than linux-generic.

I see. Well, it's very obscure.

Btw. I don't see why we have origin_qe->s.reorder_tail: we never use its 
value, and even if we would have the tail member of the linked list, it 
has little use as it is not double-linked.

>
> However the performance penalty is minor if you want to keep a common
> linux-generic/linux-dpdk version of this file, so:
>
> On Mon, Dec 7, 2015 at 11:36 AM, Zoltan Kiss <zoltan.kiss@linaro.org
> <mailto:zoltan.kiss@linaro.org>> wrote:
>
>     Maxim, Bill, please check this ASAP, and if feasible commit it to
>     master, it blocks ODP-DPDK upgrade to 1.5
>
>     Zoli
>
>
>     On 07/12/15 17:33, Zoltan Kiss wrote:
>
>         reorder_prev is set to the address of the pointer
>         origin_qe->s.reorder_head,
>         which is wrong. If the linked list was empty, that won't be
>         corrected, and
>         reorder_prev->next points to the adjacent queue entry's status
>         field. If that
>         entry is used, that queue's metadata will be corrupted.
>         This was found by running the chaos scheduler test with ODP-DPDK.
>
>         Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org
>         <mailto:zoltan.kiss@linaro.org>>
>
>
> Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org
> <mailto:bill.fischofer@linaro.org>>
>
>         ---
>         diff --git a/platform/linux-generic/include/odp_queue_internal.h
>         b/platform/linux-generic/include/odp_queue_internal.h
>         index a70044b..1cc0ed2 100644
>         --- a/platform/linux-generic/include/odp_queue_internal.h
>         +++ b/platform/linux-generic/include/odp_queue_internal.h
>         @@ -212,8 +212,7 @@ static inline void reorder_enq(queue_entry_t
>         *queue,
>                                         int sustain)
>            {
>                  odp_buffer_hdr_t *reorder_buf = origin_qe->s.reorder_head;
>         -       odp_buffer_hdr_t *reorder_prev =
>         -               (odp_buffer_hdr_t *)(void
>         *)&origin_qe->s.reorder_head;
>         +       odp_buffer_hdr_t *reorder_prev = NULL;
>
>                  while (reorder_buf && order >= reorder_buf->order) {
>                          reorder_prev = reorder_buf;
>         @@ -221,7 +220,12 @@ static inline void
>         reorder_enq(queue_entry_t *queue,
>                  }
>
>                  buf_hdr->next = reorder_buf;
>         -       reorder_prev->next = buf_hdr;
>         +
>         +       if (reorder_prev)
>         +               reorder_prev->next = buf_hdr;
>         +       else
>         +               origin_qe->s.reorder_head = buf_hdr;
>         +
>
>                  if (!reorder_buf)
>                          origin_qe->s.reorder_tail = buf_hdr;
>
>
Zoltan Kiss Dec. 7, 2015, 6:47 p.m. UTC | #5
On 07/12/15 17:54, Bill Fischofer wrote:
> BTW, this is why you see in the linux-generic version of
> odp_buffer_internal.h the comment:
>
> /* Common buffer header */
> struct odp_buffer_hdr_t {
> struct odp_buffer_hdr_t *next;       /* next buf in a list--keep 1st */
>
> Since you've split this file for linux-dpdk to be consistent you should
> also split include/odp_queue_internal.h as well so the code is isolated
> to the platform that has this problem.

I'll check upon that. If there are more places in the queuing code where 
it relies on this assumption, we should probably fork that code.

Regards,

Zoltan


>
> On Mon, Dec 7, 2015 at 11:52 AM, Bill Fischofer
> <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>> wrote:
>
>     You're seeing this because in linux-dpdk the next field of the
>     odp_buffer_hdr_t is not at offset 0.  In linux-generic next is at
>     offset zero so this code is slightly more efficient.  So this change
>     really should be in the linux-dpdk version of this code rather than
>     linux-generic.
>
>     However the performance penalty is minor if you want to keep a
>     common linux-generic/linux-dpdk version of this file, so:
>
>     On Mon, Dec 7, 2015 at 11:36 AM, Zoltan Kiss <zoltan.kiss@linaro.org
>     <mailto:zoltan.kiss@linaro.org>> wrote:
>
>         Maxim, Bill, please check this ASAP, and if feasible commit it
>         to master, it blocks ODP-DPDK upgrade to 1.5
>
>         Zoli
>
>
>         On 07/12/15 17:33, Zoltan Kiss wrote:
>
>             reorder_prev is set to the address of the pointer
>             origin_qe->s.reorder_head,
>             which is wrong. If the linked list was empty, that won't be
>             corrected, and
>             reorder_prev->next points to the adjacent queue entry's
>             status field. If that
>             entry is used, that queue's metadata will be corrupted.
>             This was found by running the chaos scheduler test with
>             ODP-DPDK.
>
>             Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org
>             <mailto:zoltan.kiss@linaro.org>>
>
>
>     Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org
>     <mailto:bill.fischofer@linaro.org>>
>
>             ---
>             diff --git
>             a/platform/linux-generic/include/odp_queue_internal.h
>             b/platform/linux-generic/include/odp_queue_internal.h
>             index a70044b..1cc0ed2 100644
>             --- a/platform/linux-generic/include/odp_queue_internal.h
>             +++ b/platform/linux-generic/include/odp_queue_internal.h
>             @@ -212,8 +212,7 @@ static inline void
>             reorder_enq(queue_entry_t *queue,
>                                             int sustain)
>                {
>                      odp_buffer_hdr_t *reorder_buf =
>             origin_qe->s.reorder_head;
>             -       odp_buffer_hdr_t *reorder_prev =
>             -               (odp_buffer_hdr_t *)(void
>             *)&origin_qe->s.reorder_head;
>             +       odp_buffer_hdr_t *reorder_prev = NULL;
>
>                      while (reorder_buf && order >= reorder_buf->order) {
>                              reorder_prev = reorder_buf;
>             @@ -221,7 +220,12 @@ static inline void
>             reorder_enq(queue_entry_t *queue,
>                      }
>
>                      buf_hdr->next = reorder_buf;
>             -       reorder_prev->next = buf_hdr;
>             +
>             +       if (reorder_prev)
>             +               reorder_prev->next = buf_hdr;
>             +       else
>             +               origin_qe->s.reorder_head = buf_hdr;
>             +
>
>                      if (!reorder_buf)
>                              origin_qe->s.reorder_tail = buf_hdr;
>
>
>
Bill Fischofer Dec. 8, 2015, 2:43 a.m. UTC | #6
At present reorder_enq() is the only routine that takes advantage of that
relationship.  As to your other question, reorder_tail exists because the
reorder queue is a queue, however elements on it are sorted by order rather
than arrival time. The tail is used because dequeues can dequeue multiple
elements at a time.

On Mon, Dec 7, 2015 at 12:47 PM, Zoltan Kiss <zoltan.kiss@linaro.org> wrote:

>

>

> On 07/12/15 17:54, Bill Fischofer wrote:

>

>> BTW, this is why you see in the linux-generic version of

>> odp_buffer_internal.h the comment:

>>

>> /* Common buffer header */

>> struct odp_buffer_hdr_t {

>> struct odp_buffer_hdr_t *next;       /* next buf in a list--keep 1st */

>>

>> Since you've split this file for linux-dpdk to be consistent you should

>> also split include/odp_queue_internal.h as well so the code is isolated

>> to the platform that has this problem.

>>

>

> I'll check upon that. If there are more places in the queuing code where

> it relies on this assumption, we should probably fork that code.

>

> Regards,

>

> Zoltan

>

>

>

>> On Mon, Dec 7, 2015 at 11:52 AM, Bill Fischofer

>> <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>> wrote:

>>

>>     You're seeing this because in linux-dpdk the next field of the

>>     odp_buffer_hdr_t is not at offset 0.  In linux-generic next is at

>>     offset zero so this code is slightly more efficient.  So this change

>>     really should be in the linux-dpdk version of this code rather than

>>     linux-generic.

>>

>>     However the performance penalty is minor if you want to keep a

>>     common linux-generic/linux-dpdk version of this file, so:

>>

>>     On Mon, Dec 7, 2015 at 11:36 AM, Zoltan Kiss <zoltan.kiss@linaro.org

>>     <mailto:zoltan.kiss@linaro.org>> wrote:

>>

>>         Maxim, Bill, please check this ASAP, and if feasible commit it

>>         to master, it blocks ODP-DPDK upgrade to 1.5

>>

>>         Zoli

>>

>>

>>         On 07/12/15 17:33, Zoltan Kiss wrote:

>>

>>             reorder_prev is set to the address of the pointer

>>             origin_qe->s.reorder_head,

>>             which is wrong. If the linked list was empty, that won't be

>>             corrected, and

>>             reorder_prev->next points to the adjacent queue entry's

>>             status field. If that

>>             entry is used, that queue's metadata will be corrupted.

>>             This was found by running the chaos scheduler test with

>>             ODP-DPDK.

>>

>>             Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org

>>             <mailto:zoltan.kiss@linaro.org>>

>>

>>

>>     Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org

>>     <mailto:bill.fischofer@linaro.org>>

>>

>>

>>             ---

>>             diff --git

>>             a/platform/linux-generic/include/odp_queue_internal.h

>>             b/platform/linux-generic/include/odp_queue_internal.h

>>             index a70044b..1cc0ed2 100644

>>             --- a/platform/linux-generic/include/odp_queue_internal.h

>>             +++ b/platform/linux-generic/include/odp_queue_internal.h

>>             @@ -212,8 +212,7 @@ static inline void

>>             reorder_enq(queue_entry_t *queue,

>>                                             int sustain)

>>                {

>>                      odp_buffer_hdr_t *reorder_buf =

>>             origin_qe->s.reorder_head;

>>             -       odp_buffer_hdr_t *reorder_prev =

>>             -               (odp_buffer_hdr_t *)(void

>>             *)&origin_qe->s.reorder_head;

>>             +       odp_buffer_hdr_t *reorder_prev = NULL;

>>

>>                      while (reorder_buf && order >= reorder_buf->order) {

>>                              reorder_prev = reorder_buf;

>>             @@ -221,7 +220,12 @@ static inline void

>>             reorder_enq(queue_entry_t *queue,

>>                      }

>>

>>                      buf_hdr->next = reorder_buf;

>>             -       reorder_prev->next = buf_hdr;

>>             +

>>             +       if (reorder_prev)

>>             +               reorder_prev->next = buf_hdr;

>>             +       else

>>             +               origin_qe->s.reorder_head = buf_hdr;

>>             +

>>

>>                      if (!reorder_buf)

>>                              origin_qe->s.reorder_tail = buf_hdr;

>>

>>

>>

>>
Maxim Uvarov Dec. 8, 2015, 12:27 p.m. UTC | #7
Patch merged,
Maxim.

On 12/07/2015 21:44, Zoltan Kiss wrote:
>
>
> On 07/12/15 17:52, Bill Fischofer wrote:
>> You're seeing this because in linux-dpdk the next field of the
>> odp_buffer_hdr_t is not at offset 0.  In linux-generic next is at offset
>> zero so this code is slightly more efficient.  So this change really
>> should be in the linux-dpdk version of this code rather than 
>> linux-generic.
>
> I see. Well, it's very obscure.
>
> Btw. I don't see why we have origin_qe->s.reorder_tail: we never use 
> its value, and even if we would have the tail member of the linked 
> list, it has little use as it is not double-linked.
>
>>
>> However the performance penalty is minor if you want to keep a common
>> linux-generic/linux-dpdk version of this file, so:
>>
>> On Mon, Dec 7, 2015 at 11:36 AM, Zoltan Kiss <zoltan.kiss@linaro.org
>> <mailto:zoltan.kiss@linaro.org>> wrote:
>>
>>     Maxim, Bill, please check this ASAP, and if feasible commit it to
>>     master, it blocks ODP-DPDK upgrade to 1.5
>>
>>     Zoli
>>
>>
>>     On 07/12/15 17:33, Zoltan Kiss wrote:
>>
>>         reorder_prev is set to the address of the pointer
>>         origin_qe->s.reorder_head,
>>         which is wrong. If the linked list was empty, that won't be
>>         corrected, and
>>         reorder_prev->next points to the adjacent queue entry's status
>>         field. If that
>>         entry is used, that queue's metadata will be corrupted.
>>         This was found by running the chaos scheduler test with 
>> ODP-DPDK.
>>
>>         Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org
>>         <mailto:zoltan.kiss@linaro.org>>
>>
>>
>> Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org
>> <mailto:bill.fischofer@linaro.org>>
>>
>>         ---
>>         diff --git a/platform/linux-generic/include/odp_queue_internal.h
>>         b/platform/linux-generic/include/odp_queue_internal.h
>>         index a70044b..1cc0ed2 100644
>>         --- a/platform/linux-generic/include/odp_queue_internal.h
>>         +++ b/platform/linux-generic/include/odp_queue_internal.h
>>         @@ -212,8 +212,7 @@ static inline void reorder_enq(queue_entry_t
>>         *queue,
>>                                         int sustain)
>>            {
>>                  odp_buffer_hdr_t *reorder_buf = 
>> origin_qe->s.reorder_head;
>>         -       odp_buffer_hdr_t *reorder_prev =
>>         -               (odp_buffer_hdr_t *)(void
>>         *)&origin_qe->s.reorder_head;
>>         +       odp_buffer_hdr_t *reorder_prev = NULL;
>>
>>                  while (reorder_buf && order >= reorder_buf->order) {
>>                          reorder_prev = reorder_buf;
>>         @@ -221,7 +220,12 @@ static inline void
>>         reorder_enq(queue_entry_t *queue,
>>                  }
>>
>>                  buf_hdr->next = reorder_buf;
>>         -       reorder_prev->next = buf_hdr;
>>         +
>>         +       if (reorder_prev)
>>         +               reorder_prev->next = buf_hdr;
>>         +       else
>>         +               origin_qe->s.reorder_head = buf_hdr;
>>         +
>>
>>                  if (!reorder_buf)
>>                          origin_qe->s.reorder_tail = buf_hdr;
>>
>>
Zoltan Kiss Dec. 9, 2015, 4:54 p.m. UTC | #8
Hi,

On 08/12/15 02:43, Bill Fischofer wrote:
> As to your other question, reorder_tail exists because the reorder queue
> is a queue, however elements on it are sorted by order rather than
> arrival time. The tail is used because dequeues can dequeue multiple
> elements at a time.

My problem is that the reorder_tail struct member is only set twice (in 
queue_init() and reorder_enq()), and never read. And even in 
reorder_enq(), we basically save buf_hdr there when reorder_head was 
NULL, so basically the first buffer ever placed on the reorder queue. 
That doesn't make too much sense to me, but maybe I misunderstand 
something. Maybe that struct member is also accessed by other means than 
referring to it's name? (in that case, a comment would be very good for 
the struct description)

Regards,

Zoltan
Bill Fischofer Dec. 9, 2015, 5 p.m. UTC | #9
It's there for consistency with other queue structures.  Are you looking to
remove it because you want to save 8 bytes? That should be doable if needed.

On Wed, Dec 9, 2015 at 10:54 AM, Zoltan Kiss <zoltan.kiss@linaro.org> wrote:

> Hi,

>

> On 08/12/15 02:43, Bill Fischofer wrote:

>

>> As to your other question, reorder_tail exists because the reorder queue

>> is a queue, however elements on it are sorted by order rather than

>> arrival time. The tail is used because dequeues can dequeue multiple

>> elements at a time.

>>

>

> My problem is that the reorder_tail struct member is only set twice (in

> queue_init() and reorder_enq()), and never read. And even in reorder_enq(),

> we basically save buf_hdr there when reorder_head was NULL, so basically

> the first buffer ever placed on the reorder queue. That doesn't make too

> much sense to me, but maybe I misunderstand something. Maybe that struct

> member is also accessed by other means than referring to it's name? (in

> that case, a comment would be very good for the struct description)

>

> Regards,

>

> Zoltan

>
Zoltan Kiss Dec. 9, 2015, 5:42 p.m. UTC | #10
On 09/12/15 17:00, Bill Fischofer wrote:
> It's there for consistency with other queue structures.  Are you looking
> to remove it because you want to save 8 bytes? That should be doable if
> needed.

I don't mind the 8 bytes too much, though if it's absolutely unused then 
we should rather remove it. But I think it's very confusing that we are 
writing it there in reorder_enq(), while that write doesn't make any 
sense. Other people might waste their time too to figure out that it 
really doesn't have any effect.

>
> On Wed, Dec 9, 2015 at 10:54 AM, Zoltan Kiss <zoltan.kiss@linaro.org
> <mailto:zoltan.kiss@linaro.org>> wrote:
>
>     Hi,
>
>     On 08/12/15 02:43, Bill Fischofer wrote:
>
>         As to your other question, reorder_tail exists because the
>         reorder queue
>         is a queue, however elements on it are sorted by order rather than
>         arrival time. The tail is used because dequeues can dequeue multiple
>         elements at a time.
>
>
>     My problem is that the reorder_tail struct member is only set twice
>     (in queue_init() and reorder_enq()), and never read. And even in
>     reorder_enq(), we basically save buf_hdr there when reorder_head was
>     NULL, so basically the first buffer ever placed on the reorder
>     queue. That doesn't make too much sense to me, but maybe I
>     misunderstand something. Maybe that struct member is also accessed
>     by other means than referring to it's name? (in that case, a comment
>     would be very good for the struct description)
>
>     Regards,
>
>     Zoltan
>
>
diff mbox

Patch

diff --git a/platform/linux-generic/include/odp_queue_internal.h b/platform/linux-generic/include/odp_queue_internal.h
index a70044b..1cc0ed2 100644
--- a/platform/linux-generic/include/odp_queue_internal.h
+++ b/platform/linux-generic/include/odp_queue_internal.h
@@ -212,8 +212,7 @@  static inline void reorder_enq(queue_entry_t *queue,
 			       int sustain)
 {
 	odp_buffer_hdr_t *reorder_buf = origin_qe->s.reorder_head;
-	odp_buffer_hdr_t *reorder_prev =
-		(odp_buffer_hdr_t *)(void *)&origin_qe->s.reorder_head;
+	odp_buffer_hdr_t *reorder_prev = NULL;
 
 	while (reorder_buf && order >= reorder_buf->order) {
 		reorder_prev = reorder_buf;
@@ -221,7 +220,12 @@  static inline void reorder_enq(queue_entry_t *queue,
 	}
 
 	buf_hdr->next = reorder_buf;
-	reorder_prev->next = buf_hdr;
+
+	if (reorder_prev)
+		reorder_prev->next = buf_hdr;
+	else
+		origin_qe->s.reorder_head = buf_hdr;
+
 
 	if (!reorder_buf)
 		origin_qe->s.reorder_tail = buf_hdr;