MIME-Version: 1.0
In-Reply-To: <20170613145227.6cc9d1d7@gandalf.local.home>
References: <20170609025327.9508-1-joelaf@google.com> <20170609025327.9508-3-joelaf@google.com>
 <20170613145227.6cc9d1d7@gandalf.local.home>
From: Joel Fernandes <joelaf@google.com>
Date: Tue, 13 Jun 2017 16:47:39 -0700
Message-ID: <CAJWu+opNKE+ZX_+_Xp9S0w=3zGNQNhiS+L20b-v6DMxBuFzxgw@mail.gmail.com>
Subject: Re: [PATCH RFC v3 2/4] tracing: Add support for recording tgid of tasks
To: Steven Rostedt <rostedt@goodmis.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
        Michael Sartain <mikesart@gmail.com>, kernel-team@android.com,
        Ingo Molnar <mingo@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4630
Lines: 152

On Tue, Jun 13, 2017 at 11:52 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
[..]

>> +/**
>> + * tracing_record_taskinfo - record the task information of multiple tasks
>> + *
>> + * @tasks - list of tasks (array of task_struct pointers)
>> + * @len   - length of the list of tasks
>> + * @flags - TRACE_RECORD_CMDLINE for recording comm
>> + *          TRACE_RECORD_TGID for recording tgid
>> + */
>> +void tracing_record_taskinfo(struct task_struct **tasks, int len, int flags)
>> +{
>> +     int i;
>> +     bool cmdline, tgid;
>> +
>> +     tgid = !!(flags & TRACE_RECORD_TGID);
>> +     cmdline = !!(flags & TRACE_RECORD_CMDLINE);
>> +
>> +     if (unlikely(!cmdline && !tgid))
>> +             return;
>
> This would be better to move this above the assignment of tgid and
> cmdline and just have:
>
>         if (unlikely(!(flags & (TRACE_RECORD_TGID | TRACE_RECORD_CMDLINE))
>                 return;

Ok will fix.

>
> Also, no need for the !! in the assignments. They are already marked as
> boolean. A simple:
>
>         tgid = flags & TRACE_RECORDC_TGID;
>
> would suffice.

Got it, will fix, thanks.

>> +
>> +     if (atomic_read(&trace_record_taskinfo_disabled) || !tracing_is_on())
>>               return;
>>
>> -     if (!__this_cpu_read(trace_cmdline_save))
>> +     if (!__this_cpu_read(trace_taskinfo_save))
>>               return;
>
> You can move the assignments after the above too. gcc *should*
> optimize it anyway. But I like to have all exit conditions first before
> we add other code, which includes assignments.

Ok, I agree.

>>
>> -     if (trace_save_cmdline(tsk))
>> -             __this_cpu_write(trace_cmdline_save, false);
>> +     for (i = 0; i < len; i++) {
>> +             if (cmdline && !trace_save_cmdline(tasks[i]))
>> +                     break;
>> +             if (tgid && !trace_save_tgid(tasks[i]))
>> +                     break;
>> +     }
>
> I really do not like this loop in the hot path. I say keep this as a
> single code as default, and add a tracing_record_taskinfo_multi(), as
> that's the exception (I only see one user compared to 3 users of
> single).
> You can add a helper function that is static __always_inline that can
> be used by both. But please, no loops for the single case. Actually,
> since there's only one multi user case, and it's always 2, I would say
> nuke the loop for that one and have a:
>
>  tracing_record_taskinfo_sched_switch()
>
> that takes a prev and a next, and updates both.
>
> I don't envision more than 2, and we don't need this to be robust for a
> use case that doesn't exist by sacrificing performance.

Ok, I agree its better to just handle the simple case first. I will
work on doing that.

>>  {
>> -     if (unlikely(!sched_ref))
>> -             return;
>> +     int flags = 0;
>> +     struct task_struct *tasks[2] = { prev, next };
>> +
>> +     if (sched_cmdline_ref != 0)
>> +             flags |= RECORD_CMDLINE;
>> +
>> +     if (sched_tgid_ref != 0)
>> +             flags |= RECORD_TGID;
>
>         flags = (RECORD_CMDLINE * !!(sched_cmdline_ref)) +
>                 (RECORD_TGID * !!(sched_tgid_ref));
>
>         if (!flags)
>                 return;

That looks much better, I will do that.

>> -     tracing_record_cmdline(prev);
>> -     tracing_record_cmdline(next);
>> +     tracing_record_taskinfo(tasks, 2, flags);
>
>         tracing_record_taskinfo_sched_switch(prev, next, flags);

Yep.

[..]
>> -static void tracing_start_sched_switch(void)
>> +static void tracing_start_sched_switch(int ops)
>>  {
>> +     bool sched_register;
>>       mutex_lock(&sched_register_mutex);
>> -     if (!(sched_ref++))
>> +     sched_register = (!sched_cmdline_ref && !sched_tgid_ref);
>> +
>> +     switch (ops) {
>> +     case RECORD_CMDLINE:
>> +             sched_cmdline_ref++;
>> +             break;
>> +
>> +     case RECORD_TGID:
>> +             sched_tgid_ref++;
>> +             break;
>> +     }
>> +
>> +     if (sched_tgid_ref == 1)
>> +             tracing_alloc_tgid_map();
>
> Since tgid_map is never freed, just do:
>
>         if (!tgid_map)
>                 tracing_alloc_tgid_map();
>
> Then if it fails to allocate this time, there's a chance that it will
> allocate another time.

I still need to check for the sched_tgid_ref flag though because in
the case where record-tgid is not enabled, then it would still try to
allocate and that's what I wanted to avoid.  I can change it to
(sched_tgid_ref > 0 && !tgid_map) which should take care of the
scenario you mentioned. Thanks for the idea.

Thanks for the review. I'll get to work and send out new and better
patches soon!

Regards,
Joel