Re: [PATCH v2 1/4] dma-mapping: benchmark: fix up kthread-related error handling

[Fedor Pchelkin <pchelkin@ispras.ru>]

Hi,

Thanks for review of the series!

Robin Murphy wrote:
> On 2024-05-04 12:47 pm, Fedor Pchelkin wrote:
> > kthread creation failure is invalidly handled inside do_map_benchmark().
> > The put_task_struct() calls on the error path are supposed to balance the
> > get_task_struct() calls which only happen after all the kthreads are
> > successfully created. Rollback using kthread_stop() for already created
> > kthreads in case of such failure.
> > 
> > In normal situation call kthread_stop_put() to gracefully stop kthreads
> > and put their task refcounts. This should be done for all started
> > kthreads.
> 
> Although strictly there were two overlapping bugs here, I'd agree that 
> it really doesn't seem worth the bother of trying to distinguish them. 
> I'm far from a kthread expert, but as best I can tell this looks like it 
> achieves a sensible final state. Modulo one nit below,
> 
> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
> 
> > Found by Linux Verification Center (linuxtesting.org).
> > 
> > Fixes: 65789daa8087 ("dma-mapping: add benchmark support for streaming DMA APIs")
> > Suggested-by: Robin Murphy <robin.murphy@arm.com>
> > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> > ---
> >   kernel/dma/map_benchmark.c | 16 ++++++++++------
> >   1 file changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/kernel/dma/map_benchmark.c b/kernel/dma/map_benchmark.c
> > index 02205ab53b7e..2478957cf9f8 100644
> > --- a/kernel/dma/map_benchmark.c
> > +++ b/kernel/dma/map_benchmark.c
> > @@ -118,6 +118,8 @@ static int do_map_benchmark(struct map_benchmark_data *map)
> >   		if (IS_ERR(tsk[i])) {
> >   			pr_err("create dma_map thread failed\n");
> >   			ret = PTR_ERR(tsk[i]);
> > +			while (--i >= 0)
> > +				kthread_stop(tsk[i]);
> 
> I think this means we'd end up actually starting the threads purely to 
> get them to see the KTHREAD_SHOULD_STOP flag and exit again? Not that 
> I'm too fussed about optimising an unexpected error path, however I 
> can't help but wonder if we might only need a put_task_struct() if we 
> can safely assume that the threads have never been started at this point.

The threads have been already started to the moment by
kthread_create_on_node() but put to uninterruptible sleep: please see
kthread() function. They just have to be explicitly awakened, find that
the KTHREAD_SHOULD_STOP flag was set and perform do_exit() in order to
terminate and release all resources. The thread_fn won't be executed in
such case.

I feel there is no more convenient way for doing this differently than
calling kthread_stop(). And the comment for kthread_stop() actually implies
that it is intended to work also just after kthread creation.

Calling put_task_struct() in that place will hit WARN_ON(!tsk->exit_state).
I'd say the last call to this function should be made after (or in result
of) the do_exit().