From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sasha Levin Subject: Re: [PATCH v2 6/8] kvm tools: Add rwlock wrapper Date: Mon, 30 May 2011 13:41:58 +0300 Message-ID: <1306752118.14564.87.camel@lappy> References: <1306744247-26051-6-git-send-email-levinsasha928@gmail.com> <20110530084309.GH30513@elte.hu> <1306748069.14564.52.camel@lappy> <1306748796.14564.62.camel@lappy> <20110530095645.GC8461@elte.hu> <1306749934.14564.71.camel@lappy> <20110530101333.GB17821@elte.hu> <1306750963.14564.79.camel@lappy> <20110530103020.GD17821@elte.hu> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: Pekka Enberg , kvm@vger.kernel.org, asias.hejun@gmail.com, gorcunov@gmail.com, prasadjoshi124@gmail.com, "Paul E. McKenney" To: Ingo Molnar Return-path: Received: from mail-ww0-f44.google.com ([74.125.82.44]:58342 "EHLO mail-ww0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751354Ab1E3Km1 (ORCPT ); Mon, 30 May 2011 06:42:27 -0400 Received: by wwa36 with SMTP id 36so3780456wwa.1 for ; Mon, 30 May 2011 03:42:26 -0700 (PDT) In-Reply-To: <20110530103020.GD17821@elte.hu> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, 2011-05-30 at 12:30 +0200, Ingo Molnar wrote: > * Sasha Levin wrote: > > > On Mon, 2011-05-30 at 12:13 +0200, Ingo Molnar wrote: > > > * Sasha Levin wrote: > > > > > > > On Mon, 2011-05-30 at 11:56 +0200, Ingo Molnar wrote: > > > > > * Sasha Levin wrote: > > > > > > > > > > > I'm just saying that we're limited to as many VCPU threads as we > > > > > > can create. br_read_lock() won't do anything on a non-VCPU thread, > > > > > > which makes it impossible to test it on non-VCPUs. > > > > > > > > > > btw., i wondered about that limit - don't we want to fix it? > > > > > > > > > > I mean, there's no fundamental reason why brlocks should do 'nothing' > > > > > in worker threads. In fact it's a subtle breakage waiting AFAICS. > > > > > > > > Can they do anything useful without locking? I think we should work > > > > on integrating an RCU and changing brlocks to use that instead of > > > > focusing too much on the current implementation. > > > > > > What do you mean 'without locking'? If a worker thread uses a > > > br_read_lock() then that will be 'locking'. It should map to a real > > > read_lock() in the rwlock debug case, etc. > > > > > I meant without locking anything within br_read_lock(), because we > > wanted to keep the read patch lock-free. > > oh, so it's not recursive. > > Sane enough - might be worth adding: > > br_is_read_locked(&lock) > > and a debug check for that into br_read_lock(): > > BUG_ON(br_is_read_locked(&lock)); > > > > > This will also fix that limit you don't like :) > > > > > > I'd prefer brlocks to more complex solutions in cases where the write > > > path is very infrequent! > > > > > > So we don't want to keep brlocks intentionally crippled. > > > > Do you see brlock as a global lock that will pause the entire guest > > (not just VCPUs - anything except the calling thread)? > > Yeah, that's how such brlocks work - life has to stop when there's > write modifications going on. > > There should be a mutex around br_write_lock() itself, to make sure > two br_write_lock() attempts cannot deadlock each other, but other > than that it should be pretty straightforward and robust. > Yes, It'll need to wait for a thread manager to be added so it could be un-crippled. > And note that such a pause/suspend thing might be helpful to do a > *real* host driven suspend feature in the future: stop all vcpus, all > worker threads, save state to disk and exit? > > Thanks, > > Ingo -- Sasha.