From 90bad7def4a060f0fc5e7cd77a9e707777ff8d20 Mon Sep 17 00:00:00 2001 From: Nischal Sheth Date: Sun, 3 May 2015 19:31:48 -0700 Subject: [PATCH] Fix concurrency issue in RibUpdateMonitor::MergeUpdate The return value of RibUpdateMonitor::EnqueueUpdateUnlocked could be incorrect if it's called without holding the monitor mutex. That in turn prevents BgpExport::Join from kicking SchedulingGroup to start a tail dequeue for the bulk UpdateQueue. Change-Id: I1d2a4418346bb0e90eaed68a3a535d9bc741825e Closes-Bug: #1451306 --- src/bgp/bgp_update_monitor.cc | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/src/bgp/bgp_update_monitor.cc b/src/bgp/bgp_update_monitor.cc index 81d8af727ce..46e60ba3416 100644 --- a/src/bgp/bgp_update_monitor.cc +++ b/src/bgp/bgp_update_monitor.cc @@ -369,8 +369,8 @@ bool RibUpdateMonitor::GetPeerSetCurrentAndScheduled(DBEntryBase *db_entry, // // Concurrency: Called in the context of the DB partition task. There's no -// entry lock as such since the DBState is a RouteState. We may or may not -// hold the monitor lock, but in either case we don't try to acquire it. +// entry lock since the DBState is a RouteState, but the monitor lock must +// be held. // // Helper routine for MergeUpdate to handle case where the current DBState // is a RouteState. @@ -470,17 +470,10 @@ bool RibUpdateMonitor::MergeUpdate(DBEntryBase *db_entry, RouteUpdate *rt_update) { CHECK_CONCURRENCY("db::DBTable"); - // Don't need to bother going through the monitor if there's no DBState. - // The scheduling group task can't hold a lock on the entry in this case. - DBState *dbstate = - db_entry->GetState(ribout_->table(), ribout_->listener_id()); - if (dbstate == NULL) { - return EnqueueUpdateUnlocked(db_entry, rt_update); - } - - // Go through the monitor and handle the DBState as appropriate. Note - // that we still need to check for the DBState being NULL as things may - // have changed after the check made above. + // Go through the monitor and handle the DBState as necessary. Need + // to use the monitor even if there's no DBState in order to protect + // against race conditions which could result in an incorrect return + // value. while (true) { tbb::interface5::unique_lock toplock(mutex_); DBState *dbstate =