From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
To: Rusty Russell <rusty@rustcorp.com.au>
Cc: Gregory Haskins <gregory.haskins@gmail.com>,
"Michael S. Tsirkin" <mst@redhat.com>,
Avi Kivity <avi@redhat.com>,
Gregory Haskins <ghaskins@novell.com>,
linux-kernel@vger.kernel.org, agraf@suse.de,
pmullaney@novell.com, pmorreale@novell.com,
anthony@codemonkey.ws, netdev@vger.kernel.org,
kvm@vger.kernel.org, bhutchings@solarflare.com,
andi@firstfloor.org, gregkh@suse.de, herber@gondor.apana.org.au,
chrisw@sous-sol.org, shemminger@vyatta.com
Subject: Re: [RFC PATCH v2 00/19] virtual-bus
Date: Fri, 5 Jun 2009 09:25:53 -0700 [thread overview]
Message-ID: <20090605162553.GC6778@linux.vnet.ibm.com> (raw)
In-Reply-To: <200906060025.57961.rusty@rustcorp.com.au>
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
next prev parent reply other threads:[~2009-06-05 16:26 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-04-09 16:30 [RFC PATCH v2 00/19] virtual-bus Gregory Haskins
2009-04-09 16:30 ` [RFC PATCH v2 01/19] shm-signal: shared-memory signals Gregory Haskins
2009-04-09 16:30 ` [RFC PATCH v2 02/19] vbus: add virtual-bus definitions Gregory Haskins
2009-04-09 16:30 ` [RFC PATCH v2 03/19] vbus: add connection-client helper infrastructure Gregory Haskins
2009-06-04 18:06 ` Michael S. Tsirkin
2009-06-04 18:18 ` Gregory Haskins
2009-06-04 18:24 ` Avi Kivity
2009-06-04 18:30 ` Gregory Haskins
2009-06-04 19:04 ` Avi Kivity
2009-06-04 18:23 ` Avi Kivity
2009-04-09 16:31 ` [RFC PATCH v2 04/19] vbus: add bus-registration notifiers Gregory Haskins
2009-04-09 16:31 ` [RFC PATCH v2 05/19] vbus: add a "vbus-proxy" bus model for vbus_driver objects Gregory Haskins
2009-04-09 16:31 ` [RFC PATCH v2 06/19] ioq: Add basic definitions for a shared-memory, lockless queue Gregory Haskins
2009-04-09 16:31 ` [RFC PATCH v2 07/19] ioq: add vbus helpers Gregory Haskins
2009-04-09 16:31 ` [RFC PATCH v2 08/19] venet: add the ABI definitions for an 802.x packet interface Gregory Haskins
2009-04-09 16:31 ` [RFC PATCH v2 09/19] net: Add vbus_enet driver Gregory Haskins
2009-04-09 16:37 ` Stephen Hemminger
2009-04-09 19:50 ` Greg KH
2009-04-09 16:31 ` [RFC PATCH v2 10/19] venet-tap: Adds a "venet" compatible "tap" device to VBUS Gregory Haskins
2009-04-09 16:31 ` [RFC PATCH v2 11/19] venet: add scatter-gather support Gregory Haskins
2009-04-09 16:31 ` [RFC PATCH v2 12/19] venettap: " Gregory Haskins
2009-04-09 16:31 ` [RFC PATCH v2 13/19] x86: allow the irq->vector translation to be determined outside of ioapic Gregory Haskins
2009-04-09 16:31 ` [RFC PATCH v2 14/19] kvm: add a reset capability Gregory Haskins
2009-04-11 16:56 ` Avi Kivity
2009-04-09 16:32 ` [RFC PATCH v2 15/19] kvm: add dynamic IRQ support Gregory Haskins
2009-04-11 17:01 ` Avi Kivity
2009-04-13 17:44 ` Gregory Haskins
2009-04-09 16:32 ` [RFC PATCH v2 16/19] kvm: Add VBUS support to the host Gregory Haskins
2009-04-09 16:32 ` [RFC PATCH v2 17/19] kvm: Add guest-side support for VBUS Gregory Haskins
2009-04-09 16:32 ` [RFC PATCH v2 18/19] vbus: add a userspace connector Gregory Haskins
2009-04-09 16:32 ` [RFC PATCH v2 19/19] virtio: add a vbus transport Gregory Haskins
2009-08-09 16:40 ` Anthony Liguori
2009-08-10 15:40 ` Gregory Haskins
2009-04-09 16:48 ` [RFC PATCH v2 00/19] virtual-bus Gregory Haskins
2009-04-11 16:45 ` Avi Kivity
2009-06-04 18:49 ` Gregory Haskins
2009-06-05 4:55 ` Rusty Russell
2009-06-05 5:30 ` Paul E. McKenney
2009-06-05 14:55 ` Rusty Russell
2009-06-05 16:25 ` Paul E. McKenney [this message]
2009-06-11 13:21 ` Rusty Russell
2009-06-11 15:48 ` Paul E. McKenney
2009-06-05 11:56 ` Gregory Haskins
2009-06-05 12:53 ` Avi Kivity
2009-06-05 12:54 ` Gregory Haskins
2009-06-05 14:35 ` Rusty Russell
2009-06-05 14:44 ` Gregory Haskins
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20090605162553.GC6778@linux.vnet.ibm.com \
--to=paulmck@linux.vnet.ibm.com \
--cc=agraf@suse.de \
--cc=andi@firstfloor.org \
--cc=anthony@codemonkey.ws \
--cc=avi@redhat.com \
--cc=bhutchings@solarflare.com \
--cc=chrisw@sous-sol.org \
--cc=ghaskins@novell.com \
--cc=gregkh@suse.de \
--cc=gregory.haskins@gmail.com \
--cc=herber@gondor.apana.org.au \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mst@redhat.com \
--cc=netdev@vger.kernel.org \
--cc=pmorreale@novell.com \
--cc=pmullaney@novell.com \
--cc=rusty@rustcorp.com.au \
--cc=shemminger@vyatta.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).