[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20090605162553.GC6778@linux.vnet.ibm.com>
Date: Fri, 5 Jun 2009 09:25:53 -0700
From: "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To: Rusty Russell <rusty@...tcorp.com.au>
Cc: Gregory Haskins <gregory.haskins@...il.com>,
"Michael S. Tsirkin" <mst@...hat.com>, Avi Kivity <avi@...hat.com>,
Gregory Haskins <ghaskins@...ell.com>,
linux-kernel@...r.kernel.org, agraf@...e.de, pmullaney@...ell.com,
pmorreale@...ell.com, anthony@...emonkey.ws,
netdev@...r.kernel.org, kvm@...r.kernel.org,
bhutchings@...arflare.com, andi@...stfloor.org, gregkh@...e.de,
herber@...dor.apana.org.au, chrisw@...s-sol.org,
shemminger@...tta.com
Subject: Re: [RFC PATCH v2 00/19] virtual-bus
On Sat, Jun 06, 2009 at 12:25:57AM +0930, Rusty Russell wrote:
> On Fri, 5 Jun 2009 03:00:10 pm Paul E. McKenney wrote:
> > On Fri, Jun 05, 2009 at 02:25:01PM +0930, Rusty Russell wrote:
> > > + /* lg->eventfds is RCU-protected */
> > > + preempt_disable();
> >
> > Suggest changing to rcu_read_lock() to match the synchronize_rcu().
>
> Ah yes, much better. As I was implementing it I warred with myself since
> lguest aims for simplicity above all else. But since we only ever add things
> to the array, RCU probably is simpler.
;-)
> > > + for (i = 0; i < cpu->lg->num_eventfds; i++) {
> > > + if (cpu->lg->eventfds[i].addr == cpu->pending_notify) {
> > > + eventfd_signal(cpu->lg->eventfds[i].event, 1);
> >
> > Shouldn't this be something like the following?
> >
> > p = rcu_dereference(cpu->lg->eventfds);
> > if (p[i].addr == cpu->pending_notify) {
> > eventfd_signal(p[i].event, 1);
>
> Hmm, need to read num_eventfds first, too. It doesn't matter if we get the old
> ->num_eventfds and the new ->eventfds, but the other way around would be bad.
Yep!!! ;-)
> Here's the inter-diff:
>
> diff --git a/drivers/lguest/lguest_user.c b/drivers/lguest/lguest_user.c
> --- a/drivers/lguest/lguest_user.c
> +++ b/drivers/lguest/lguest_user.c
> @@ -39,18 +39,24 @@ static int break_guest_out(struct lg_cpu
>
> bool send_notify_to_eventfd(struct lg_cpu *cpu)
> {
> - unsigned int i;
> + unsigned int i, num;
> + struct lg_eventfds *eventfds;
> +
> + /* Make sure we grab the total number before accessing the array. */
> + cpu->lg->num_eventfds = num;
> + rmb();
>
> /* lg->eventfds is RCU-protected */
> rcu_read_lock();
> - for (i = 0; i < cpu->lg->num_eventfds; i++) {
> - if (cpu->lg->eventfds[i].addr == cpu->pending_notify) {
> - eventfd_signal(cpu->lg->eventfds[i].event, 1);
> + eventfds = rcu_dereference(cpu->lg->eventfds);
> + for (i = 0; i < num; i++) {
> + if (eventfds[i].addr == cpu->pending_notify) {
> + eventfd_signal(eventfds[i].event, 1);
> cpu->pending_notify = 0;
> break;
> }
> }
> - preempt_enable();
> + rcu_read_unlock();
> return cpu->pending_notify == 0;
> }
It is possible to get rid of the rmb() and wmb() as well, doing
something like the following:
struct lg_eventfds_num {
unsigned int n;
struct lg_eventfds a[0];
}
Then the rcu_dereference() gets you a pointer to a struct lg_eventfds_num,
which has the array and its length in guaranteed synchronization without
the need for barriers.
Does this work for you, or is there some complication that I am missing?
Thanx, Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists