Re: On netfront accelerator add/remove watches

[Neil Turton <nturton@solarflare.com>]

Hi,

BVK Chaitanya wrote:
> I see that netfront_accel_add_watch and netfront_accel_remove_watch 
> functions are _not_ protected by accelerator_mutex in accel.c   Is there 
> any specific reason for this?

Yes.  These functions need to be synchronised by the callers.  Adding a
mutex here would ensure that they didn't execute at the same time, but
wouldn't impose any order on the calls.  This matters because add
followed by remove is different from remove followed by add.  The
callers need to decide which order they should be executed in.

The relevant call chains are as follows:

xenbus otherend_changed callback
  -> backend_changed [netfront.c]
    -> network_connect
      -> talk_to_backend
        -> netfront_accelerator_add_watch

xenbus suspend_cancel callback
  -> netfront_suspend_cancel
    -> netfront_accelerator_suspend_cancel
      -> netfront_accelerator_add_watch

xenbus suspend callback
  -> netfront_suspend
    -> netfront_accelerator_suspend
      -> netfront_accelerator_remove_watch

xenbus remove callback
  -> netfront_remove
    -> netfront_accelerator_call_remove
      -> netfront_accelerator_remove_watch

So the watch is only added/removed from a xenbus callback.  I think
these callbacks should be synchronised by xenbus.  Can someone confirm that?

> I see that they sometimes get called twice (and result in BUG_ON) in 
> very fast (20ms) domain suspend-resume cycles and I couldn't figure out 
> how it is possible :-(

Is that the BUG_ON in netfront_accelerator_add_watch?  One possible
explanation is that suspend_cancel is called and then otherend_changed
is called.  Can you add a printk to netfront_suspend_cancel to see if it
gets called just before the BUG_ON gets triggered?

Cheers, Neil.