Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754174Ab3ILPpo (ORCPT ); Thu, 12 Sep 2013 11:45:44 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:47476 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753181Ab3ILPpn (ORCPT ); Thu, 12 Sep 2013 11:45:43 -0400 Message-ID: <5231E18D.7070306@canonical.com> Date: Thu, 12 Sep 2013 17:45:17 +0200 From: Maarten Lankhorst User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130804 Thunderbird/17.0.8 MIME-Version: 1.0 To: Daniel Vetter CC: Peter Zijlstra , Dave Airlie , Thomas Hellstrom , intel-gfx , dri-devel , Linux Kernel Mailing List , Ingo Molnar , Thomas Gleixner Subject: Re: [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE References: <20130912150645.GZ31370@twins.programming.kicks-ass.net> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3211 Lines: 64 Op 12-09-13 17:36, Daniel Vetter schreef: > On Thu, Sep 12, 2013 at 5:06 PM, Peter Zijlstra wrote: >> So I'm poking around the preemption code and stumbled upon: >> >> drivers/gpu/drm/i915/i915_gem.c: set_need_resched(); >> drivers/gpu/drm/ttm/ttm_bo_vm.c: set_need_resched(); >> drivers/gpu/drm/ttm/ttm_bo_vm.c: set_need_resched(); >> drivers/gpu/drm/udl/udl_gem.c: set_need_resched(); >> >> All these sites basically do: >> >> while (!trylock()) >> yield(); >> >> which is a horrible and broken locking pattern. >> >> Firstly its deadlock prone, suppose the faulting process is a FIFOn+1 >> task that preempted the lock holder at FIFOn. >> >> Secondly the implementation is worse than usual by abusing >> VM_FAULT_NOPAGE, which is supposed to install a PTE so that the fault >> doesn't retry, but you're using it as a get out of fault path. And >> you're using set_need_resched() which is not something a driver should >> _ever_ touch. >> >> Now I'm going to take away set_need_resched() -- and while you can >> 'reimplement' it using set_thread_flag() you're not going to do that >> because it will be broken due to changes to the preempt code. >> >> So please as to fix ASAP and don't allow anybody to trick you into >> merging silly things like that again ;-) > The set_need_resched in i915_gem.c:i915_gem_fault can actually be > removed. It was there to give the error handler a chance to sneak in > and reset the hw/sw tracking when the gpu is dead. That hack goes back > to the days when the locking around our error handler was somewhere > between nonexistent and totally broken, nowadays we keep things from > live-locking by a bit of magic in i915_mutex_lock_interruptible. I'll > whip up a patch to rip this out. I'll also check that our testsuite > properly exercises this path (needs a bit of work on a quick look for > better coverage). > > The one in ttm is just bonghits to shut up lockdep: ttm can recurse > into it's own pagefault handler and then deadlock, the trylock just > keeps lockdep quiet. We've had that bug arise in drm/i915 due to some > fun userspace did and now have testcases for them. The right solution > to fix this is to use copy_to|from_user_atomic in ttm everywhere it > holds locks and have slowpaths which drops locks, copies stuff into a > temp allocation and then continues. At least that's how we've fixed > all those inversions in i915-gem. I'm not volunteering to fix this ;-) Ah the case where a mmap'd address is passed to the execbuf ioctl? :P Fine I'll look into it a bit, hopefully before tuesday. Else it might take a bit longer since I'll be on my way to plumbers.. > The one in udl just looks like copypasta from i915, without any > justification (at least I don't see any) for why it's there. Probably > can die, too, since there isn't any gpu to reset on usb display-link > devices ... > > Cheers, Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/