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
