Messages in this thread | | | Date | Wed, 15 May 2024 15:14:59 +0300 | From | Fedor Pchelkin <> | Subject | Re: [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().
| |