meta-quanta/meta-gbs/recipes-phosphor/sensors/phosphor-hwmon/0001-sensor-Implement-sensor-ASYNC_READ_TIMEOUT.patch

From c1b3d32430df12ab01f9cacf132aedff8324d55b Mon Sep 17 00:00:00 2001
From: Brandon Kim <brandonkim@google.com>
Date: Fri, 9 Aug 2019 15:38:53 -0700
Subject: [PATCH] sensor: Implement sensor "ASYNC_READ_TIMEOUT"

This change will prevent sensors from blocking all other sensor reads
and D-Bus if they do not report failures quickly enough.

If "ASYNC_READ_TIMEOUT" environment variable is defined in the
sensor's config file for a key_type, the sensor read will be
asynchronous with timeout set in milliseconds.

For example for "sensor1":
ASYNC_READ_TIMEOUT_sensor1="1000"  // Timeout will be set to 1 sec

If the read times out, the sensor read will be skipped and the
sensor's functional property will be set to 'false'. Timed out futures
will be placed in a map to prevent their destructor from running and
blocking until the read completes (limiation of std::async).

Change-Id: I3d9ed4d5c9cc87d3196fc281451834f3001d0b48
Signed-off-by: Brandon Kim <brandonkim@google.com>
---
 Makefile.am  |  2 ++
 mainloop.cpp | 73 +++++++++++++++++++++++++++++++++++++++++++++++---
 mainloop.hpp |  3 +++
 meson.build  |  1 +
 sensor.cpp   | 75 ++++++++++++++++++++++++++++++++++++++++++++++------
 sensor.hpp   | 20 ++++++++++++--
 6 files changed, 160 insertions(+), 14 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 706a6cc..c620fa4 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -46,6 +46,7 @@ libhwmon_la_LIBADD = \
 	$(SDEVENTPLUS_LIBS) \
 	$(PHOSPHOR_DBUS_INTERFACES_LIBS) \
 	$(PHOSPHOR_LOGGING_LIBS) \
+	$(PTHREAD_LIBS) \
 	$(GPIOPLUS_LIBS) \
 	$(STDPLUS_LIBS) \
 	$(CODE_COVERAGE_LIBS) \
@@ -55,6 +56,7 @@ libhwmon_la_CXXFLAGS = \
 	$(SDEVENTPLUS_CFLAGS) \
 	$(PHOSPHOR_DBUS_INTERFACES_CFLAGS) \
 	$(PHOSPHOR_LOGGING_CFLAGS) \
+	$(PTHREAD_CFLAGS) \
 	$(STDPLUS_CFLAGS) \
 	$(CODE_COVERAGE_CXXFLAGS)
 
diff --git a/mainloop.cpp b/mainloop.cpp
index ecceee5..29dc26a 100644
--- a/mainloop.cpp
+++ b/mainloop.cpp
@@ -34,6 +34,7 @@
 #include <cassert>
 #include <cstdlib>
 #include <functional>
+#include <future>
 #include <iostream>
 #include <memory>
 #include <phosphor-logging/elog-errors.hpp>
