From patchwork Fri Aug 9 15:31:00 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Oleg Nesterov X-Patchwork-Id: 2842510 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id A76FFBF546 for ; Sat, 10 Aug 2013 18:17:43 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id BA1B22027F for ; Sat, 10 Aug 2013 18:17:42 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id B22EC20151 for ; Sat, 10 Aug 2013 18:17:41 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id A0C1AE675E for ; Sat, 10 Aug 2013 11:17:41 -0700 (PDT) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by gabe.freedesktop.org (Postfix) with ESMTP id 7F1BFE6124 for ; Fri, 9 Aug 2013 08:36:47 -0700 (PDT) Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r79FaTtI018414 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 9 Aug 2013 11:36:29 -0400 Received: from tranklukator.brq.redhat.com (dhcp-1-192.brq.redhat.com [10.34.1.192]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with SMTP id r79FaQbF025129; Fri, 9 Aug 2013 11:36:27 -0400 Received: by tranklukator.brq.redhat.com (nbSMTP-1.00) for uid 500 oleg@redhat.com; Fri, 9 Aug 2013 17:31:02 +0200 (CEST) Date: Fri, 9 Aug 2013 17:31:00 +0200 From: Oleg Nesterov To: Linus Torvalds , Dave Airlie Subject: block_all_signals() must die (Was: Patch for lost wakeups) Message-ID: <20130809153100.GA4968@redhat.com> References: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.18 (2008-05-17) X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-Mailman-Approved-At: Sat, 10 Aug 2013 11:06:26 -0700 Cc: dri-devel@lists.freedesktop.org, Andrew Morton , Al Viro , Long Gao , Linux Kernel Mailing List X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org Errors-To: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP And sorry for off-topic email, but I can't resist. Can't we finally kill block_all_signals() and ->notifier ? This is very, very wrong and doesn't work anyway. I tried to ask many, many times. Starting from 2007 at least. And every time the discussion "hangs". I am quoting the last email I sent below. Dave, your reply was: I'm on holidays for another week or so, maybe once I get back I'll find some time to figure out how it works vs what happens, but really I suspect we can kill this with fire. So perhaps I should simply send the patch with your ack? ;) Oleg. From oleg@redhat.com Tue Jul 12 20:15:36 2011 Date: Tue, 12 Jul 2011 20:15:36 +0200 Hello. I tried many times to ask about the supposed behaviour of block_all_signals() in drm, but it seems nobody can answer. So I am going to send the patch which simply removes block_all_signals() and friends. There are numeruous problems with this interace, I can't even enumerate them. But I think that it is enough to mention that block_all_signals() simply can not work. AT ALL. I am wondering, was it ever tested and how. So. ioctl()->drm_lock() "blocks" the stop signals. Probably to ensure the task can't be stopped until it does DRM_IOCTL_UNLOCK. And what does this mean? Yes, the task won't stop if it receives, say, SIGTSTP. But! Instead it will loop forever in kernel mode until it receives another unblocked/non-ignored signal which should be numerically less than SIGSTOP. Why do we need this? Once again. block_all_signals(SIGTSTP) only means that the caller will burn cpu instead of sleeping in TASK_STOPPED after ^Z. What is the point? And once again, there are other problems. For example, even if block_all_signals() actually blocked SIGSTOP/etc, this can not help if the caller is multithreaded. I strongly believe block_all_signals() should die. Given that it doesn't work, could somebody please explain me what will be broken? Just in case... Please look at the debugging patch below. With this patch, $ perl -le 'syscall 157,666 and die $!; sleep 1, print while ++$_' 1 2 3 ^Z Hang. So it does react to ^Z anyway, just it is looping in the endless loop in the kernel. It can only look as if ^Z is ignored, because obviously bash doesn't see it stopped. Now lets look at drm_notifier(). If it returns 0 it does: /* Otherwise, set flag to force call to drmUnlock */ drmUnlock? grep shows nothing... do { old = s->lock->lock; new = old | _DRM_LOCK_CONT; prev = cmpxchg(&s->lock->lock, old, new); } while (prev != old); return 0; OK. So, if block_all_signals() makes any sense, it seems that this is only because we add _DRM_LOCK_CONT. Who checks _DRM_LOCK_CONT? _DRM_LOCK_IS_CONT(), but it has no users. Hmm. Looks like via_release_futex() is the only user, but it doesn't look as "force call to drmUnlock" and it is CONFIG_DRM_VIA only. I am totally confused. But block_all_signals() should die anyway. We can probably implement something like 'i-am-going-to-stop' or even 'can-i-stop' per-thread notifiers, although this all looks like the user-space problem to me (yes, I know absolutely nothing about drm/etc). If nothing else. We can change drm_lock/drm_unlock to literally block/unblock SIGSTOP/etc (or perhaps we only should worry about the signals from tty?). This is the awful hack and this can't work with the multithreaded tasks too, but still it is better than what we have now. Oleg. --- a/kernel/sys.c~ 2011-06-16 20:12:18.000000000 +0200 +++ b/kernel/sys.c 2011-07-12 16:24:50.000000000 +0200 @@ -1614,6 +1614,11 @@ SYSCALL_DEFINE1(umask, int, mask) return mask; } +static int notifier(void *arg) +{ + return 0; +} + SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, unsigned long, arg4, unsigned long, arg5) { @@ -1627,6 +1632,13 @@ SYSCALL_DEFINE5(prctl, int, option, unsi error = 0; switch (option) { + case 666: { + sigset_t *pmask = kmalloc(sizeof(*pmask), GFP_KERNEL); + siginitset(pmask, sigmask(SIGTSTP)); + block_all_signals(notifier, NULL, pmask); + break; + } + case PR_SET_PDEATHSIG: if (!valid_signal(arg2)) { error = -EINVAL;