[ovs-dev] [PATCH v3 01/13] ofproto: Don't use connmgr after destruction.

Jarno Rajahalme jarno at ovn.org
Thu Sep 15 21:51:18 UTC 2016


> On Sep 13, 2016, at 5:13 PM, Ben Pfaff <blp at ovn.org> wrote:
> 
> On Tue, Sep 13, 2016 at 02:45:54PM -0700, Jarno Rajahalme wrote:
>> 
>>> On Sep 13, 2016, at 10:56 AM, Ben Pfaff <blp at ovn.org> wrote:
>>> 
>>> On Mon, Sep 12, 2016 at 01:52:31PM -0700, Jarno Rajahalme wrote:
>>>> Set ofproto's connmgr pointer to NULL after the connmgr has been
>>>> destructed, and check for NULL when sending a flow removed
>>>> notification.
>>>> 
>>>> Verified by sending the flow removed message unconditionally and
>>>> observing numerous core dumps in the test suite.
>>>> 
>>>> Found by inspection.
>>>> 
>>>> Fixes: f695ebfae5 ("ofproto: Postpone sending flow removed messages.")
>>>> Signed-off-by: Jarno Rajahalme <jarno at ovn.org>
>>>> ---
>>>> v3: New patch for v3.
>>> 
>>> I'm worried a bit that the rules needed to access these structures
>>> correctly are getting complicated.  Maybe there should be a high-level
>>> overview somewhere.
>>> 
>> 
>> The main complication here is that if we tagged connmgr * with OVS_GUARDED_BY(ofproto_mutex), we would need to do widen the scope of ofproto_mutex quite a bit. Maybe introducing a rwlock for connmgr separately would do it, albeit with the assumption that the main thread remains the only writer, all that extra read locking from the main thread would be new overhead. It would be more maintainable, though.
> 
> Yes, I recognize that and I don't want to make the scope bigger if we
> can avoid it.
> 
> Another option might be to just stick the flow_removed messages on a
> list from the callback and then flush them later in the ofproto_run()
> loop.  They're already asynchronous and unpredictable since RCU can
> delay sending them an arbitrary amount of time.
> 

Even that would have to deal with ofproto disappearing, but I like it as it would simplify locking requirements. I’ll make a note to look at this later.

>> I made a note to look into this later.
>> 
>> Do you think we should apply this to branch-2.6 as well as the master?
> 
> It fixes a crash bug right?  So, yes, unless you know a simpler fix.

I back ported this to 2.6.

  Jarno




More information about the dev mailing list