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

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

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@xxxxxxxxx> 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