On Tue, Apr 9, 2013 at 9:04 PM, Pravin Shelar <pshe...@nicira.com> wrote:
> On Tue, Apr 9, 2013 at 6:41 PM, Jesse Gross <je...@nicira.com> wrote:
>> On Tue, Apr 9, 2013 at 3:03 PM, Pravin B Shelar <pshe...@nicira.com>
>> 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.
>> > 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.
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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
