[Sheepdog] [PATCH v3 06/13] trace: low-level trace infrastructure proper

[Liu Yuan]

On 04/09/2012 01:02 PM, MORITA Kazutaka wrote:

> On Mon, Apr 9, 2012 at 1:14 PM, Liu Yuan <namei.unix at gmail.com> wrote:
>> On 04/09/2012 10:47 AM, MORITA Kazutaka wrote:
>>
>>> On Mon, Apr 9, 2012 at 10:28 AM, Liu Yuan <namei.unix at gmail.com> wrote:
>>>> On 04/06/2012 12:09 AM, MORITA Kazutaka wrote:
>>>>
>>>>> At Thu,  1 Mar 2012 10:20:18 +0800,
>>>>> Liu Yuan wrote:
>>>>>>
>>>>>> +}
>>>>>> +
>>>>>> +notrace struct caller *trace_lookup_ip(unsigned long ip, int create)
>>>>>> +{
>>>>>> +    int h = trace_hash(ip);
>>>>>> +    struct hlist_head *head = trace_hashtable + h;
>>>>>> +    struct hlist_node *node;
>>>>>> +    struct ipinfo info;
>>>>>> +    struct caller *new = NULL;
>>>>>> +
>>>>>> +    pthread_mutex_lock(&trace_lock);
>>>>>> +    if (hlist_empty(head))
>>>>>
>>>>> hlist_empty() calls trace_lookup_ip() again if it is not inlined, and
>>>>> it results in a deadlock.
>>>>>
>>>>> How about preventing a recursive call of trace_lookup_ip()?
>>>>>
>>>>> E.g.:
>>>>>
>>>>> notrace struct caller *trace_lookup_ip(unsigned long ip, int create)
>>>>> {
>>>>>         int h;
>>>>>         struct hlist_head *head;
>>>>>         struct hlist_node *node;
>>>>>         struct ipinfo info;
>>>>>         struct caller *new = NULL;
>>>>>         static __thread int looking_up_ip;
>>>>>
>>>>>         if (looking_up_ip)
>>>>>                 return NULL;
>>>>>
>>>>>         looking_up_ip = 1;
>>>>>
>>>>>         pthread_mutex_lock(&trace_lock);
>>>>>
>>>>>         ...
>>>>>
>>>>>         pthread_mutex_unlock(&trace_lock);
>>>>>
>>>>>         looking_up_ip = 0;
>>>>>         return new;
>>>>> }
>>>>>
>>>>
>>>>
>>>> Nope, this bring more problems than it resolves:
>>>>
>>>> 1) return NULL while there entry does exist will break current trace logic
>>>
>>> Is it difficult to change the logic?
>>
>>
>> Yes, kind of hard, currently we call trace_lookup_ip() to check if IP is
>> registered or not, upper layer (graph.c) cannot handle the case that
>> trace_lookup_ip() fails but IP is already registered.
> 
> If trace_lookup_ip() returns NULL, tracer returns without doing anything.
> Doesn't it work?
> ==
> diff --git a/sheep/trace/graph.c b/sheep/trace/graph.c
> index fff9060..8c9405a 100644
> --- a/sheep/trace/graph.c
> +++ b/sheep/trace/graph.c
> @@ -86,6 +86,8 @@ static notrace void graph_tracer(unsigned long ip,
> unsigned long *ret_addr)
>         struct caller *cr;
> 
>         cr = trace_lookup_ip(ip, 0);
> +       if (!cr)
> +               return;
>         assert(cr->namelen + 1 < TRACE_FNAME_LEN);
>         memcpy(trace.fname, cr->name, cr->namelen);
>         memset(trace.fname + cr->namelen, '\0', 1);
> ==
> 


I think the change of the semantic of trace_lookup_ip() is not
affordable, it is much easier for callers to expect "Yes" or "No" usage
other than "Yes" or "Maybe".

I'd suggest remove the re-entrance race inside trace_lookup_ip() instead
of workaround it outside. Your patch means let re-entrance happen, and
when it happens, just ignores it. This mean we don't trace the function
when the race is happing.

So I think tracer itself is a very isolated subsystem without a big code
set, so it is worth our time to make it clean and deal with 'notrace',
'always inline' attribute ourselves with care.

Now xzalloc cause problems, I think we can call malloc or calloc instead
and write down a warning to any future patcher that, "Hey, we should not
call sheep functions inside trace_lookup_ip() that are expected to be
traced".

>>
>>
>>>
>>>> 2) __thread variable doesn't make any effect to exclude concurrent access
>>>
>>> Concurrent access is excluded by pthread_mutex_lock().  The __thread
>>> variable just prevents the same thread from re-entering the core of
>>> trace_lookup_ip code.
>>
>>
>>>> 3) one line function of hlist_empty() will be inline for any sane
>>>> compiler if it supports it.
>>>
>>> If we add --enable-debug to configure options, the function is not
>>> inlined.
>>>
>>
>>
>> Even for now, I can't get the point of --enable-debug, which simply add
>> -O0 to gcc, does this mean we don't trust GCC reorder & optimisation? I
>> don't think sheepdog is low level enough to meet this problem.
> 
> Sometimes -O0 is useful to debug Sheepdog with GDB.
> 


I can't imagine what kind of problem need -O0, but anyway, if anyone
finds it useful, we need to keep it.

>>
>> If you really concerns of this, we can add a always_inline attribute to
>> hlist_empty().
> 
> It means we need to add always_inline for each time we add a function
> in tracer_lookup_ip().  It's not good.
> 
>>
>>>>
>>>>
>>>>>
>>>>>> +            goto not_found;
>>>>>> +
>>>>>> +    hlist_for_each_entry(new, node, head, hash) {
>>>>>> +            if (new->mcount == ip)
>>>>>> +                    goto out;
>>>>>> +    }
>>>>>> +not_found:
>>>>>> +    if (get_ipinfo(ip, &info) < 0) {
>>>>>> +            dprintf("ip: %lx not found\n", ip);
>>>>>> +            new = NULL;
>>>>>> +            goto out;
>>>>>> +    }
>>>>>> +    if (create) {
>>>>>> +            /* unlock to avoid deadlock */
>>>>>> +            pthread_mutex_unlock(&trace_lock);
>>>>>
>>>>> I think we don't need unlock here if we prevent recursion.
>>>>>
>>>>>
>>>>>> +            new = xzalloc(sizeof(*new));
>>>>>> +            pthread_mutex_lock(&trace_lock);
>>>
>>> IIUC, you call pthread_mutex_unlock()/lock() here because xzalloc()
>>> will call another trace_lookup_ip() and it results in deadlock, yes?
>>>
>>
>>
>> Yes
>>
>>> I think this code is really hard to maintain because we cannot call
>>> other functions which wouldn't be inlined.  In addition, it smells
>>> like a race condition since other function can enter the core of
>>> trace_lookup_ip() just after we release the lock here.
>>>
>>
>>
>> Yes, some code of Linux kernel has this race too (A ugly practice), so
>> here I just assume this wouldn't happen. But if we are really concerned
>> of it, we can add 'notrace' to it to get rid of race completely.
> 
> It's not good too to add notrace for each time we add a function in
> trace_lookup_ip(). It will reduce code maintainability.

I am afraid we have to take more care of ourselves when patching tracer
code. But once it is done, we don't need extra care for tracer users.
Tracer internal code is a bit lower level than other sheep code, so I
think it deserver our special care.

>>
>> BTW, speaking of cod maintainability, coroutine is much harder to read &
>> maintain.
> 
> I don't thinks so, but I don't mind dropping coroutine code.
> 
> Anyway, It's okay for me to merge trace code without addressing the
> above comments,
> but please disable tracer in configure by default until the code
> becomes mature and
> the majority of users want to use it.

Yes, I'll make it mute as default and it needs rebase quite a lot to the
master.

Thanks,
Yuan