mac: apply changes by PR reviewers

- reinstate write_lock on ue metrics
- change "rwlock" variable name in mac_nr.h

Signed-off-by: Carlo Galiotto <carlo@srs.io>

lib/include/srsran/common/rwlock_guard.h

@@ -31,19 +31,19 @@ private:
   pthread_rwlock_t* rwlock;
 };
 
-// Shared lock guard that automatically unlocks rwmutex on exit
+// Shared lock guard that automatically unlocks rwlock on exit
 class rwlock_read_guard
 {
 public:
-  rwlock_read_guard(pthread_rwlock_t& rwlock_) : rwmutex(&rwlock_) { pthread_rwlock_rdlock(rwmutex); }
+  rwlock_read_guard(pthread_rwlock_t& rwlock_) : rwlock(&rwlock_) { pthread_rwlock_rdlock(rwlock); }
   rwlock_read_guard(const rwlock_read_guard&) = delete;
   rwlock_read_guard(rwlock_read_guard&&)      = delete;
   rwlock_read_guard& operator=(const rwlock_read_guard&) = delete;
   rwlock_read_guard& operator=(rwlock_read_guard&&) = delete;
-  ~rwlock_read_guard() { pthread_rwlock_unlock(rwmutex); }
+  ~rwlock_read_guard() { pthread_rwlock_unlock(rwlock); }
 
 private:
-  pthread_rwlock_t* rwmutex;
+  pthread_rwlock_t* rwlock;
 };
 
 } // namespace srsran

srsenb/hdr/stack/mac/mac.h

@@ -112,7 +112,7 @@ private:
 
   srslog::basic_logger& logger;
 
-  // We use a rwmutex in MAC to allow multiple workers to access MAC simultaneously. No conflicts will happen since
+  // We use a rwlock in MAC to allow multiple workers to access MAC simultaneously. No conflicts will happen since
   // access for different TTIs
   pthread_rwlock_t rwlock = {};
 

srsenb/hdr/stack/mac/nr/mac_nr.h

@@ -111,7 +111,7 @@ private:
   std::vector<sched_nr_interface::cell_cfg_t> cell_config;
 
   // Map of active UEs
-  pthread_rwlock_t                                                              rwlock     = {};
+  pthread_rwlock_t                                                              rwmutex    = {};
   static const uint16_t                                                         FIRST_RNTI = 0x4601;
   srsran::static_circular_map<uint16_t, std::unique_ptr<ue_nr>, SRSENB_MAX_UES> ue_db;
 

srsenb/src/stack/mac/mac.cc

@@ -996,7 +996,7 @@ void mac::write_mcch(const srsran::sib2_mbms_t* sib2_,
   rrc_h->add_user(SRSRAN_MRNTI, {});
 }
 
