Date: Tue, 11 Aug 2015 17:26:24 +0900
From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
To: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
Cc: 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: <20150811082623.GB351@js1304-P5Q-DELUXE>
References: <1438934609-16924-1-git-send-email-iamjoonsoo.kim@lge.com>
 <20150807091457.GL1891@swordfish>
 <20150807095816.GP1891@swordfish>
 <20150810003229.GA26074@js1304-P5Q-DELUXE>
 <20150810021659.GD645@swordfish>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20150810021659.GD645@swordfish>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3663
Lines: 79

On Mon, Aug 10, 2015 at 11:16:59AM +0900, Sergey Senozhatsky wrote:
> 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.

>From Minchan's comment, I think that race is not possible.
I will drop the patch.

Thanks.
--
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/