Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752794AbbHJCQ2 (ORCPT ); Sun, 9 Aug 2015 22:16:28 -0400 Received: from mail-pa0-f45.google.com ([209.85.220.45]:33103 "EHLO mail-pa0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752305AbbHJCQZ (ORCPT ); Sun, 9 Aug 2015 22:16:25 -0400 Date: Mon, 10 Aug 2015 11:16:59 +0900 From: Sergey Senozhatsky To: Joonsoo Kim Cc: Sergey Senozhatsky , Andrew Morton , Minchan Kim , Nitin Gupta , linux-kernel@vger.kernel.org Subject: Re: [PATCH] zram: fix possible race when checking idle_strm Message-ID: <20150810021659.GD645@swordfish> References: <1438934609-16924-1-git-send-email-iamjoonsoo.kim@lge.com> <20150807091457.GL1891@swordfish> <20150807095816.GP1891@swordfish> <20150810003229.GA26074@js1304-P5Q-DELUXE> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150810003229.GA26074@js1304-P5Q-DELUXE> User-Agent: Mutt/1.5.23+102 (2ca89bed6448) (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4030 Lines: 96 Hello Joonsoo, On (08/10/15 09:32), Joonsoo Kim wrote: > > on the other hand... it's actually > > > > wait_event() is > > > > if (condition) > > break; > > prepare_to_wait_event(&wq, &__wait, state) > > if (condition) > > break; > > schedule(); > > > > if first condition check was false and we missed a wakeup call between > > first condition and prepare_to_wait_event(), then second condition > > check should do the trick I think (or you expect that second condition > > check may be wrongly pre-fetched or something). > ... > I expected that second condition can be false if compiler reuse result > of first check for optimization. I guess that there is no prevention > for this kind of optimization. hm... so we have outer and inner checks (out of loop and inside of loop). can compiler decide that outer and inner checks are equivalent here? #define wait_event(wq, condition) \ do { \ might_sleep(); \ if (condition) \ break; \ .... for (;;) { \ long __int = prepare_to_wait_event(&wq, &__wait, state);\ \ if (condition) \ break; \ \ if (___wait_is_interruptible(state) && __int) { \ __ret = __int; \ if (exclusive) { \ abort_exclusive_wait(&wq, &__wait, \ state, NULL); \ goto __out; \ } \ break; \ } \ \ cmd; \ } \ .... } while (0) I probably don't have enough knowledge about compilers; but I think it must keep two checks. But I may be wrong. just out of curiosity, a quick grep wait_event(zatm_vcc->tx_wait, !skb_peek(&zatm_vcc->tx_queue)) wait_event(pmu->recv.wait, (pmu->recv.process == 0)) wait_event(ep->com.waitq, ep->com.rpl_done) wait_event(cs->waitqueue, !cs->waiting) wait_event(resync_wait, (mddev->sync_thread == NULL &&... wait_event(mddev->sb_wait, mddev->flags == 0 ||... and so on. -ss > So, following is the problem sequence I thought. > T1 means thread 1, T2 means another thread, 2. > > check if idle_strm is empty or not with holding the lock > It is so do spin_unlock and run wait_event macro > check if idle_strm is empty or not > It is still empty > > do strm release > call wake_up > > add T1 to wait queue > check if idle_strm is empty or not > compiler reuse 's result or CPU just fetch cached > result so T1 starts waiting > > In this case, T1 can be sleep permanently. To prevent compiler > optimization or fetching cached value, we need a lock here. -ss -- 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/