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

[MORITA Kazutaka]

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?

> 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.

>
>
>>
>>> +            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?

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.

Thanks,

Kazutaka