Messages in this thread | | | Date | Fri, 10 May 2024 18:35:17 +0100 | Subject | Re: [PATCH v2 1/4] dma-mapping: benchmark: fix up kthread-related error handling | From | Robin Murphy <> |
| |
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.
Thanks, Robin.
> goto out; > } > > @@ -139,13 +141,17 @@ static int do_map_benchmark(struct map_benchmark_data *map) > > msleep_interruptible(map->bparam.seconds * 1000); > > - /* wait for the completion of benchmark threads */ > + /* wait for the completion of all started benchmark threads */ > for (i = 0; i < threads; i++) { > - ret = kthread_stop(tsk[i]); > - if (ret) > - goto out; > + int kthread_ret = kthread_stop_put(tsk[i]); > + > + if (kthread_ret) > + ret = kthread_ret; > } > > + if (ret) > + goto out; > + > loops = atomic64_read(&map->loops); > if (likely(loops > 0)) { > u64 map_variance, unmap_variance; > @@ -170,8 +176,6 @@ static int do_map_benchmark(struct map_benchmark_data *map) > } > > out: > - for (i = 0; i < threads; i++) > - put_task_struct(tsk[i]); > put_device(map->dev); > kfree(tsk); > return ret;
| |