[review] updates: ..

Author: zeeshanlakhani

dpd/src/mcast/mod.rs

@@ -246,12 +246,13 @@ pub struct MulticastGroupData {
 impl MulticastGroupData {
     const GENERATOR_START: u16 = 100;
 
+    /// Creates a new instance of MulticastGroupData with pre-populated free
+    /// group IDs.
     pub(crate) fn new() -> Self {
         // Pre-populate with all available IDs from GENERATOR_START to u16::MAX-1
         // Using a Vec as a stack for O(1) push/pop operations
-        let free_group_ids = Arc::new(Mutex::new(
-            (Self::GENERATOR_START..u16::MAX).collect::<Vec<u16>>(),
-        ));
+        let free_group_ids =
+            Arc::new(Mutex::new((Self::GENERATOR_START..u16::MAX).collect()));
 
         Self {
             groups: BTreeMap::new(),
@@ -263,6 +264,7 @@ impl MulticastGroupData {
     /// Generates a unique multicast group ID with automatic cleanup on drop.
     ///
     /// O(1) allocation from pre-populated free list. Never allocates.
+    ///
     /// IDs below GENERATOR_START (100) to avoid conflicts with reserved ranges.
     ///
     /// Returns a ScopedGroupId that will automatically return the ID to the
@@ -402,17 +404,11 @@ pub(crate) fn add_group_external(
             let internal_ip = group_info.nat_target.internal_ip.into();
             debug!(
                 s.log,
-                "External group {} with VLAN {} references internal group {}",
+                "External group {} with VLAN {} references internal group {}, propagating VLAN to existing internal group",
                 group_ip,
                 vlan_id,
                 internal_ip
             );
-            debug!(
-                s.log,
-                "Propagating VLAN {} to existing internal group {}",
-                vlan_id,
-                internal_ip
-            );
 
             let internal_group = mcast
                 .groups
@@ -581,7 +577,7 @@ fn add_group_internal_only(
 /// Delete a multicast group from the switch, including all associated tables
 /// and port mappings.
 pub(crate) fn del_group(s: &Switch, group_ip: IpAddr) -> DpdResult<()> {
-    let (group, group_ids_to_free, nat_target_to_remove) = {
+    let (group, nat_target_to_remove) = {
         let mut mcast = s.mcast.lock().unwrap();
 
         let group = mcast.groups.remove(&group_ip).ok_or_else(|| {
@@ -591,21 +587,12 @@ pub(crate) fn del_group(s: &Switch, group_ip: IpAddr) -> DpdResult<()> {
             ))
         })?;
 
-        // Collect group IDs that need to be freed
-        let mut group_ids_to_free = Vec::new();
-        if let Some(external_id) = group.external_group_id {
-            group_ids_to_free.push(external_id);
-        }
-        if let Some(underlay_id) = group.underlay_group_id {
-            group_ids_to_free.push(underlay_id);
-        }
-
         let nat_target_to_remove = group
             .int_fwding
             .nat_target
             .map(|nat| nat.internal_ip.into());
 
-        (group, group_ids_to_free, nat_target_to_remove)
+        (group, nat_target_to_remove)
     };
 
     debug!(s.log, "deleting multicast group for IP {}", group_ip);
@@ -624,16 +611,6 @@ pub(crate) fn del_group(s: &Switch, group_ip: IpAddr) -> DpdResult<()> {
         if let Some(internal_ip) = nat_target_to_remove {
             mcast.remove_nat_target_ref(group_ip, internal_ip);
         }
-        if !group_ids_to_free.is_empty() {
-            if let Ok(mut pool) = mcast.free_group_ids.lock() {
-                for group_id in group_ids_to_free {
-                    // Only add to free list if it's in valid range
-                    if group_id >= MulticastGroupData::GENERATOR_START {
-                        pool.push(group_id);
-                    }
-                }
-            }
-        }
     }
 
     Ok(())
@@ -1302,6 +1279,25 @@ fn delete_multicast_groups(
         })?;
     }
 
+    // Return group IDs to the free pool
+    let free_group_ids = {
+        let mcast = s.mcast.lock().unwrap();
+        mcast.free_group_ids.clone() // Cloning an ARC (cheap clone)
+    };
+
+    if let Ok(mut pool) = free_group_ids.lock() {
+        if let Some(external_id) = external_group_id {
+            if external_id >= MulticastGroupData::GENERATOR_START {
+                pool.push(external_id);
+            }
+        }
+        if let Some(underlay_id) = underlay_group_id {
+            if underlay_id >= MulticastGroupData::GENERATOR_START {
+                pool.push(underlay_id);
+            }
+        }
+    }
+
     Ok(())
 }
 
@@ -2420,7 +2416,7 @@ mod tests {
     #[test]
     fn test_concurrent_id_allocation() {
         let mcast_data = Arc::new(Mutex::new(MulticastGroupData::new()));
-        let mut handles = vec![];
+        let mut handles = Vec::new();
 
         // Spawn multiple threads to allocate IDs concurrently
         for _ in 0..10 {
@@ -2435,7 +2431,7 @@ mod tests {
         }
 
         // Collect all allocated IDs
-        let mut allocated_ids = vec![];
+        let mut allocated_ids = Vec::new();
         for handle in handles {
             allocated_ids.push(handle.join().unwrap());
         }
@@ -2455,7 +2451,7 @@ mod tests {
     #[test]
     fn test_concurrent_allocation_and_deallocation() {
         let mcast_data = Arc::new(Mutex::new(MulticastGroupData::new()));
-        let mut handles = vec![];
+        let mut handles = Vec::new();
 
         // Spawn threads that allocate and immediately drop (deallocate)
         for _ in 0..5 {