From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ingo Molnar Subject: Re: [PATCH 3/4] kvm tools: Add a brlock Date: Sun, 29 May 2011 21:38:25 +0200 Message-ID: <20110529193825.GA13539@elte.hu> References: <1306690348-23260-1-git-send-email-levinsasha928@gmail.com> <1306690348-23260-3-git-send-email-levinsasha928@gmail.com> <20110529184731.GB9835@elte.hu> <1306697458.14564.22.camel@lappy> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: penberg@kernel.org, avi@redhat.com, kvm@vger.kernel.org, asias.hejun@gmail.com, gorcunov@gmail.com, prasadjoshi124@gmail.com To: Sasha Levin Return-path: Received: from mx3.mail.elte.hu ([157.181.1.138]:53844 "EHLO mx3.mail.elte.hu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753070Ab1E2Tih (ORCPT ); Sun, 29 May 2011 15:38:37 -0400 Content-Disposition: inline In-Reply-To: <1306697458.14564.22.camel@lappy> Sender: kvm-owner@vger.kernel.org List-ID: * Sasha Levin wrote: > On Sun, 2011-05-29 at 20:47 +0200, Ingo Molnar wrote: > > * Sasha Levin wrote: > > > > > +++ b/tools/kvm/include/kvm/brlock.h > > > @@ -0,0 +1,12 @@ > > > +#ifndef KVM__BRLOCK_H > > > +#define KVM__BRLOCK_H > > > + > > > +#include "kvm/kvm.h" > > > +#include "kvm/barrier.h" > > > + > > > +#define br_read_lock() mb() > > > +#define br_read_unlock() mb() > > > > These only need to be compiler barrier()s AFAICS, because the 'pause' > > op will signal back to the requestor thread - which whole operation > > is a barrier to begin with. > > I'm wondering why we need a barrier here at all. In this brlock > implementation the readers are waiting on a mutex in their main > loop - right before a call to KVM_RUN. They can't get anywhere near > a br_read_lock() once br_write_lock() has completed. Yes - and i alluded to that in one of my previous mails - but i think we should do a barrier() just to make sure people use them ;-) We don't want huge sections of code assuming readonly data structures. We should probably also add a debug variant that switches this all to rwlocks: that way the correctness of the critical sections can be tested. 5 years down the line we do not want to end up with another 'BKL' kind of situation. > > > +#define br_write_lock() kvm__pause() > > > +#define br_write_unlock() kvm__continue() > > > +#endif > > > > Btw., it might make sense to add a comment to this header file, > > explaining what a 'big reader lock' is :-) > > I'll put the commit message into the header, should be enough? Yeah, that should be more than enough! Thanks, Ingo