From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752623AbdDIUQX (ORCPT ); Sun, 9 Apr 2017 16:16:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51286 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752537AbdDIUQP (ORCPT ); Sun, 9 Apr 2017 16:16:15 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 0FFDA85543 Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=aarcange@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 0FFDA85543 Date: Fri, 7 Apr 2017 18:48:58 +0200 From: Andrea Arcangeli To: Chris Wilson , Martin Kepplinger , Thorsten Leemhuis , daniel.vetter@intel.com, Dave Airlie , intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org Subject: Re: [PATCH 2/5] i915: flush gem obj freeing workqueues to add accuracy to the i915 shrinker Message-ID: <20170407164858.GF5035@redhat.com> References: <87pogtplxr.fsf@intel.com> <20170406232347.988-1-aarcange@redhat.com> <20170406232347.988-3-aarcange@redhat.com> <20170407100211.GG10496@nuc-i3427.alporthouse.com> <20170407130600.GA5035@redhat.com> <20170407153011.GR10496@nuc-i3427.alporthouse.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170407153011.GR10496@nuc-i3427.alporthouse.com> User-Agent: Mutt/1.8.0 (2017-02-23) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Sun, 09 Apr 2017 20:16:14 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Apr 07, 2017 at 04:30:11PM +0100, Chris Wilson wrote: > Not getting hangs is a good sign, but lockdep doesn't like it: > > [ 460.684901] WARNING: CPU: 1 PID: 172 at kernel/workqueue.c:2418 check_flush_dependency+0x92/0x130 > [ 460.684924] workqueue: PF_MEMALLOC task 172(kworker/1:1H) is flushing !WQ_MEM_RECLAIM events:__i915_gem_free_work [i915] > > If I allocated the workqueue with WQ_MEM_RELCAIM, it complains bitterly > as well. So in PF_MEMALLOC context we can't flush a workqueue with !WQ_MEM_RECLAIM. system_wq = alloc_workqueue("events", 0, 0); My point is synchronize_rcu_expedited will still push its work in the same system_wq workqueue... /* Marshall arguments & schedule the expedited grace period. */ rew.rew_func = func; rew.rew_rsp = rsp; rew.rew_s = s; INIT_WORK_ONSTACK(&rew.rew_work, wait_rcu_exp_gp); schedule_work(&rew.rew_work); It's also using schedule_work, so either the above is a false positive, or we've still a problem with synchronize_rcu_expedited, just a reproducible issue anymore after we stop running it under the struct_mutex. Even synchronize_sched will wait on the system_wq if synchronize_rcu_expedited has been issued in parallel by some other code, it's just there's no check for it because it's not invoking flush_work to wait. The deadlock happens if we flush_work() while holding any lock that may be taken by any of the workqueues that could be queued there. i915 makes sure to flush_work only if the struct_mutex was released (not my initial version) but we're not checking if any of the other system_wq workqueues could possibly taking a lock that may have been hold during the allocation that invoked reclaim, I suppose that is the problem left, but I don't see how flush_work is special about it, synchronize_rcu_expedited would still have the same issue no? (despite no lockdep warning) I suspect this means synchronize_rcu_expedited() is not usable in reclaim context and lockdep should warn if PF_MEMALLOC is set when synchronize_rcu_expedited/synchronize_sched/synchronize_rcu are called. Probably to fix this we should create a private workqueue for both RCU and i915 and stop sharing the system_wq with the rest of the system (and of course set WQ_MEM_RECLAIM in both workqueues). This makes sure when we call synchronize_rcu_expedited; flush_work from the shrinker, we won't risk waiting on other random work that may be taking locks that are hold by the code that invoked reclaim during an allocation. The macro bug of waiting on system_wq 100% of the time while always holding the struct_mutex is gone, but we need to perfect this further and stop using the system_wq for RCU and i915 shrinker work. > We do need it for shrinker_count as well. If we just report 0 objects, Yes the shrinker_count too. > the shrinker_scan callback will be skipped, iirc. All we do need it for > direct calls to i915_gem_shrink() which themselves may or may not be > underneath the struct_mutex at the time. Yes. > I don't follow, > > static inline struct task_struct *__mutex_owner(struct mutex *lock) > { > return (struct task_struct *)(atomic_long_read(&lock->owner) & ~0x07); > } > > The atomic read is equivalent to READ_ONCE(). What's broken here? (I > guess strict aliasing and pointer cast?) That was an oversight, atomic64_read does READ_ONCE internally (as it should of course being an atomic read). I didn't recall it uses atomic_long_read. Thanks, Andrea From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrea Arcangeli Subject: Re: [PATCH 2/5] i915: flush gem obj freeing workqueues to add accuracy to the i915 shrinker Date: Fri, 7 Apr 2017 18:48:58 +0200 Message-ID: <20170407164858.GF5035@redhat.com> References: <87pogtplxr.fsf@intel.com> <20170406232347.988-1-aarcange@redhat.com> <20170406232347.988-3-aarcange@redhat.com> <20170407100211.GG10496@nuc-i3427.alporthouse.com> <20170407130600.GA5035@redhat.com> <20170407153011.GR10496@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <20170407153011.GR10496@nuc-i3427.alporthouse.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Chris Wilson , Martin Kepplinger , Thorsten Leemhuis , daniel.vetter@intel.com, Dave Airlie , intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org T24gRnJpLCBBcHIgMDcsIDIwMTcgYXQgMDQ6MzA6MTFQTSArMDEwMCwgQ2hyaXMgV2lsc29uIHdy b3RlOgo+IE5vdCBnZXR0aW5nIGhhbmdzIGlzIGEgZ29vZCBzaWduLCBidXQgbG9ja2RlcCBkb2Vz bid0IGxpa2UgaXQ6Cj4gCj4gWyAgNDYwLjY4NDkwMV0gV0FSTklORzogQ1BVOiAxIFBJRDogMTcy IGF0IGtlcm5lbC93b3JrcXVldWUuYzoyNDE4IGNoZWNrX2ZsdXNoX2RlcGVuZGVuY3krMHg5Mi8w eDEzMAo+IFsgIDQ2MC42ODQ5MjRdIHdvcmtxdWV1ZTogUEZfTUVNQUxMT0MgdGFzayAxNzIoa3dv cmtlci8xOjFIKSBpcyBmbHVzaGluZyAhV1FfTUVNX1JFQ0xBSU0gZXZlbnRzOl9faTkxNV9nZW1f ZnJlZV93b3JrIFtpOTE1XQo+IAo+IElmIEkgYWxsb2NhdGVkIHRoZSB3b3JrcXVldWUgd2l0aCBX UV9NRU1fUkVMQ0FJTSwgaXQgY29tcGxhaW5zIGJpdHRlcmx5Cj4gYXMgd2VsbC4KClNvIGluIFBG X01FTUFMTE9DIGNvbnRleHQgd2UgY2FuJ3QgZmx1c2ggYSB3b3JrcXVldWUgd2l0aAohV1FfTUVN X1JFQ0xBSU0uCgoJc3lzdGVtX3dxID0gYWxsb2Nfd29ya3F1ZXVlKCJldmVudHMiLCAwLCAwKTsK Ck15IHBvaW50IGlzIHN5bmNocm9uaXplX3JjdV9leHBlZGl0ZWQgd2lsbCBzdGlsbCBwdXNoIGl0 cyB3b3JrIGluCnRoZSBzYW1lIHN5c3RlbV93cSB3b3JrcXVldWUuLi4KCgkJLyogTWFyc2hhbGwg YXJndW1lbnRzICYgc2NoZWR1bGUgdGhlIGV4cGVkaXRlZCBncmFjZSBwZXJpb2QuICovCgkJcmV3 LnJld19mdW5jID0gZnVuYzsKCQlyZXcucmV3X3JzcCA9IHJzcDsKCQlyZXcucmV3X3MgPSBzOwoJ CUlOSVRfV09SS19PTlNUQUNLKCZyZXcucmV3X3dvcmssIHdhaXRfcmN1X2V4cF9ncCk7CgkJc2No ZWR1bGVfd29yaygmcmV3LnJld193b3JrKTsKCkl0J3MgYWxzbyB1c2luZyBzY2hlZHVsZV93b3Jr LCBzbyBlaXRoZXIgdGhlIGFib3ZlIGlzIGEgZmFsc2UKcG9zaXRpdmUsIG9yIHdlJ3ZlIHN0aWxs IGEgcHJvYmxlbSB3aXRoIHN5bmNocm9uaXplX3JjdV9leHBlZGl0ZWQsCmp1c3QgYSByZXByb2R1 Y2libGUgaXNzdWUgYW55bW9yZSBhZnRlciB3ZSBzdG9wIHJ1bm5pbmcgaXQgdW5kZXIgdGhlCnN0 cnVjdF9tdXRleC4KCkV2ZW4gc3luY2hyb25pemVfc2NoZWQgd2lsbCB3YWl0IG9uIHRoZSBzeXN0 ZW1fd3EgaWYKc3luY2hyb25pemVfcmN1X2V4cGVkaXRlZCBoYXMgYmVlbiBpc3N1ZWQgaW4gcGFy YWxsZWwgYnkgc29tZSBvdGhlcgpjb2RlLCBpdCdzIGp1c3QgdGhlcmUncyBubyBjaGVjayBmb3Ig aXQgYmVjYXVzZSBpdCdzIG5vdCBpbnZva2luZwpmbHVzaF93b3JrIHRvIHdhaXQuCgpUaGUgZGVh ZGxvY2sgaGFwcGVucyBpZiB3ZSBmbHVzaF93b3JrKCkgd2hpbGUgaG9sZGluZyBhbnkgbG9jayB0 aGF0Cm1heSBiZSB0YWtlbiBieSBhbnkgb2YgdGhlIHdvcmtxdWV1ZXMgdGhhdCBjb3VsZCBiZSBx dWV1ZWQgdGhlcmUuIGk5MTUKbWFrZXMgc3VyZSB0byBmbHVzaF93b3JrIG9ubHkgaWYgdGhlIHN0 cnVjdF9tdXRleCB3YXMgcmVsZWFzZWQgKG5vdApteSBpbml0aWFsIHZlcnNpb24pIGJ1dCB3ZSdy ZSBub3QgY2hlY2tpbmcgaWYgYW55IG9mIHRoZSBvdGhlcgpzeXN0ZW1fd3Egd29ya3F1ZXVlcyBj b3VsZCBwb3NzaWJseSB0YWtpbmcgYSBsb2NrIHRoYXQgbWF5IGhhdmUgYmVlbgpob2xkIGR1cmlu ZyB0aGUgYWxsb2NhdGlvbiB0aGF0IGludm9rZWQgcmVjbGFpbSwgSSBzdXBwb3NlIHRoYXQgaXMg dGhlCnByb2JsZW0gbGVmdCwgYnV0IEkgZG9uJ3Qgc2VlIGhvdyBmbHVzaF93b3JrIGlzIHNwZWNp YWwgYWJvdXQgaXQsCnN5bmNocm9uaXplX3JjdV9leHBlZGl0ZWQgd291bGQgc3RpbGwgaGF2ZSB0 aGUgc2FtZSBpc3N1ZSBubz8gKGRlc3BpdGUKbm8gbG9ja2RlcCB3YXJuaW5nKQoKSSBzdXNwZWN0 IHRoaXMgbWVhbnMgc3luY2hyb25pemVfcmN1X2V4cGVkaXRlZCgpIGlzIG5vdCB1c2FibGUgaW4K cmVjbGFpbSBjb250ZXh0IGFuZCBsb2NrZGVwIHNob3VsZCB3YXJuIGlmIFBGX01FTUFMTE9DIGlz IHNldCB3aGVuCnN5bmNocm9uaXplX3JjdV9leHBlZGl0ZWQvc3luY2hyb25pemVfc2NoZWQvc3lu Y2hyb25pemVfcmN1IGFyZQpjYWxsZWQuCgpQcm9iYWJseSB0byBmaXggdGhpcyB3ZSBzaG91bGQg Y3JlYXRlIGEgcHJpdmF0ZSB3b3JrcXVldWUgZm9yIGJvdGggUkNVCmFuZCBpOTE1IGFuZCBzdG9w IHNoYXJpbmcgdGhlIHN5c3RlbV93cSB3aXRoIHRoZSByZXN0IG9mIHRoZSBzeXN0ZW0KKGFuZCBv ZiBjb3Vyc2Ugc2V0IFdRX01FTV9SRUNMQUlNIGluIGJvdGggd29ya3F1ZXVlcykuIFRoaXMgbWFr ZXMgc3VyZQp3aGVuIHdlIGNhbGwgc3luY2hyb25pemVfcmN1X2V4cGVkaXRlZDsgZmx1c2hfd29y ayBmcm9tIHRoZSBzaHJpbmtlciwKd2Ugd29uJ3QgcmlzayB3YWl0aW5nIG9uIG90aGVyIHJhbmRv bSB3b3JrIHRoYXQgbWF5IGJlIHRha2luZyBsb2Nrcwp0aGF0IGFyZSBob2xkIGJ5IHRoZSBjb2Rl IHRoYXQgaW52b2tlZCByZWNsYWltIGR1cmluZyBhbiBhbGxvY2F0aW9uLgoKVGhlIG1hY3JvIGJ1 ZyBvZiB3YWl0aW5nIG9uIHN5c3RlbV93cSAxMDAlIG9mIHRoZSB0aW1lIHdoaWxlIGFsd2F5cwpo b2xkaW5nIHRoZSBzdHJ1Y3RfbXV0ZXggaXMgZ29uZSwgYnV0IHdlIG5lZWQgdG8gcGVyZmVjdCB0 aGlzIGZ1cnRoZXIKYW5kIHN0b3AgdXNpbmcgdGhlIHN5c3RlbV93cSBmb3IgUkNVIGFuZCBpOTE1 IHNocmlua2VyIHdvcmsuCgo+IFdlIGRvIG5lZWQgaXQgZm9yIHNocmlua2VyX2NvdW50IGFzIHdl bGwuIElmIHdlIGp1c3QgcmVwb3J0IDAgb2JqZWN0cywKClllcyB0aGUgc2hyaW5rZXJfY291bnQg dG9vLgoKPiB0aGUgc2hyaW5rZXJfc2NhbiBjYWxsYmFjayB3aWxsIGJlIHNraXBwZWQsIGlpcmMu IEFsbCB3ZSBkbyBuZWVkIGl0IGZvcgo+IGRpcmVjdCBjYWxscyB0byBpOTE1X2dlbV9zaHJpbmso KSB3aGljaCB0aGVtc2VsdmVzIG1heSBvciBtYXkgbm90IGJlCj4gdW5kZXJuZWF0aCB0aGUgc3Ry dWN0X211dGV4IGF0IHRoZSB0aW1lLgoKWWVzLgoKPiBJIGRvbid0IGZvbGxvdywKPiAKPiBzdGF0 aWMgaW5saW5lIHN0cnVjdCB0YXNrX3N0cnVjdCAqX19tdXRleF9vd25lcihzdHJ1Y3QgbXV0ZXgg KmxvY2spCj4gewo+ICAgICAgICAgcmV0dXJuIChzdHJ1Y3QgdGFza19zdHJ1Y3QgKikoYXRvbWlj X2xvbmdfcmVhZCgmbG9jay0+b3duZXIpICYgfjB4MDcpOwo+IH0KPiAKPiBUaGUgYXRvbWljIHJl YWQgaXMgZXF1aXZhbGVudCB0byBSRUFEX09OQ0UoKS4gV2hhdCdzIGJyb2tlbiBoZXJlPyAoSQo+ IGd1ZXNzIHN0cmljdCBhbGlhc2luZyBhbmQgcG9pbnRlciBjYXN0PykKClRoYXQgd2FzIGFuIG92 ZXJzaWdodCwgYXRvbWljNjRfcmVhZCBkb2VzIFJFQURfT05DRSBpbnRlcm5hbGx5IChhcyBpdApz aG91bGQgb2YgY291cnNlIGJlaW5nIGFuIGF0b21pYyByZWFkKS4gSSBkaWRuJ3QgcmVjYWxsIGl0 IHVzZXMKYXRvbWljX2xvbmdfcmVhZC4KClRoYW5rcywKQW5kcmVhCl9fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJp LWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9y Zy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo=