From 131ea969cf91c332bd533b7e9d67c729149e9b4d Mon Sep 17 00:00:00 2001 From: Frederic Suter Date: Mon, 13 Jan 2020 10:49:57 +0100 Subject: [PATCH] [sonar] Don't mix public/private data members --- src/plugins/host_energy.cpp | 24 ++++++++++----------- src/plugins/vm/VirtualMachineImpl.cpp | 17 +++++++-------- src/plugins/vm/VirtualMachineImpl.hpp | 31 ++++++++++++++++----------- src/plugins/vm/VmLiveMigration.cpp | 14 ++++++------ src/s4u/s4u_Host.cpp | 9 +++----- src/s4u/s4u_Storage.cpp | 2 +- src/surf/HostImpl.cpp | 16 +++++++++++++- src/surf/HostImpl.hpp | 8 ++++--- src/surf/StorageImpl.cpp | 2 +- src/surf/StorageImpl.hpp | 26 +++++++++++----------- src/surf/sg_platf.cpp | 6 ++---- src/surf/storage_n11.cpp | 4 ++-- 12 files changed, 88 insertions(+), 71 deletions(-) diff --git a/src/plugins/host_energy.cpp b/src/plugins/host_energy.cpp index 9d35a37408..1f4ba650f1 100644 --- a/src/plugins/host_energy.cpp +++ b/src/plugins/host_energy.cpp @@ -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 HostEnergy::EXTENSION_ID; @@ -171,7 +171,7 @@ simgrid::xbt::Extension 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(); - if (host_energy->last_updated_ < surf_get_clock()) + if (host_energy->get_last_update_time() < surf_get_clock()) host_energy->update(); } } diff --git a/src/plugins/vm/VirtualMachineImpl.cpp b/src/plugins/vm/VirtualMachineImpl.cpp index 03e42035ce..2d79dcbbb6 100644 --- a/src/plugins/vm/VirtualMachineImpl.cpp +++ b/src/plugins/vm/VirtualMachineImpl.cpp @@ -60,7 +60,7 @@ static void add_active_exec(s4u::Actor const&, s4u::Exec const& task) const s4u::VirtualMachine* vm = dynamic_cast(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(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); diff --git a/src/plugins/vm/VirtualMachineImpl.hpp b/src/plugins/vm/VirtualMachineImpl.hpp index bb1807e912..a3c5464d4c 100644 --- a/src/plugins/vm/VirtualMachineImpl.hpp +++ b/src/plugins/vm/VirtualMachineImpl.hpp @@ -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 on_creation; /** @brief Callbacks fired after VM destruction. Signature: `void(VirtualMachineImpl const&)` */ static xbt::signal on_destruction; + static std::deque 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 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::max(); - size_t ramsize_ = 0; + size_t ramsize_ = 0; s4u::VirtualMachine::state vm_state_ = s4u::VirtualMachine::state::CREATED; + bool is_migrating_ = false; }; /********* diff --git a/src/plugins/vm/VmLiveMigration.cpp b/src/plugins/vm/VmLiveMigration.cpp index 0690ee225b..43f2629a5e 100644 --- a/src/plugins/vm/VmLiveMigration.cpp +++ b/src/plugins/vm/VmLiveMigration.cpp @@ -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()->rx_->kill(); vm.extension()->tx_->kill(); vm.extension()->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); } diff --git a/src/s4u/s4u_Host.cpp b/src/s4u/s4u_Host.cpp index 858750bc81..519cb965ed 100644 --- a/src/s4u/s4u_Host.cpp +++ b/src/s4u/s4u_Host.cpp @@ -307,12 +307,9 @@ std::vector Host::get_attached_storages() const std::unordered_map const& Host::get_mounted_storages() { - if (mounts_ == nullptr) { - mounts_ = new std::unordered_map(); - 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_; } diff --git a/src/s4u/s4u_Storage.cpp b/src/s4u/s4u_Storage.cpp index b051b69f31..dfd02fd22d 100644 --- a/src/s4u/s4u_Storage.cpp +++ b/src/s4u/s4u_Storage.cpp @@ -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* Storage::get_properties() const diff --git a/src/surf/HostImpl.cpp b/src/surf/HostImpl.cpp index 84e257ebac..2366c33b37 100644 --- a/src/surf/HostImpl.cpp +++ b/src/surf/HostImpl.cpp @@ -111,6 +111,13 @@ std::vector HostImpl::get_disks() return disks; } +void HostImpl::set_disks(const std::vector& 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 HostImpl::get_attached_storages() storages.push_back(s.second->get_iface()->get_cname()); return storages; } - +std::unordered_map* HostImpl::get_mounted_storages() +{ + std::unordered_map* mounts = new std::unordered_map(); + for (auto const& m : storage_) { + mounts->insert({m.first, m.second->get_iface()}); + } + return mounts; +} } // namespace surf } // namespace simgrid diff --git a/src/surf/HostImpl.hpp b/src/surf/HostImpl.hpp index 40ef7d7bb2..91cf35c81e 100644 --- a/src/surf/HostImpl.hpp +++ b/src/surf/HostImpl.hpp @@ -45,6 +45,8 @@ public: class XBT_PRIVATE HostImpl : public xbt::PropertyHolder { std::vector actors_at_boot_; s4u::Host* piface_ = nullptr; // FIXME: why don't we store a s4u::Host here as we do everywhere else? + std::map storage_; + std::vector disks_; public: friend simgrid::vm::VirtualMachineImpl; @@ -52,14 +54,14 @@ public: virtual ~HostImpl(); std::vector get_disks(); + void set_disks(const std::vector& 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 get_attached_storages(); - - std::map storage_; - std::vector disks_; + std::unordered_map* get_mounted_storages(); + void set_storages(const std::map& storages) { storage_ = storages; } s4u::Host* get_iface() { return piface_; } diff --git a/src/surf/StorageImpl.cpp b/src/surf/StorageImpl.cpp index 78eb021a04..4db2a48818 100644 --- a/src/surf/StorageImpl.cpp +++ b/src/surf/StorageImpl.cpp @@ -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); diff --git a/src/surf/StorageImpl.hpp b/src/surf/StorageImpl.hpp index 2b3e5c5094..b1c60472f4 100644 --- a/src/surf/StorageImpl.hpp +++ b/src/surf/StorageImpl.hpp @@ -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_; }; /********** diff --git a/src/surf/sg_platf.cpp b/src/surf/sg_platf.cpp index 3dc8bb3a30..22d8507240 100644 --- a/src/surf/sg_platf.cpp +++ b/src/surf/sg_platf.cpp @@ -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) diff --git a/src/surf/storage_n11.cpp b/src/surf/storage_n11.cpp index dd83d42ea3..50439d8426 100644 --- a/src/surf/storage_n11.cpp +++ b/src/surf/storage_n11.cpp @@ -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; -- 2.20.1