Re: [PATCH 1/1] kthread: fix possible infinite wait for parking when kthread exits meanwhile

[Roman Penyaev]

On Tue, Oct 25, 2016 at 5:16 PM, Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> Well. I was going to ignore this patch, I will leave this to Thomas
> anyway...
>
> But can't resist, because I have to admit I dislike this one too ;)
>
> On 10/25, Roman Pen wrote:
>>
>>  int kthread_park(struct task_struct *k)
>>  {
>> -     struct kthread *kthread = to_live_kthread_and_get(k);
>> +     struct kthread *kthread;
>>       int ret = -ENOSYS;
>>
>> -     if (kthread) {
>> -             if (!test_bit(KTHREAD_IS_PARKED, &kthread->flags)) {
>> +     /*
>> +      * Here we try to be careful and handle the case, when kthread
>> +      * is going to die and will never park.
>
> But why?
>
> IMO we should not complicate this code for the case when the kernel
> thread crashes.

that is not only about crashes, or let's say this is *only* about exit
of a thread.  exit, which asynchronously happens.  the kthread dies or
you have a bug in your kthread, say, in handling some pending signals,
or whatever,  but the result is the same: the thread exits before you
have invoked kthread_park(), then you call kthread_park() and you hang
forever.

so the complication for me comes from the fact, that kthread.h is an
API, which should provide some guarantees and if it does not do that,
at least the API should be explicit on that and I (as a user) must
take care.

e.g. kthread_data() says 'The caller is responsible for ensuring the
validity of @task when calling this function.'. That is clear.  As a
user of the kthread API I can do it myself, doing get_task_struct().

but with kthread_park() call I can't do anything.  I can rely only on
the fact, that my thread is still running.

I understand, that kthread_park() is used only in special cases, I've
counted only 5 callers, which are probably completely aware of what
they are doing.  so of course it does not make a lot of sense even to
touch that code.  but I did not like that particular place, which is
easy to fix with rather small and painless changes.

>
>> +     do {
>> +             kthread = to_live_kthread_and_get(k);
>> +             if (!kthread)
>> +                     break;
>> +             if (!__kthread_isparked(kthread)) {
>>                       set_bit(KTHREAD_SHOULD_PARK, &kthread->flags);
>>                       if (k != current) {
>>                               wake_up_process(k);
>>                               wait_for_completion(&kthread->parked);
>>                       }
>>               }
>> +             if (k == current || __kthread_isparked(kthread))
>> +                     /* The way out */
>> +                     ret = 0;
>
> And why do we need to restart if __kthread_isparked() is false? In this
> case we know that we were woken up by the exiting kthread which won't
> park anyway.

That's true.  Loop is an overkill.

--
Roman