Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751657AbbHGJOa (ORCPT ); Fri, 7 Aug 2015 05:14:30 -0400 Received: from mail-pd0-f169.google.com ([209.85.192.169]:36328 "EHLO mail-pd0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750809AbbHGJOW (ORCPT ); Fri, 7 Aug 2015 05:14:22 -0400 Date: Fri, 7 Aug 2015 18:14:57 +0900 From: Sergey Senozhatsky To: Joonsoo Kim Cc: Andrew Morton , Minchan Kim , Nitin Gupta , Sergey Senozhatsky , linux-kernel@vger.kernel.org, Joonsoo Kim Subject: Re: [PATCH] zram: fix possible race when checking idle_strm Message-ID: <20150807091457.GL1891@swordfish> References: <1438934609-16924-1-git-send-email-iamjoonsoo.kim@lge.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1438934609-16924-1-git-send-email-iamjoonsoo.kim@lge.com> 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: 2254 Lines: 61 On (08/07/15 17:03), Joonsoo Kim wrote: > Currently, when we enter the wait state due to lack of idle stream, > we check idle_strm list without holding the lock in expanding of > wait_event define. In this case, some one can see stale value and > process could fall into wait state without any upcoming wakeup process. hm... I need to think about it more. we do wake_up every time we put stream back to the list zcomp_strm_multi_release(): spin_lock(&zs->strm_lock); if (zs->avail_strm <= zs->max_strm) { list_add(&zstrm->list, &zs->idle_strm); spin_unlock(&zs->strm_lock); wake_up(&zs->strm_wait); return; } but I can probably see what you mean... in some very extreme case, though. I can't even formulate it... eh... we use a multi stream backend with ->max_strm == 1 and there are two processes, one just falsely passed the wait_event() `if (condition)' check, the other one just put stream back to ->idle_strm and called wake_up(), but the first process hasn't yet executed prepare_to_wait_event() so it might miss a wakeup. and there should be no other process doing read or write operation. otherwise, there will be wakeup eventually. is this the case you were thinking of?... then yes, this spinlock may help. hm... we also can simply forbid downgrading a multi stream backend to a single stream (in terms of performance it is much slower than a real single steam anyway). will this do the trick? if we have more than 1 stream and idle list then there will be more that one wakeup. and every woken up task will call wakeup once it put stream. I don't think I'll be able to check the email until this Sunday, so here is my Acked-by: Sergey Senozhatsky to this patch if Minchan decides that `forbid downgrading` is hacky or doesn't work. very nice finding, Joonsoo! p.s. hm... we can take a look at 'forbid downgrading a multi stream backend to a single-streamed'. -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/