Hi,
On 03/02/2016 11:05 AM, Minoru Usui wrote:
> Hi, Zhou
>
>>>>>>> ===
>>>>>>> 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.
>
> If you don't get mutex lock in consumer side, this change is meaningless.
> Of course, info->current_pfn may equal to end_pfn at the end of the cycle,
> but there is a timing that info->current_pfn is bigger than end_pfn in processing producer thread.
>
> The root cause is producer increments info->current_pfn everytime, even if info->current_pfn == end_pfn
> in following code.
>
Actually, I didn't get what you mean exactly until this letter...
I think there is no problem if the info->current_pfn is larger than the end_pfn
in the function write_kdump_pages_parallel_cyclic(), for no other one will use
current_pfn here.
Since we can't and needn't prevent using info->current_pfn outside the function,
we should keep info->current_pfn correct before returning from the function.
> ===
>>>>> + /* get next pfn */
>>>>> + pthread_mutex_lock(&info->current_pfn_mutex);
>>>>> + pfn = info->current_pfn;
>>>>> + info->current_pfn++; # increment everytime
>>>>> + page_flag_buf->ready = FLAG_FILLING;
>>>>> + pthread_mutex_unlock(&info->current_pfn_mutex);
>>>>>
>>>>> - buf_ready = TRUE;
>>>>> + page_flag_buf->pfn = pfn;
>>>>>
>>>>> - page_data_buf[index].pfn = pfn;
>>>>> - page_data_buf[index].ready = 1;
>>>>> + if (pfn >= kdump_thread_args->end_pfn) {
>>>>> + page_data_buf[index].used = FALSE;
>>>>> + page_flag_buf->ready = FLAG_READY;
>>>>> + break; # not decrement
>>>>> + }
> ===
>
> If you don't allow info->current_pfn is bigger than end_pfn,
> you don't need to increment info->current_pfn when pfn >= kdump_thread_args->end_pfn like following.
>
> ===
> /* get next pfn */
> pthread_mutex_lock(&info->current_pfn_mutex);
> pfn = info->current_pfn;
> page_flag_buf->pfn = pfn;
> if (pfn >= kdump_thread_args->end_pfn) {
> page_data_buf[index].used = FALSE;
> page_flag_buf->ready = FLAG_READY;
> pthread_mutex_unlock(&info->current_pfn_mutex);
> break;
> }
> page_flag_buf->ready = FLAG_FILLING;
> info->current_pfn++;
> pthread_mutex_unlock(&info->current_pfn_mutex);
> ===
>
> If you allow info->current_pfn is bigger than end_pfn, producer doesn't need to change info->current_pfn.
>
I also have thought about it.
It can keep current_pfn never larger than the end.
But it also makes the code a bit more complex.
If there aren't any special reason, I don't think it's worth to do it.
--
Thanks
Zhou
