Logo AND Algorithmique Numérique Distribuée

Public GIT Repository
[sonar] Don't mix public/private data members
authorFrederic Suter <frederic.suter@cc.in2p3.fr>
Mon, 13 Jan 2020 09:49:57 +0000 (10:49 +0100)
committerFrederic Suter <frederic.suter@cc.in2p3.fr>
Mon, 13 Jan 2020 13:11:05 +0000 (14:11 +0100)
12 files changed:
src/plugins/host_energy.cpp
src/plugins/vm/VirtualMachineImpl.cpp
src/plugins/vm/VirtualMachineImpl.hpp
src/plugins/vm/VmLiveMigration.cpp
src/s4u/s4u_Host.cpp
src/s4u/s4u_Storage.cpp
src/surf/HostImpl.cpp
src/surf/HostImpl.hpp
src/surf/StorageImpl.cpp
src/surf/StorageImpl.hpp
src/surf/sg_platf.cpp
src/surf/storage_n11.cpp

index 9d35a37..1f4ba65 100644 (file)
@@ -137,6 +137,9 @@ class HostEnergy {
    */
   int pstate_           = 0;
   const int pstate_off_ = -1;
+  double watts_off_     = 0.0;              /*< Consumption when the machine is turned off (shutdown) */
+  double total_energy_  = 0.0;              /*< Total energy consumed by the host */
+  double last_updated_  = surf_get_clock(); /*< Timestamp of the last energy update event*/
 
   /* Only used to split total energy into unused/used hosts.
    * If you want to get this info for something else, rather use the host_load plugin
@@ -159,11 +162,8 @@ public:
   double get_watt_min_at(int pstate);
   double get_watt_max_at(int pstate);
   double get_power_range_slope_at(int pstate);
+  double get_last_update_time() { return last_updated_; }
   void update();
-
-  double watts_off_    = 0.0; /*< Consumption when the machine is turned off (shutdown) */
-  double total_energy_ = 0.0; /*< Total energy consumed by the host */
-  double last_updated_ = surf_get_clock(); /*< Timestamp of the last energy update event*/
 };
 
 simgrid::xbt::Extension<simgrid::s4u::Host, HostEnergy> HostEnergy::EXTENSION_ID;
@@ -171,7 +171,7 @@ simgrid::xbt::Extension<simgrid::s4u::Host, HostEnergy> HostEnergy::EXTENSION_ID
 /* Computes the consumption so far. Called lazily on need. */
 void HostEnergy::update()
 {
-  double start_time  = this->last_updated_;
+  double start_time  = last_updated_;
   double finish_time = surf_get_clock();
   //
   // We may have start == finish if the past consumption was updated since the simcall was started
@@ -180,7 +180,7 @@ void HostEnergy::update()
   // Even in this case, we need to save the pstate for the next call (after this if),
   // which may have changed since that recent update.
   if (start_time < finish_time) {
-    double previous_energy = this->total_energy_;
+    double previous_energy = total_energy_;
 
     double instantaneous_power_consumption = this->get_current_watts_value();
 
@@ -188,17 +188,17 @@ void HostEnergy::update()
 
     // TODO Trace: Trace energy_this_step from start_time to finish_time in host->getName()
 
-    this->total_energy_ = previous_energy + energy_this_step;
-    this->last_updated_ = finish_time;
+    total_energy_ = previous_energy + energy_this_step;
+    last_updated_ = finish_time;
 
     XBT_DEBUG("[update_energy of %s] period=[%.8f-%.8f]; current speed=%.2E flop/s (pstate %i); total consumption "
               "before: %.8f J -> added now: %.8f J",
-              host_->get_cname(), start_time, finish_time, host_->get_pstate_speed(this->pstate_), this->pstate_,
-              previous_energy, energy_this_step);
+              host_->get_cname(), start_time, finish_time, host_->get_pstate_speed(pstate_), pstate_, previous_energy,
+              energy_this_step);
   }
 
   /* Save data for the upcoming time interval: whether it's on/off and the pstate if it's on */
-  this->pstate_ = host_->is_on() ? host_->get_pstate() : pstate_off_;
+  pstate_ = host_->is_on() ? host_->get_pstate() : pstate_off_;
 }
 
 HostEnergy::HostEnergy(simgrid::s4u::Host* ptr) : host_(ptr)
