Re: [PATCH 2/3] kernel-shark: Change the mechanism of the multi-threaded search

["Yordan Karadzhov (VMware)" <y.karadz@gmail.com>]

On 24.04.20 г. 23:12 ч., Steven Rostedt wrote:
> 
> After applying this patch, I was hitting a lockup with this file:
> 
> http://rostedt.org/private/trace-all.dat.xz
> 
> By selecting "Event" and searching for "sched" it would hang at %87
> 

Hi Steven,
Very well spotted. I am not able to reproduce the bug directly, but I 
guess this is because you are running this on a machine that has 24 CPU 
cores (or 12 and hyper-trading enabled).

> I found the bug below.
> 
> On Mon, 30 Mar 2020 19:17:22 +0300
> "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> 
>> --- a/kernel-shark/src/KsModels.cpp
>> +++ b/kernel-shark/src/KsModels.cpp
>> @@ -52,22 +52,25 @@ size_t KsFilterProxyModel::_search(int column,
>>   				   const QString &searchText,
>>   				   search_condition_func cond,
>>   				   QList<int> *matchList,
>> +				   int step,
>>   				   int first, int last,
>>   				   QProgressBar *pb,
>>   				   QLabel *l,
>> +				   int *lastRowSearched,
>>   				   bool notify)
>>   {
>>   	int index, row, nRows(last - first + 1);
>> -	int pbCount(1);
>> +	int milestone(1), pbCount(1);
>>   	QString item;
>>   
>>   	if (nRows > KS_PROGRESS_BAR_MAX)
>> -		pbCount = nRows / (KS_PROGRESS_BAR_MAX - _searchProgress);
>> +		milestone = pbCount = nRows / (KS_PROGRESS_BAR_MAX - step -
>> +					       _searchProgress);

The problem will show up if this division has no remainder.

>>   	else
>>   		_searchProgress = KS_PROGRESS_BAR_MAX - nRows;
>>   
>>   	/* Loop over the items of the proxy model. */
>> -	for (index = first; index <= last; ++index) {
>> +	for (index = first; index <= last; index += step) {
>>   		/*
>>   		 * Use the index of the proxy model to retrieve the value
>>   		 * of the row number in the base model.
>> @@ -78,17 +81,23 @@ size_t KsFilterProxyModel::_search(int column,
>>   			matchList->append(row);
>>   
>>   		if (_searchStop) {
>> -			if (notify) {
>> -				_searchProgress = KS_PROGRESS_BAR_MAX;
>> +			if (lastRowSearched)
>> +				*lastRowSearched = index;
>> +
>> +			if (notify)
>>   				_pbCond.notify_one();
>> -			}
>>   
>>   			break;
>>   		}
>>   
>>   		/* Deal with the Progress bar of the seatch. */
>> -		if ((index - first) % pbCount == 0) {
>> +		if ((index - first) > milestone) {
> 
> Changing the above to:
> 
> 		if ((index - first) >= milestone) {
> 
> fixes it.

Correct.

> 
>> +			milestone += pbCount;
>>   			if (notify) {
>> +				/*
>> +				 * This is a multi-threaded search. Notify
>> +				 * the main thread to update the progress bar.
>> +				 */
>>   				std::lock_guard<std::mutex> lk(_mutex);
>>   				++_searchProgress;
>>   				_pbCond.notify_one();
>> @@ -100,6 +109,7 @@ size_t KsFilterProxyModel::_search(int column,
>>   
>>   				if (l)
>>   					l->setText(QString(" %1").arg(matchList->count()));
>> +
>>   				QApplication::processEvents();
>>   			}
>>   		}
> 
> 
>   Otherwise it looks like it leaves out the final update and the
> code hangs here:
> 
> 	/* Start all other threads. */
> 	for (int r = 1; r < nThreads; ++r)
> 		maps.push_back(std::async(lamSearchMap,
> 					  startFrom + r,
> 					  false)); // notify = false
> 
> 	while (_searchFSM.getState() == search_state_t::InProgress_s &&
> 	       _proxyModel.searchProgress() < KS_PROGRESS_BAR_MAX - nThreads) {

Alternative fix can be to changing the above to:
	       _proxyModel.searchProgress() < KS_PROGRESS_BAR_MAX - nThreads - 1) {

I would say we can apply both. What do you think?

Thanks!
Yordan

> 		std::unique_lock<std::mutex> lk(_proxyModel._mutex);
> 		_proxyModel._pbCond.wait(lk);
> 
> < It's stuck above >
> 
> 		_searchFSM.setProgress(_proxyModel.searchProgress());
> 		QApplication::processEvents();
> 	}
> 
> 
> -- Steve
>