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@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Reply via email to