@@ -483,7 +483,7 @@ static void on_action_state_change(simgrid::kernel::resource::CpuAction const& a
       // Get the host_energy extension for the relevant host
       HostEnergy* host_energy = host->extension<HostEnergy>();
 
-      if (host_energy->last_updated_ < surf_get_clock())
+      if (host_energy->get_last_update_time() < surf_get_clock())
         host_energy->update();
     }
   }
index 03e4203..2d79dcb 100644 (file)
@@ -60,7 +60,7 @@ static void add_active_exec(s4u::Actor const&, s4u::Exec const& task)
   const s4u::VirtualMachine* vm = dynamic_cast<s4u::VirtualMachine*>(task.get_host());
   if (vm != nullptr) {
     VirtualMachineImpl* vm_impl = vm->get_impl();
-    vm_impl->active_tasks_      = vm_impl->active_tasks_ + 1;
+    vm_impl->add_active_exec();
     vm_impl->update_action_weight();
   }
 }
@@ -70,7 +70,7 @@ static void remove_active_exec(s4u::Actor const&, s4u::Exec const& task)
   const s4u::VirtualMachine* vm = dynamic_cast<s4u::VirtualMachine*>(task.get_host());
   if (vm != nullptr) {
     VirtualMachineImpl* vm_impl = vm->get_impl();
-    vm_impl->active_tasks_      = vm_impl->active_tasks_ - 1;
+    vm_impl->remove_active_exec();
     vm_impl->update_action_weight();
   }
 }
@@ -86,7 +86,7 @@ static void add_active_activity(kernel::activity::ActivityImpl const& act)
   const s4u::VirtualMachine* vm = get_vm_from_activity(act);
   if (vm != nullptr) {
     VirtualMachineImpl *vm_impl = vm->get_impl();
-    vm_impl->active_tasks_ = vm_impl->active_tasks_ + 1;
+    vm_impl->add_active_exec();
     vm_impl->update_action_weight();
   }
 }
@@ -96,7 +96,7 @@ static void remove_active_activity(kernel::activity::ActivityImpl const& act)
   const s4u::VirtualMachine* vm = get_vm_from_activity(act);
   if (vm != nullptr) {
     VirtualMachineImpl *vm_impl = vm->get_impl();
-    vm_impl->active_tasks_ = vm_impl->active_tasks_ - 1;
+    vm_impl->remove_active_exec();
     vm_impl->update_action_weight();
   }
 }
