Date: Mon, 10 Aug 2015 11:16:59 +0900
From: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
To: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Minchan Kim <minchan@kernel.org>, Nitin Gupta <ngupta@vflare.org>,
        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
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.
> 
> <T1-1> check if idle_strm is empty or not with holding the lock
> <T1-2> It is  so do spin_unlock and run wait_event macro
> <T1-3> check if idle_strm is empty or not
> <T1-4> It is still empty
> 
> <T2-1> do strm release
> <T2-2> call wake_up
> 
> <T1-5> add T1 to wait queue
> <T1-6> check if idle_strm is empty or not
> <T1-7> compiler reuse <T1-4>'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/