-// Internal helper function, caller must hold UE DB rwmutex
+// Internal helper function, caller must hold UE DB rwlock
 bool mac::check_ue_active(uint16_t rnti)
 {
   if (not ue_db.contains(rnti)) {

srsenb/src/stack/mac/nr/mac_nr.cc

@@ -200,7 +200,7 @@ uint16_t mac_nr::alloc_ue(uint32_t enb_cc_idx)
 
     // Pre-check if rnti is valid
     {
-      srsran::rwlock_read_guard read_lock(rwlock);
+      srsran::rwlock_read_guard read_lock(rwmutex);
       if (not is_rnti_valid_nolock(rnti)) {
         continue;
       }
@@ -210,7 +210,7 @@ uint16_t mac_nr::alloc_ue(uint32_t enb_cc_idx)
     std::unique_ptr<ue_nr> ue_ptr = std::unique_ptr<ue_nr>(new ue_nr(rnti, enb_cc_idx, &sched, rrc, rlc, phy, logger));
 
     // Add UE to rnti map
-    srsran::rwlock_write_guard rw_lock(rwlock);
+    srsran::rwlock_write_guard rw_lock(rwmutex);
     if (not is_rnti_valid_nolock(rnti)) {
       continue;
     }
@@ -228,7 +228,7 @@ uint16_t mac_nr::alloc_ue(uint32_t enb_cc_idx)
 // Remove UE from the perspective of L2/L3
 int mac_nr::remove_ue(uint16_t rnti)
 {
-  srsran::rwlock_write_guard lock(rwlock);
+  srsran::rwlock_write_guard lock(rwmutex);
   if (is_rnti_active_nolock(rnti)) {
     sched.ue_rem(rnti);
     ue_db.erase(rnti);
@@ -292,7 +292,7 @@ int mac_nr::get_dl_sched(const srsran_slot_cfg_t& slot_cfg, dl_sched_t& dl_sched
   dl_sched = dl_res.dl_sched;
 
   uint32_t                  rar_count = 0;
-  srsran::rwlock_read_guard rw_lock(rwlock);
+  srsran::rwlock_read_guard rw_lock(rwmutex);
   for (pdsch_t& pdsch : dl_sched.pdsch) {
     if (pdsch.sch.grant.rnti_type == srsran_rnti_type_c) {
       uint16_t rnti = pdsch.sch.grant.rnti;
@@ -330,7 +330,7 @@ int mac_nr::get_ul_sched(const srsran_slot_cfg_t& slot_cfg, ul_sched_t& ul_sched
   slot_point pusch_slot = srsran::slot_point{NUMEROLOGY_IDX, slot_cfg.idx};
   ret                   = sched.get_ul_sched(pusch_slot, 0, ul_sched);
 
-  srsran::rwlock_read_guard rw_lock(rwlock);
+  srsran::rwlock_read_guard rw_lock(rwmutex);
   for (auto& pusch : ul_sched.pusch) {
     if (ue_db.contains(pusch.sch.grant.rnti)) {
       ue_db[pusch.sch.grant.rnti]->metrics_ul_mcs(pusch.sch.grant.tb->mcs);
@@ -355,7 +355,7 @@ bool mac_nr::handle_uci_data(const uint16_t rnti, const srsran_uci_cfg_nr_t& cfg
     const srsran_harq_ack_bit_t* ack_bit = &cfg_.ack.bits[i];
     bool                         is_ok   = (value.ack[i] == 1) and value.valid;
     sched.dl_ack_info(rnti, 0, ack_bit->pid, 0, is_ok);
-    srsran::rwlock_read_guard rw_lock(rwlock);
+    srsran::rwlock_read_guard rw_lock(rwmutex);
     if (ue_db.contains(rnti)) {
       ue_db[rnti]->metrics_tx(is_ok, 0 /*TODO get size of packet from scheduler somehow*/);
     }
@@ -367,7 +367,7 @@ bool mac_nr::handle_uci_data(const uint16_t rnti, const srsran_uci_cfg_nr_t& cfg
   }
 
   // Process CQI
-  srsran::rwlock_read_guard rw_lock(rwlock);
+  srsran::rwlock_read_guard rw_lock(rwmutex);
   ue_db[rnti]->metrics_dl_cqi(cfg_, value.csi->wideband_cri_ri_pmi_cqi.cqi, value.valid);
 
   return true;
@@ -394,7 +394,7 @@ int mac_nr::pusch_info(const srsran_slot_cfg_t& slot_cfg, mac_interface_phy_nr::
     }
 
     auto process_pdu_task = [this, rnti](srsran::unique_byte_buffer_t& pdu) {
-      srsran::rwlock_read_guard lock(rwlock);
+      srsran::rwlock_read_guard lock(rwmutex);
       if (is_rnti_active_nolock(rnti)) {
         ue_db[rnti]->process_pdu(std::move(pdu));
       } else {
@@ -403,7 +403,7 @@ int mac_nr::pusch_info(const srsran_slot_cfg_t& slot_cfg, mac_interface_phy_nr::
     };
     stack_task_queue.try_push(std::bind(process_pdu_task, std::move(pusch_info.pdu)));
   }
-  srsran::rwlock_read_guard rw_lock(rwlock);
+  srsran::rwlock_read_guard rw_lock(rwmutex);
   if (ue_db.contains(rnti)) {
     ue_db[rnti]->metrics_rx(pusch_info.pusch_data.tb[0].crc, nof_bytes);
   }

srsenb/src/stack/mac/nr/ue_nr.cc

@@ -196,8 +196,7 @@ void ue_nr::metrics_read(mac_ue_metrics_t* metrics_)
 
 void ue_nr::metrics_dl_cqi(const srsran_uci_cfg_nr_t& cfg_, uint32_t dl_cqi, bool valid_cqi)
 {
-  // I think this is not necessary, as we locked from the calling function
-  // std::lock_guard<std::mutex> lock(metrics_mutex);
+  std::lock_guard<std::mutex> lock(metrics_mutex);
 
   // Process CQI
   for (uint32_t i = 0; i < cfg_.nof_csi; i++) {