Re: perf: add cond_resched() to task_function_call()

[Barret Rhoden]

On 4/14/20 4:42 PM, Peter Zijlstra wrote:
> On Tue, Apr 14, 2020 at 03:03:51PM -0400, Barret Rhoden wrote:
> > Under rare circumstances, task_function_call() can repeatedly fail and
> > cause a soft lockup.
> >
> > There is a slight race where the process is no longer running on the cpu
> > we targeted by the time remote_function() runs.  The code will simply
> > try again.  If we are very unlucky, this will continue to fail, until a
> > watchdog fires.  This can happen in a heavily loaded, multi-core virtual
> > machine.
>
> Sigh,.. virt again :/
>
> > Reported-by: syzbot+bb4935a5c09b5ff79940@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Signed-off-by: Barret Rhoden <brho@xxxxxxxxxx>
> > ---
> >   kernel/events/core.c | 12 ++++++++----
> >   1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index 55e44417f66d..65c2c05e24c2 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -99,7 +99,7 @@ static void remote_function(void *data)
> >    *
> >    * returns: @func return value, or
> >    *	    -ESRCH  - when the process isn't running
> > - *	    -EAGAIN - when the process moved away
> > + *	    -ENXIO  - when the cpu the process was on has gone offline
> >    */
>
> Hurm.. I don't think that was actually intended behaviour.

Yeah, I noticed we never return EAGAIN, so I figured I'd look for any 
other potential return values.

> As long as
> the task lives we ought to retry. Luckily I don't think the current code
> cares much, it'll loop again on the caller side.
>
> With the exception of perf_cgroup_attach() that is, that might actually
> be broken because of this.
>
> >   static int
> >   task_function_call(struct task_struct *p, remote_function_f func, void *info)
> > @@ -112,11 +112,15 @@ task_function_call(struct task_struct *p, remote_function_f func, void *info)
> >   	};
> >   	int ret;
> >   
> > -	do {
> > -		ret = smp_call_function_single(task_cpu(p), remote_function, &data, 1);
> > +	while (1) {
> > +		ret = smp_call_function_single(task_cpu(p), remote_function,
> > +					       &data, 1);
> >   		if (!ret)
> >   			ret = data.ret;
> > -	} while (ret == -EAGAIN);
> > +		if (ret != -EAGAIN)
> > +			break;
> > +		cond_resched();
> > +	}
>
> So how about we make that:
>
> 	for (;;) {
> 		ret = smp_call_function_single(task_cpu(p), remote_function, &data, 1);
> 		ret = !ret ? data.ret : -EAGAIN;
>
> 		if (ret != -EAGAIN)
> 			break;
>
> 		cond_resched();
> 	}
>
> Or something like that, hmmm?

Can do.  I can put a comment in too.  /* Retry due to any failure in 
smp_call_function */ or something.

Thanks,

Barret