Messages in this thread | | | Subject | Re: [PATCH v2 4/4] perf arm-spe: Support hardware-based PID tracing | From | German Gomez <> | Date | Thu, 11 Nov 2021 12:23:08 +0000 |
| |
Hi Leo, Namhyung,
On 11/11/2021 08:30, Leo Yan wrote: > Hi Namhyung, > > On Wed, Nov 10, 2021 at 11:59:05PM -0800, Namhyung Kim wrote: > > [...] > >>>>> +static void arm_spe_set_pid_tid_cpu(struct arm_spe *spe, >>>>> + struct auxtrace_queue *queue) >>>>> +{ >>>>> + struct arm_spe_queue *speq = queue->priv; >>>>> + pid_t tid; >>>>> + >>>>> + tid = machine__get_current_tid(spe->machine, speq->cpu); >>>>> + if (tid != -1) { >>>>> + speq->tid = tid; >>>>> + thread__zput(speq->thread); >>>>> + } else >>>>> + speq->tid = queue->tid; >>>>> + >>>>> + if ((!speq->thread) && (speq->tid != -1)) { >>>>> + speq->thread = machine__find_thread(spe->machine, -1, >>>>> + speq->tid); >>>>> + } >>>>> + >>>>> + if (speq->thread) { >>>>> + speq->pid = speq->thread->pid_; >>>>> + if (queue->cpu == -1) >>>>> + speq->cpu = speq->thread->cpu; >>>>> + } >>>>> +} >>>>> + >>>>> +static int arm_spe_set_tid(struct arm_spe_queue *speq, pid_t tid) >>>>> +{ >>>>> + struct arm_spe *spe = speq->spe; >>>>> + int err = machine__set_current_tid(spe->machine, speq->cpu, tid, tid); >>>> I think we should pass -1 as pid as we don't know the real pid. >>> AFAICT, I observe one case for machine__set_current_tid() returning error >>> is 'speq->cpu' is -1 (this is the case for per-thread tracing). In >>> this case, if pass '-1' for pid/tid, it still will return failure. >>> >>> So here should return the error as it is. Am I missing anything? >> I'm not saying about the error. It's about thread status. >> In the machine__set_current_tid(), it calls >> machine__findnew_thread() with given pid and tid. >> >> I suspect it can set pid to a wrong value if the thread has >> no pid value at the moment. > Here we should avoid to write pid '-1' with > machine__set_current_tid().
If the kernel is writing the tids to the contextidr, isn't it wrong to assume tid == pid when decoding the context packets here? I haven't observed any impact in the built-in commands though, so there must be something I'm not seeing.
Thanks, German > > The function arm_spe_set_tid() is invoked when SPE trace data contains > context packet and it passes pid coming from the context packet. On > the other hand, when SPE trace data doesn't contain context packet, we > relies on context switch event to set pid value. So if we pass pid > '-1' in arm_spe_set_tid(), it will overwrite the pid value which has > been set by context switch event. > > Simply say, if SPE trace data contains context packet with valid pid, > perf invokes arm_spe_set_tid() to set the pid value. Otherwise, it > should skip this operation and roll back to use the pid value from > the context switch event. > > Thanks, > Leo
| |