On Thu, 2018-02-08 at 17:56 -0600, Benjamin Marzinski wrote:
> static void cleanup_func(void *data)
> {
> - int holders;
> + int running, holders;
> struct tur_checker_context *ct = data;
> - pthread_spin_lock(&ct->hldr_lock);
> - ct->holders--;
> - holders = ct->holders;
> - ct->thread = 0;
> - pthread_spin_unlock(&ct->hldr_lock);
> +
> + running = uatomic_xchg(&ct->running, 0);
> + holders = uatomic_sub_return(&ct->holders, 1);
> if (!holders)
> cleanup_context(ct);
> + if (!running)
> + pause();
> }
Hello Ben,
Why has the pause() call been added? I think it is safe to call pthread_cancel()
for a non-detached thread that has finished so I don't think that pause() call
is necessary.
> static int tur_running(struct tur_checker_context *ct)
> {
> - pthread_t thread;
> -
> - pthread_spin_lock(&ct->hldr_lock);
> - thread = ct->thread;
> - pthread_spin_unlock(&ct->hldr_lock);
> -
> - return thread != 0;
> + return (uatomic_read(&ct->running) != 0);
> }
Is such a one line function really useful? I think the code will be easier to read
if this function is inlined into its callers.
> @@ -418,8 +396,12 @@ int libcheck_check(struct checker * c)
> (tur_status == PATH_PENDING || tur_status == PATH_UNCHECKED)) {
> condlog(3, "%s: tur checker still running",
> tur_devt(devt, sizeof(devt), ct));
> - ct->running = 1;
> tur_status = PATH_PENDING;
> + } else {
> + int running = uatomic_xchg(&ct->running, 0);
> + if (running)
> + pthread_cancel(ct->thread);
> + ct->thread = 0;
> }
> }
Why has this pthread_cancel() call been added? I think that else clause can only be
reached if ct->running == 0 so I don't think that the pthread_cancel() call will ever
be reached.
Thanks,
Bart.
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
