Re: [PATCH] zram: fix possible race when checking idle_strm

Joonsoo Kim
2015-08-11T08:20:51Z

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.