| Message ID | 1418898463-25492-1-git-send-email-taras.kondratiuk@linaro.org |
|---|---|
| State | New |
| Headers | show |
This looks good. As a practice, for patch revisions post-merge I think these should be tracked as bugs that are being fixed, and the patch comments should note what bug it's addressing. On Thu, Dec 18, 2014 at 4:27 AM, Taras Kondratiuk < taras.kondratiuk@linaro.org> wrote: > > PKTIN queue was not handled correctly. Check queue status instead of > queue type. > > Reported-by: Stuart Haslam <stuart.haslam@arm.com> > Signed-off-by: Taras Kondratiuk <taras.kondratiuk@linaro.org> > --- > platform/linux-generic/odp_queue.c | 39 > ++++++++++++++++++++------------------ > 1 file changed, 21 insertions(+), 18 deletions(-) > > diff --git a/platform/linux-generic/odp_queue.c > b/platform/linux-generic/odp_queue.c > index 1462b41..c128aa0 100644 > --- a/platform/linux-generic/odp_queue.c > +++ b/platform/linux-generic/odp_queue.c > @@ -207,27 +207,30 @@ int odp_queue_destroy(odp_queue_t handle) > queue->s.enqueue = queue_enq_dummy; > queue->s.enqueue_multi = queue_enq_multi_dummy; > > - if (queue->s.type == ODP_QUEUE_TYPE_POLL || > - queue->s.type == ODP_QUEUE_TYPE_PKTOUT) { > + switch (queue->s.status) { > + case QUEUE_STATUS_READY: > queue->s.status = QUEUE_STATUS_FREE; > queue->s.head = NULL; > queue->s.tail = NULL; > - } else if (queue->s.type == ODP_QUEUE_TYPE_SCHED) { > - if (queue->s.status == QUEUE_STATUS_SCHED) { > - /* > - * Override dequeue_multi to destroy queue when it > will > - * be scheduled next time. > - */ > - queue->s.status = QUEUE_STATUS_DESTROYED; > - queue->s.dequeue_multi = queue_deq_multi_destroy; > - } else { > - /* Queue won't be scheduled anymore */ > - odp_buffer_free(queue->s.sched_buf); > - queue->s.sched_buf = ODP_BUFFER_INVALID; > - queue->s.status = QUEUE_STATUS_FREE; > - queue->s.head = NULL; > - queue->s.tail = NULL; > - } > + break; > + case QUEUE_STATUS_SCHED: > + /* > + * Override dequeue_multi to destroy queue when it will > + * be scheduled next time. > + */ > + queue->s.status = QUEUE_STATUS_DESTROYED; > + queue->s.dequeue_multi = queue_deq_multi_destroy; > + break; > + case QUEUE_STATUS_NOTSCHED: > + /* Queue won't be scheduled anymore */ > + odp_buffer_free(queue->s.sched_buf); > + queue->s.sched_buf = ODP_BUFFER_INVALID; > + queue->s.status = QUEUE_STATUS_FREE; > + queue->s.head = NULL; > + queue->s.tail = NULL; > + break; > + default: > + ODP_ABORT("Unexpected queue status\n"); > } > UNLOCK(&queue->s.lock); > > -- > 1.9.1 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 12/18/2014 01:42 PM, Bill Fischofer wrote: > This looks good. As a practice, for patch revisions post-merge I think > these should be tracked as bugs that are being fixed, and the patch > comments should note what bug it's addressing. Is it our 'official' approach? Should I create bugs on bugs.linaro.org by myself in this case?
I'm not sure if we have an official process (yet), but that's what I've been doing. Comments, Mike? On Thu, Dec 18, 2014 at 10:55 AM, Taras Kondratiuk < taras.kondratiuk@linaro.org> wrote: > > On 12/18/2014 01:42 PM, Bill Fischofer wrote: > >> This looks good. As a practice, for patch revisions post-merge I think >> these should be tracked as bugs that are being fixed, and the patch >> comments should note what bug it's addressing. >> > > Is it our 'official' approach? > Should I create bugs on bugs.linaro.org by myself in this case? >
On 12/18/2014 08:05 PM, Bill Fischofer wrote: > I'm not sure if we have an official process (yet), but that's what > I've been doing. Comments, Mike? > I think we don't need to open bugs for already fixed things. What is the reason for that? Maxim. > On Thu, Dec 18, 2014 at 10:55 AM, Taras Kondratiuk > <taras.kondratiuk@linaro.org <mailto:taras.kondratiuk@linaro.org>> wrote: > > On 12/18/2014 01:42 PM, Bill Fischofer wrote: > > This looks good. As a practice, for patch revisions > post-merge I think > these should be tracked as bugs that are being fixed, and the > patch > comments should note what bug it's addressing. > > > Is it our 'official' approach? > Should I create bugs on bugs.linaro.org <http://bugs.linaro.org> > by myself in this case? > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
On 18 December 2014 at 12:23, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 12/18/2014 08:05 PM, Bill Fischofer wrote: > >> I'm not sure if we have an official process (yet), but that's what I've >> been doing. Comments, Mike? >> >> > I think we don't need to open bugs for already fixed things. What is the > reason for that? > To make it easier to track things like this :) To my shame I see that I never responded to this originally :( > > Maxim. > > On Thu, Dec 18, 2014 at 10:55 AM, Taras Kondratiuk < >> taras.kondratiuk@linaro.org <mailto:taras.kondratiuk@linaro.org>> wrote: >> >> On 12/18/2014 01:42 PM, Bill Fischofer wrote: >> >> This looks good. As a practice, for patch revisions >> post-merge I think >> these should be tracked as bugs that are being fixed, and the >> patch >> comments should note what bug it's addressing. >> >> >> Is it our 'official' approach? >> Should I create bugs on bugs.linaro.org <http://bugs.linaro.org> >> by myself in this case? >> >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp >> > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 01/14/2015 09:07 PM, Stuart Haslam wrote: > On Thu, Dec 18, 2014 at 10:27:43AM +0000, Taras Kondratiuk wrote: >> PKTIN queue was not handled correctly. Check queue status instead of >> queue type. >> > > I just had cause to want to destroy a PKTIN queue and remembered this > patch. I applied it and tested but get a SEGV in queue_deq_multi_destroy > when trying to free queue->s.sched_buf as it's ODP_BUFFER_INVALID, which > seems to be due to it being called twice for the same queue. Could you please push your test code somewhere? I don't see where it can be called twice. BTW should odp_buffer_free(ODP_BUFFER_INVALID) cause SEGV?
On 14 January 2015 at 21:50, Taras Kondratiuk <taras.kondratiuk@linaro.org> wrote: > On 01/14/2015 09:07 PM, Stuart Haslam wrote: >> On Thu, Dec 18, 2014 at 10:27:43AM +0000, Taras Kondratiuk wrote: >>> PKTIN queue was not handled correctly. Check queue status instead of >>> queue type. >>> >> >> I just had cause to want to destroy a PKTIN queue and remembered this >> patch. I applied it and tested but get a SEGV in queue_deq_multi_destroy >> when trying to free queue->s.sched_buf as it's ODP_BUFFER_INVALID, which >> seems to be due to it being called twice for the same queue. > > Could you please push your test code somewhere? > I don't see where it can be called twice. > > BTW should odp_buffer_free(ODP_BUFFER_INVALID) cause SEGV? Yes or play a game of tic-tac-toe. Or anything else. I see odp_buffer_free() as performance critical and would like to be able to avoid any parameter checks. Your implementation may check the buffer handle but passing an invalid handle is an illegal operation and must not be treated as if it is OK. Calling abort() is fine. > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
diff --git a/platform/linux-generic/odp_queue.c b/platform/linux-generic/odp_queue.c index 1462b41..c128aa0 100644 --- a/platform/linux-generic/odp_queue.c +++ b/platform/linux-generic/odp_queue.c @@ -207,27 +207,30 @@ int odp_queue_destroy(odp_queue_t handle) queue->s.enqueue = queue_enq_dummy; queue->s.enqueue_multi = queue_enq_multi_dummy; - if (queue->s.type == ODP_QUEUE_TYPE_POLL || - queue->s.type == ODP_QUEUE_TYPE_PKTOUT) { + switch (queue->s.status) { + case QUEUE_STATUS_READY: queue->s.status = QUEUE_STATUS_FREE; queue->s.head = NULL; queue->s.tail = NULL; - } else if (queue->s.type == ODP_QUEUE_TYPE_SCHED) { - if (queue->s.status == QUEUE_STATUS_SCHED) { - /* - * Override dequeue_multi to destroy queue when it will - * be scheduled next time. - */ - queue->s.status = QUEUE_STATUS_DESTROYED; - queue->s.dequeue_multi = queue_deq_multi_destroy; - } else { - /* Queue won't be scheduled anymore */ - odp_buffer_free(queue->s.sched_buf); - queue->s.sched_buf = ODP_BUFFER_INVALID; - queue->s.status = QUEUE_STATUS_FREE; - queue->s.head = NULL; - queue->s.tail = NULL; - } + break; + case QUEUE_STATUS_SCHED: + /* + * Override dequeue_multi to destroy queue when it will + * be scheduled next time. + */ + queue->s.status = QUEUE_STATUS_DESTROYED; + queue->s.dequeue_multi = queue_deq_multi_destroy; + break; + case QUEUE_STATUS_NOTSCHED: + /* Queue won't be scheduled anymore */ + odp_buffer_free(queue->s.sched_buf); + queue->s.sched_buf = ODP_BUFFER_INVALID; + queue->s.status = QUEUE_STATUS_FREE; + queue->s.head = NULL; + queue->s.tail = NULL; + break; + default: + ODP_ABORT("Unexpected queue status\n"); } UNLOCK(&queue->s.lock);
PKTIN queue was not handled correctly. Check queue status instead of queue type. Reported-by: Stuart Haslam <stuart.haslam@arm.com> Signed-off-by: Taras Kondratiuk <taras.kondratiuk@linaro.org> --- platform/linux-generic/odp_queue.c | 39 ++++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 18 deletions(-)