Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S942005AbcJYRei (ORCPT ); Tue, 25 Oct 2016 13:34:38 -0400 Received: from mail-yw0-f182.google.com ([209.85.161.182]:35326 "EHLO mail-yw0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S941943AbcJYReg (ORCPT ); Tue, 25 Oct 2016 13:34:36 -0400 MIME-Version: 1.0 In-Reply-To: <20161025151639.GE4326@redhat.com> References: <20161025110525.9100-1-roman.penyaev@profitbricks.com> <20161025151639.GE4326@redhat.com> From: Roman Penyaev Date: Tue, 25 Oct 2016 19:34:15 +0200 Message-ID: Subject: Re: [PATCH 1/1] kthread: fix possible infinite wait for parking when kthread exits meanwhile To: Oleg Nesterov Cc: Andy Lutomirski , Peter Zijlstra , Thomas Gleixner , Ingo Molnar , Tejun Heo , linux-kernel@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2830 Lines: 74 On Tue, Oct 25, 2016 at 5:16 PM, Oleg Nesterov 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