On Wed, Apr 10, 2013 at 9:01 AM, Jesse Gross <[email protected]> wrote:
> On Tue, Apr 9, 2013 at 9:04 PM, Pravin Shelar <[email protected]> wrote: > > On Tue, Apr 9, 2013 at 6:41 PM, Jesse Gross <[email protected]> wrote: > >> On Tue, Apr 9, 2013 at 3:03 PM, Pravin B Shelar <[email protected]> > >> wrote: > >> > diff --git a/datapath/datapath.h b/datapath/datapath.h > >> > index 9bc98fb..58d7169 100644 > >> > --- a/datapath/datapath.h > >> > +++ b/datapath/datapath.h > >> > -static inline struct vport *ovs_vport_rtnl_rcu(const struct datapath > >> > *dp, int port_no) > >> > +static inline struct vport *ovs_vport_ovsl_rcu(const struct datapath > >> > *dp, int port_no) > >> > { > >> > - WARN_ON_ONCE(!rcu_read_lock_held() && !rtnl_is_locked()); > >> > + WARN_ON_ONCE(!rcu_read_lock_held() && !ovs_is_locked()); > >> > return ovs_lookup_vport(dp, port_no); > >> > } > >> > > >> > -static inline struct vport *ovs_vport_rtnl(const struct datapath *dp, > >> > int port_no) > >> > +static inline struct vport *ovs_vport_ovsl(const struct datapath *dp, > >> > int port_no) > >> > { > >> > - ASSERT_RTNL(); > >> > + ASSERT_OVSL(); > >> > >> For these two functions it seems like it might be better to use the > >> lockdep version of the check rather than unconditionally doing it. > >> > > do you mean lockdep_assert_held() ? > > Yes, I think we should create an ovsl wrapper around > lockdep_assert_held() and then use it here. > > ok. > >> > diff --git a/datapath/dp_notify.c b/datapath/dp_notify.c > >> > index c9eeafe..1e19125 100644 > >> > --- a/datapath/dp_notify.c > >> > +++ b/datapath/dp_notify.c > >> I think there is potentially another race condition here: if a > >> notification comes after we retrieve the value of count but before we > >> exit the workqueue. The counter will be incremented but another work > >> instance won't be scheduled since one is already running. Essentially > >> we're trying to emulate a semaphore here but actually doing that > >> directly would require introducing a new thread, which I really don't > >> want to do. > >> > > Pending flag is cleared before actual work execution is started. So I > think > > this race is not possible. > > You're right, that's great. We should also double check that our > compatibility code for workqueues is sufficient for everything we are > trying to do. > > ok. > By the way, I think we may be able to make the workqueue compatibility > code conditional on version again. The comment says that we were > having problems with waiting on genl mutex when using workqueues but > we shouldn't have to do that anymore. > I guess this softlockup issue will move from genl_lock to ovs_mutex, as rehash_wq need to take ovs_mutex. therefore we still need to use compat workq code.
_______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
