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

[Joonsoo Kim]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/