Re: [PATCH v2] Improve the performance of --num-threads -d 31

["\"Zhou, Wenjian/周文?\"" <zhouwj-fnst@cn.fujitsu.com>]

Hi,

On 02/24/2016 08:45 AM, Minoru Usui wrote:
>>>>>>
>>>>>> -		/*
>>>>>> -		 * check pfn first without mutex locked to reduce the time
>>>>>> -		 * trying to lock the mutex
>>>>>> -		 */
>>>>>> -		if (page_data_buf[index].pfn != consuming_pfn)
>>>>>> -			continue;
>>>>>> +					info->page_flag_buf[i]->ready = FLAG_UNUSED;
>>>>>>
>>>>>> -		if (pthread_mutex_trylock(&page_data_buf[index].mutex) != 0)
>>>>>> -			continue;
>>>>>> +					info->current_pfn = end_pfn;
>>>>>
>>>>> This part doesn't get info->current_pfn_mutex.
>>>>> It becomes bigger than end_pfn at the end of producer thread in following case.
>>>>>
>>>>> ===
>>>>>      producer                   Consumer
>>>>> ---------------------------------------------------------
>>>>>      pthread_mutex_lock()
>>>>>      pfn = info->current_pfn
>>>>>                                 info->current_pfn = end_pfn
>>>>>      info->current_pfn++
>>>>>        -> end_pfn + 1
>>>>>      pthread_mutex_unlock()
>>>>> ===
>>>>>
>>>>> But I know, if this race is happened, processing other producer thread and consumer thread works well
>>>>> in current cycle.
>>>>> Because each thread checks whether info->current_pfn >= end_pfn.
>>>>>
>>>>> On the other hand, info->current_pfn is initialized to cycle->start_pfn before pthread_create()
>>>>> in write_kdump_pages_parallel_cyclic().
>>>>> This means it does not cause a problem in next cycle, too.
>>>>>
>>>>> In other words, my point is following.
>>>>>
>>>>>      a) When info->current_pfn changes, you have to get info->current_pfn_mutex.
>>>>>      b) If you allow info->current_pfn is bigger than end_pfn at the end of each cycle,
>>>>>         "info->current_pfn = end_pfn;" is unnecessary.
>>>>>
>>>>> Honestly, I don't like approach b).
>>>>
>>>> You're right. Some thing I thought is wrong.
>>>> But I don't like lock if I have other choice.
>>>> I will set info->current_pfn before returning.
>>>> How about it?
>>>
>>> If you mean you set info->current_pfn by producer side,
>>> this race will occur between producer A and producer B.
>>>
>>> I think you can't avoid getting mutex lock, if you will change info->current_pfn.
>>>
>>
>> I mean setting it by consumer.
> 
> I'm sorry, I can't imagine your proposal.
> What do you change?
> 
> Please show me the code.
> 

At that time, I meant setting the current_pfn at the end of the function
write_kdump_pages_parallel_cyclic().
But I don't think it is good choice now.

>>>>> ===
>>>>>      producer                   Consumer
>>>>> ---------------------------------------------------------
>>>>>      pthread_mutex_lock()
>>>>>      pfn = info->current_pfn
>>>>>                                 info->current_pfn = end_pfn
>>>>>      info->current_pfn++
>>>>>        -> end_pfn + 1
>>>>>      pthread_mutex_unlock()
>>>>> ===

How about just changing "info->current_pfn = end_pfn" to "info->current_pfn--" ?
Just like the first version of the patch.

-- 
Thanks
Zhou



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec