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.
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;
}
Thanks!
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
