lkml.org 
[lkml]   [2024]   [May]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 1/4] dma-mapping: benchmark: fix up kthread-related error handling
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().

\
 
 \ /
  Last update: 2024-05-27 18:28    [W:0.054 / U:0.496 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site