On Fri, 15 Jun 2018 at 15:06, Kim Phillips kim.phillips@arm.com wrote:
On Fri, 15 Jun 2018 10:03:43 -0600 Mathieu Poirier mathieu.poirier@linaro.org wrote:
On Thu, 14 Jun 2018 at 17:53, Kim Phillips kim.phillips@arm.com wrote:
On Thu, 14 Jun 2018 13:41:33 -0600 Mathieu Poirier mathieu.poirier@linaro.org wrote:
@@ -430,24 +420,30 @@ static int cs_etm__setup_queue(struct cs_etm_auxtrace *etm, struct auxtrace_queue *queue, unsigned int queue_nr) {
int ret = 0; struct cs_etm_queue *etmq = queue->priv; if (list_empty(&queue->head) || etmq)
return 0;
goto out;
What does 'goto out' buy us here? Having the return in-place is easier/clearer to read.
It streamlines the exit path.
How? It unnecessarily redirects it and sends the reader hunting.
This is how the function body looks after this patch:
{ int ret = 0; struct cs_etm_queue *etmq = queue->priv;
if (list_empty(&queue->head) || etmq) goto out; etmq = cs_etm__alloc_queue(etm); if (!etmq) { ret = -ENOMEM; goto out; } queue->priv = etmq; etmq->etm = etm; etmq->queue_nr = queue_nr; etmq->cpu = queue->cpu; etmq->tid = queue->tid; etmq->pid = -1; etmq->offset = 0; etmq->period_instructions = 0;
out: return ret; }
The real gain from this approach becomes clearer when timestamp initialisation (second patchset) gets thrown in the mix.
And this is how it would look whilst continuing to directly return from either of the exit paths:
{ struct cs_etm_queue *etmq = queue->priv;
if (list_empty(&queue->head) || etmq) return 0; etmq = cs_etm__alloc_queue(etm); if (!etmq) return -ENOMEM; queue->priv = etmq; etmq->etm = etm; etmq->queue_nr = queue_nr; etmq->cpu = queue->cpu; etmq->tid = queue->tid; etmq->pid = -1; etmq->offset = 0; etmq->period_instructions = 0; return 0;
}
The latter is easier to follow, and doesn't need a 'ret' variable, any gotos, or any labels at all.
etmq = cs_etm__alloc_queue(etm, queue_nr);
etmq = cs_etm__alloc_queue(etm);
if (!etmq)
return -ENOMEM;
if (!etmq) {
ret = -ENOMEM;
goto out;
}
same here.
queue->priv = etmq;
if (queue->cpu != -1)
etmq->cpu = queue->cpu;
etmq->etm = etm;
etmq->queue_nr = queue_nr;
etmq->cpu = queue->cpu;
the != -1 condition above is being removed and not replaced, yet there's no explanation in the commit text. Should it be a part of a different patch?
Variable queue->cpu is either -1 (per-thread) or hold the value of a CPU (CPU-wide). So instead of invariably setting etmq->cpu to -1 in cs_etm__alloc_queue() and checking if it is worth changing that in cs_etm__setup_queue() as the current code does, directly doing etmq->cpu = queue->cpu yields exactly the same result.
OK, it would be nice to have that explanation in the commit text.
Thanks,
Kim