@@ -141,9 +141,8 @@ double VMModel::next_occurring_event(double now)
   for (s4u::VirtualMachine* const& ws_vm : VirtualMachineImpl::allVms_) {
     const kernel::resource::Cpu* cpu = ws_vm->pimpl_cpu;
 
-    double solved_value =
-        ws_vm->get_impl()->action_->get_variable()->get_value(); // this is X1 in comment above, what
-                                                                 // this VM got in the sharing on the PM
+    // solved_value below is X1 in comment above: what this VM got in the sharing on the PM
+    double solved_value = ws_vm->get_impl()->get_action()->get_variable()->get_value();
     XBT_DEBUG("assign %f to vm %s @ pm %s", solved_value, ws_vm->get_cname(), ws_vm->get_pm()->get_cname());
 
     xbt_assert(cpu->get_model() == surf_cpu_model_vm);
@@ -321,9 +320,9 @@ void VirtualMachineImpl::set_bound(double bound)
 
 void VirtualMachineImpl::update_action_weight(){
   /* The impact of the VM over its PM is the min between its vCPU amount and the amount of tasks it contains */
-  int impact = std::min(active_tasks_, get_core_amount());
+  int impact = std::min(active_execs_, get_core_amount());
 
-  XBT_DEBUG("set the weight of the dummy CPU action of VM%p on PM to %d (#tasks: %d)", this, impact, active_tasks_);
+  XBT_DEBUG("set the weight of the dummy CPU action of VM%p on PM to %d (#tasks: %u)", this, impact, active_execs_);
 
   if (impact > 0)
     action_->set_sharing_penalty(1. / impact);
index bb1807e..a3c5464 100644 (file)
@@ -31,14 +31,16 @@ class XBT_PUBLIC VirtualMachineImpl : public surf::HostImpl, public simgrid::xbt
   friend simgrid::s4u::VirtualMachine;
 
 public:
-  explicit VirtualMachineImpl(s4u::VirtualMachine* piface, s4u::Host* host, int core_amount, size_t ramsize);
-  ~VirtualMachineImpl();
-
   /** @brief Callbacks fired after VM creation. Signature: `void(VirtualMachineImpl&)` */
   static xbt::signal<void(simgrid::vm::VirtualMachineImpl&)> on_creation;
   /** @brief Callbacks fired after VM destruction. Signature: `void(VirtualMachineImpl const&)` */
   static xbt::signal<void(simgrid::vm::VirtualMachineImpl const&)> on_destruction;
 
+  static std::deque<s4u::VirtualMachine*> allVms_;
+
+  explicit VirtualMachineImpl(s4u::VirtualMachine* piface, s4u::Host* host, int core_amount, size_t ramsize);
+  ~VirtualMachineImpl();
+
   virtual void suspend(kernel::actor::ActorImpl* issuer);
   virtual void resume();
   virtual void shutdown(kernel::actor::ActorImpl* issuer);
@@ -54,24 +56,29 @@ public:
   s4u::VirtualMachine::state get_state() const { return vm_state_; }
   void set_state(s4u::VirtualMachine::state state) { vm_state_ = state; }
 
-  int get_core_amount() { return core_amount_; }
+  unsigned int get_core_amount() { return core_amount_; }
+  kernel::resource::Action* get_action() { return action_; }
 
   virtual void set_bound(double bound);
 
-  /* The vm object of the lower layer */
-  kernel::resource::Action* action_ = nullptr;
-  static std::deque<s4u::VirtualMachine*> allVms_;
-  bool is_migrating_ = false;
-  int active_tasks_ = 0;
-
   void update_action_weight();
 
+  void add_active_exec() { active_execs_++; }
+  void remove_active_exec() { active_execs_--; }
+
+  void start_migration() { is_migrating_ = true; }
+  void end_migration() { is_migrating_ = false; }
+  bool is_migrating() const { return is_migrating_; }
+
 private:
+  kernel::resource::Action* action_ = nullptr;
+  unsigned int active_execs_        = 0;
   s4u::Host* physical_host_;
-  int core_amount_;
+  unsigned int core_amount_;
   double user_bound_                   = std::numeric_limits<double>::max();
-  size_t ramsize_            = 0;
+  size_t ramsize_                      = 0;
   s4u::VirtualMachine::state vm_state_ = s4u::VirtualMachine::state::CREATED;
+  bool is_migrating_                   = false;
 };
 
 /*********
index 0690ee2..43f2629 100644 (file)
@@ -50,7 +50,7 @@ void MigrationRx::operator()()
   vm_->resume();
 
   // Now the VM is running on the new host (the migration is completed) (even if the SRC crash)
-  vm_->get_impl()->is_migrating_ = false;
+  vm_->get_impl()->end_migration();
   XBT_DEBUG("VM(%s) moved from PM(%s) to PM(%s)", vm_->get_cname(), src_pm_->get_cname(), dst_pm_->get_cname());
 
   if (TRACE_vm_is_enabled()) {
@@ -279,11 +279,11 @@ void MigrationTx::operator()()
 
 static void onVirtualMachineShutdown(simgrid::s4u::VirtualMachine const& vm)
 {
-  if (vm.get_impl()->is_migrating_) {
+  if (vm.get_impl()->is_migrating()) {
     vm.extension<simgrid::vm::VmMigrationExt>()->rx_->kill();
     vm.extension<simgrid::vm::VmMigrationExt>()->tx_->kill();
     vm.extension<simgrid::vm::VmMigrationExt>()->issuer_->kill();
-    vm.get_impl()->is_migrating_ = false;
+    vm.get_impl()->end_migration();
   }
 }
 
@@ -313,7 +313,7 @@ simgrid::s4u::VirtualMachine* sg_vm_create_migratable(simgrid::s4u::Host* pm, co
 
 int sg_vm_is_migrating(const simgrid::s4u::VirtualMachine* vm)
 {
-  return vm->get_impl()->is_migrating_;
+  return vm->get_impl()->is_migrating();
 }
 
 void sg_vm_migrate(simgrid::s4u::VirtualMachine* vm, simgrid::s4u::Host* dst_pm)
@@ -332,12 +332,12 @@ void sg_vm_migrate(simgrid::s4u::VirtualMachine* vm, simgrid::s4u::Host* dst_pm)
     throw simgrid::VmFailureException(
         XBT_THROW_POINT,
         simgrid::xbt::string_printf("Cannot migrate VM '%s' that is not running yet.", vm->get_cname()));
-  if (vm->get_impl()->is_migrating_)
+  if (vm->get_impl()->is_migrating())
     throw simgrid::VmFailureException(
         XBT_THROW_POINT,
         simgrid::xbt::string_printf("Cannot migrate VM '%s' that is already migrating.", vm->get_cname()));
 
-  vm->get_impl()->is_migrating_ = true;
+  vm->get_impl()->start_migration();
   simgrid::s4u::VirtualMachine::on_migration_start(*vm);
 
   std::string rx_name =
@@ -360,6 +360,6 @@ void sg_vm_migrate(simgrid::s4u::VirtualMachine* vm, simgrid::s4u::Host* dst_pm)
   tx->join();
   rx->join();
 
-  vm->get_impl()->is_migrating_ = false;
+  vm->get_impl()->end_migration();
   simgrid::s4u::VirtualMachine::on_migration_end(*vm);
 }
index 858750b..519cb96 100644 (file)
@@ -307,12 +307,9 @@ std::vector<const char*> Host::get_attached_storages() const
 
 std::unordered_map<std::string, Storage*> const& Host::get_mounted_storages()
 {
-  if (mounts_ == nullptr) {
-    mounts_ = new std::unordered_map<std::string, Storage*>();
-    for (auto const& m : this->pimpl_->storage_) {
-      mounts_->insert({m.first, m.second->get_iface()});
-    }
-  }
+  if (mounts_ == nullptr)
+    mounts_ = kernel::actor::simcall([this] { return this->pimpl_->get_mounted_storages(); });
+
   return *mounts_;
 }
 
index b051b69..dfd02fd 100644 (file)
@@ -37,7 +37,7 @@ Storage* Storage::by_name_or_null(const std::string& name)
 
 const char* Storage::get_type() const
 {
-  return pimpl_->typeId_.c_str();
+  return pimpl_->get_type();
 }
 
 const std::unordered_map<std::string, std::string>* Storage::get_properties() const
index 84e257e..2366c33 100644 (file)
@@ -111,6 +111,13 @@ std::vector<s4u::Disk*> HostImpl::get_disks()
   return disks;
 }
 
+void HostImpl::set_disks(const std::vector<kernel::resource::DiskImpl*>& disks, s4u::Host* host)
+{
+  disks_ = std::move(disks);
+  for (auto d : disks_)
+    d->set_host(host);
+}
+
 void HostImpl::add_disk(const s4u::Disk* disk)
 {
   disks_.push_back(disk->get_impl());
@@ -136,6 +143,13 @@ std::vector<const char*> HostImpl::get_attached_storages()
       storages.push_back(s.second->get_iface()->get_cname());
   return storages;
 }
-
+std::unordered_map<std::string, s4u::Storage*>* HostImpl::get_mounted_storages()
+{
+  std::unordered_map<std::string, s4u::Storage*>* mounts = new std::unordered_map<std::string, s4u::Storage*>();
+  for (auto const& m : storage_) {
+    mounts->insert({m.first, m.second->get_iface()});
+  }
+  return mounts;
+}
 } // namespace surf
 } // namespace simgrid
index 40ef7d7..91cf35c 100644 (file)
@@ -45,6 +45,8 @@ public:
 class XBT_PRIVATE HostImpl : public xbt::PropertyHolder {
   std::vector<kernel::actor::ProcessArg*> actors_at_boot_;
   s4u::Host* piface_ = nullptr; // FIXME: why don't we store a s4u::Host here as we do everywhere else?
+  std::map<std::string, kernel::resource::StorageImpl*> storage_;
+  std::vector<kernel::resource::DiskImpl*> disks_;
 
 public:
   friend simgrid::vm::VirtualMachineImpl;
@@ -52,14 +54,14 @@ public:
   virtual ~HostImpl();
 
   std::vector<s4u::Disk*> get_disks();
+  void set_disks(const std::vector<kernel::resource::DiskImpl*>& disks, s4u::Host* host);
   void add_disk(const s4u::Disk* disk);
   void remove_disk(const std::string& disk_name);
 
   /** @brief Get the vector of storages (by names) attached to the Host */
   virtual std::vector<const char*> get_attached_storages();
-
-  std::map<std::string, kernel::resource::StorageImpl*> storage_;
-  std::vector<kernel::resource::DiskImpl*> disks_;
+  std::unordered_map<std::string, s4u::Storage*>* get_mounted_storages();
+  void set_storages(const std::map<std::string, kernel::resource::StorageImpl*>& storages) { storage_ = storages; }
 
   s4u::Host* get_iface() { return piface_; }
 
index 78eb021..4db2a48 100644 (file)
@@ -41,9 +41,9 @@ StorageImpl::StorageImpl(kernel::resource::Model* model, const std::string& name
     : Resource(model, name, maxminSystem->constraint_new(this, std::max(bread, bwrite)))
     , piface_(name, this)
     , typeId_(type_id)
+    , attach_(attach)
     , content_name_(content_name)
     , size_(size)
-    , attach_(attach)
 {
   StorageImpl::turn_on();
   XBT_DEBUG("Create resource with Bread '%f' Bwrite '%f' and Size '%llu'", bread, bwrite, size);
index 2b3e5c5..b1c6047 100644 (file)
@@ -61,8 +61,18 @@ public:
  */
 class StorageImpl : public Resource, public xbt::PropertyHolder {
   s4u::Storage piface_;
+  lmm::Constraint* constraint_read_;  /* Constraint for maximum write bandwidth*/
+  lmm::Constraint* constraint_write_; /* Constraint for maximum write bandwidth*/
+
+  std::string typeId_;
+  bool currently_destroying_ = false;
+  // Name of the host to which this storage is attached. Only used at platform parsing time, then the interface stores
+  // the Host directly.
+  std::string attach_;
 
 public:
+  const std::string content_name_; // Only used at parsing time then goes to the FileSystemExtension
+  const sg_size_t size_;           // Only used at parsing time then goes to the FileSystemExtension
   /** @brief Storage constructor */
   StorageImpl(Model* model, const std::string& name, kernel::lmm::System* maxmin_system, double bread, double bwrite,
               const std::string& type_id, const std::string& content_name, sg_size_t size, const std::string& attach);
@@ -72,6 +82,9 @@ public:
   ~StorageImpl() override;
 
   s4u::Storage* get_iface() { return &piface_; }
+  const char* get_type() { return typeId_.c_str(); }
+  lmm::Constraint* get_read_constraint() const { return constraint_read_; }
+  lmm::Constraint* get_write_constraint() const { return constraint_write_; }
   /** @brief Check if the Storage is used (if an action currently uses its resources) */
   bool is_used() override;
 
@@ -98,19 +111,6 @@ public:
    */
   virtual StorageAction* write(sg_size_t size) = 0;
   const std::string& get_host() const { return attach_; }
-
-  lmm::Constraint* constraint_write_; /* Constraint for maximum write bandwidth*/
-  lmm::Constraint* constraint_read_;  /* Constraint for maximum write bandwidth*/
-
-  std::string typeId_;
-  std::string content_name_; // Only used at parsing time then goes to the FileSystemExtension
-  sg_size_t size_;          // Only used at parsing time then goes to the FileSystemExtension
-
-private:
-  bool currently_destroying_ = false;
-  // Name of the host to which this storage is attached. Only used at platform parsing time, then the interface stores
-  // the Host directly.
-  std::string attach_;
 };
 
 /**********
index 3dc8bb3..22d8507 100644 (file)
@@ -79,12 +79,10 @@ void sg_platf_new_host(simgrid::kernel::routing::HostCreationArgs* args)
   simgrid::s4u::Host* host =
       routing_get_current()->create_host(args->id, args->speed_per_pstate, args->core_amount, &props);
 
-  host->pimpl_->storage_ = mount_list;
+  host->pimpl_->set_storages(mount_list);
   mount_list.clear();
 
-  host->pimpl_->disks_ = std::move(args->disks);
-  for (auto d : host->pimpl_->disks_)
-    d->set_host(host);
+  host->pimpl_->set_disks(args->disks, host);
 
   /* Change from the defaults */
   if (args->state_trace)
index dd83d42..50439d8 100644 (file)
@@ -127,10 +127,10 @@ StorageN11Action::StorageN11Action(Model* model, double cost, bool failed, Stora
   model->get_maxmin_system()->expand(storage->get_constraint(), get_variable(), 1.0);
   switch(type) {
     case s4u::Io::OpType::READ:
-      model->get_maxmin_system()->expand(storage->constraint_read_, get_variable(), 1.0);
+      model->get_maxmin_system()->expand(storage->get_read_constraint(), get_variable(), 1.0);
       break;
     case s4u::Io::OpType::WRITE:
-      model->get_maxmin_system()->expand(storage->constraint_write_, get_variable(), 1.0);
+      model->get_maxmin_system()->expand(storage->get_write_constraint(), get_variable(), 1.0);
       break;
     default:
       THROW_UNIMPLEMENTED;