@@ -242,7 +243,7 @@ std::optional<ObjectStateData>
     {
         // Add status interface based on _fault file being present
         sensorObj->addStatus(info);
-        valueInterface = sensorObj->addValue(retryIO, info);
+        valueInterface = sensorObj->addValue(retryIO, info, _timedoutMap);
     }
     catch (const std::system_error& e)
     {
@@ -478,10 +479,74 @@ void MainLoop::read()
                 // RAII object for GPIO unlock / lock
                 auto locker = sensor::gpioUnlock(sensor->getGpio());
 
-                // Retry for up to a second if device is busy
-                // or has a transient error.
-                value = _ioAccess->read(sensorSysfsType, sensorSysfsNum, input,
+                // For sensors with attribute ASYNC_READ_TIMEOUT,
+                // spawn a thread with timeout
+                auto asyncRead =
+                    env::getEnv("ASYNC_READ_TIMEOUT", sensorSetKey);
+                if (!asyncRead.empty())
+                {
+                    // Default async read timeout
+                    std::chrono::milliseconds asyncReadTimeout{
+                        std::stoi(asyncRead)};
+                    bool valueIsValid = false;
+                    std::future<int64_t> asyncThread;
+
+                    auto asyncIter = _timedoutMap.find(sensorSetKey);
+                    if (asyncIter == _timedoutMap.end())
+                    {
+                        // If sensor not found in timedoutMap, spawn an async
+                        // thread
+                        asyncThread = std::async(
+                            std::launch::async,
+                            &hwmonio::HwmonIOInterface::read, _ioAccess,
+                            sensorSysfsType, sensorSysfsNum, input,
+                            hwmonio::retries, hwmonio::delay);
+                        valueIsValid = true;
+                    }
+                    else
+                    {
+                        // If we already have the async thread in the
+                        // timedoutMap, it means this sensor has already timed
+                        // out in the previous reads. No need to wait on
+                        // subsequent reads
+                        asyncReadTimeout = std::chrono::seconds(0);
+                        asyncThread = std::move(asyncIter->second);
+                    }
+
+                    std::future_status status =
+                        asyncThread.wait_for(asyncReadTimeout);
+                    switch (status)
+                    {
+                        // Read has finished
+                        case std::future_status::ready:
+                            // Read has finished
+                            if (valueIsValid)
+                            {
+                                value = asyncThread.get();
+                                break;
+                                // Good sensor reads should skip the code below
+                            }
+                            // Async read thread has completed, erase from
+                            // timedoutMap to allow retry then throw
+                            _timedoutMap.erase(sensorSetKey);
+                            throw sensor::AsyncSensorReadTimeOut();
+                        default:
+                            // Read timed out so add the thread to the
+                            // timedoutMap (if the entry already exists,
+                            // operator[] updates it)
+                            _timedoutMap[sensorSetKey] = std::move(asyncThread);
+                            throw sensor::AsyncSensorReadTimeOut();
+                    }
+                }
+                else
+                {
+                    // Retry for up to a second if device is busy
+                    // or has a transient error.
+                    value =
+                        _ioAccess->read(sensorSysfsType, sensorSysfsNum, input,
                                         hwmonio::retries, hwmonio::delay);
+                }
+
                 // Set functional property to true if we could read sensor
                 statusIface->functional(true);
 
diff --git a/mainloop.hpp b/mainloop.hpp
index b3de022..6803c4b 100644
--- a/mainloop.hpp
+++ b/mainloop.hpp
@@ -9,6 +9,7 @@
 #include "types.hpp"
 
 #include <any>
+#include <future>
 #include <memory>
 #include <optional>
 #include <sdbusplus/server.hpp>
@@ -116,6 +117,8 @@ class MainLoop
     /** @brief Store the specifications of sensor objects */
     std::map<SensorSet::key_type, std::unique_ptr<sensor::Sensor>>
         _sensorObjects;
+    /** @brief Store the async futures of timed out sensor objects */
+    std::map<SensorSet::key_type, std::future<int64_t>> _timedoutMap;
 
     /**
      * @brief Map of removed sensors
diff --git a/meson.build b/meson.build
index 66e6801..d6a92f8 100644
--- a/meson.build
+++ b/meson.build
@@ -84,6 +84,7 @@ libhwmon_all = static_library(
         gpioplus,
         phosphor_dbus_interfaces,
         phosphor_logging,
+        threads,
     ],
     link_with: [
         libaverage,
diff --git a/sensor.cpp b/sensor.cpp
index 09aeca6..ac2f896 100644
--- a/sensor.cpp
+++ b/sensor.cpp
@@ -15,6 +15,7 @@
 #include <cmath>
 #include <cstring>
 #include <filesystem>
+#include <future>
 #include <phosphor-logging/elog-errors.hpp>
 #include <thread>
 #include <xyz/openbmc_project/Common/error.hpp>
@@ -116,8 +117,9 @@ SensorValueType Sensor::adjustValue(SensorValueType value)
     return value;
 }
 
-std::shared_ptr<ValueObject> Sensor::addValue(const RetryIO& retryIO,
-                                              ObjectInfo& info)
+std::shared_ptr<ValueObject> Sensor::addValue(
+    const RetryIO& retryIO, ObjectInfo& info,
+    std::map<SensorSet::key_type, std::future<int64_t>>& timedoutMap)
 {
     static constexpr bool deferSignals = true;
 
@@ -144,12 +146,69 @@ std::shared_ptr<ValueObject> Sensor::addValue(const RetryIO& retryIO,
             // RAII object for GPIO unlock / lock
             auto locker = gpioUnlock(getGpio());
 
-            // Retry for up to a second if device is busy
-            // or has a transient error.
-            val =
-                _ioAccess->read(_sensor.first, _sensor.second,
-                                hwmon::entry::cinput, std::get<size_t>(retryIO),
-                                std::get<std::chrono::milliseconds>(retryIO));
+            // For sensors with attribute ASYNC_READ_TIMEOUT,
+            // spawn a thread with timeout
+            auto asyncRead = env::getEnv("ASYNC_READ_TIMEOUT", _sensor);
+            if (!asyncRead.empty())
+            {
+                // Default async read timeout
+                std::chrono::milliseconds asyncReadTimeout{
+                    std::stoi(asyncRead)};
+                bool valueIsValid = false;
+                std::future<int64_t> asyncThread;
+
+                auto asyncIter = timedoutMap.find(_sensor);
+                if (asyncIter == timedoutMap.end())
+                {
+                    // If sensor not found in timedoutMap, spawn an async thread
+                    asyncThread = std::async(
+                        std::launch::async, &hwmonio::HwmonIOInterface::read,
+                        _ioAccess, _sensor.first, _sensor.second,
+                        hwmon::entry::cinput, std::get<size_t>(retryIO),
+                        std::get<std::chrono::milliseconds>(retryIO));
+                    valueIsValid = true;
+                }
+                else
+                {
+                    // If we already have the async thread in the timedoutMap,
+                    // it means this sensor has already timed out in the
+                    // previous reads. No need to wait on subsequent reads
+                    asyncReadTimeout = std::chrono::seconds(0);
+                    asyncThread = std::move(asyncIter->second);
+                }
+
+                std::future_status status =
+                    asyncThread.wait_for(asyncReadTimeout);
+                switch (status)
+                {
+                    case std::future_status::ready:
+                        // Read has finished
+                        if (valueIsValid)
+                        {
+                            val = asyncThread.get();
+                            break;
+                            // Good sensor reads should skip the code below
+                        }
+                        // Async read thread has completed, erase from
+                        // timedoutMap to allow retry then throw
+                        timedoutMap.erase(_sensor);
+                        throw AsyncSensorReadTimeOut();
+                    default:
+                        // Read timed out so add the thread to the timedoutMap
+                        // (if the entry already exists, operator[] updates it)
+                        timedoutMap[_sensor] = std::move(asyncThread);
+                        throw AsyncSensorReadTimeOut();
+                }
+            }
+            else
+            {
+                // Retry for up to a second if device is busy
+                // or has a transient error.
+                val = _ioAccess->read(
+                    _sensor.first, _sensor.second, hwmon::entry::cinput,
+                    std::get<size_t>(retryIO),
+                    std::get<std::chrono::milliseconds>(retryIO));
+            }
         }
 #ifdef UPDATE_FUNCTIONAL_ON_FAIL
         catch (const std::system_error& e)
diff --git a/sensor.hpp b/sensor.hpp
index 4b2d281..64d6e48 100644
--- a/sensor.hpp
+++ b/sensor.hpp
@@ -4,6 +4,8 @@
 #include "sensorset.hpp"
 #include "types.hpp"
 
+#include <cerrno>
+#include <future>
 #include <gpioplus/handle.hpp>
 #include <memory>
 #include <optional>
@@ -20,6 +22,17 @@ struct valueAdjust
     std::unordered_set<int> rmRCs;
 };
 
+/** @brief Custom exception for async sensor reading timeout
+ */
+struct AsyncSensorReadTimeOut : public std::system_error
+{
+    AsyncSensorReadTimeOut() :
+        system_error(std::error_code(ETIMEDOUT, std::system_category()),
+                     "Async sensor read timed out")
+    {
+    }
+};
+
 /** @class Sensor
  *  @brief Sensor object based on a SensorSet container's key type
  *  @details Sensor object to create and modify an associated device's sensor
@@ -87,10 +100,13 @@ class Sensor
      *                      (number of and delay between)
      * @param[in] info - Sensor object information
      *
+     * @param[in] timedoutMap - Map to track timed out threads
+     *
      * @return - Shared pointer to the value object
      */
-    std::shared_ptr<ValueObject> addValue(const RetryIO& retryIO,
-                                          ObjectInfo& info);
+    std::shared_ptr<ValueObject> addValue(
+        const RetryIO& retryIO, ObjectInfo& info,
+        std::map<SensorSet::key_type, std::future<int64_t>>& timedoutMap);
 
     /**
      * @brief Add status interface and functional property for sensor
-- 
2.21.0