From 62803a81200048149cd69628f1f8696e54c0973b Mon Sep 17 00:00:00 2001 From: Peng Xu Date: Wed, 26 Apr 2017 17:17:55 -0700 Subject: Correct inotify usage and fix strong count accounting error Fix an error in polling of inotify fd to avoid 100% CPU usage. Refactored code to use android Looper. Fix a string count accounting error that causes unpaired decStrong crashing sensor hidl service. Bug: 37719320 Bug: 37714835 Test: no longer have 100% CPU usage. Test: tested connection of a few mouse/keyboard/usb disk/gnubby no crash observed. Test: cts still working properly after fix. Change-Id: Ibaa026151e5e4919d8dd134c16f5865d5e30ef8a (cherry picked from commit 172c4014f30c6d77586b1c8617682110b2b74f95) --- .../sensors/dynamic_sensor/ConnectionDetector.cpp | 129 ++++++++++++--------- .../sensors/dynamic_sensor/ConnectionDetector.h | 6 +- .../sensors/dynamic_sensor/HidRawSensorDevice.cpp | 10 +- 3 files changed, 87 insertions(+), 58 deletions(-) diff --git a/modules/sensors/dynamic_sensor/ConnectionDetector.cpp b/modules/sensors/dynamic_sensor/ConnectionDetector.cpp index 67a6f272..c51e9125 100644 --- a/modules/sensors/dynamic_sensor/ConnectionDetector.cpp +++ b/modules/sensors/dynamic_sensor/ConnectionDetector.cpp @@ -105,30 +105,35 @@ bool SocketConnectionDetector::threadLoop() { // FileConnectionDetector functions FileConnectionDetector::FileConnectionDetector ( BaseDynamicSensorDaemon *d, const std::string &path, const std::string ®ex) - : ConnectionDetector(d), Thread(false /*callCallJava*/), mPath(path), mRegex(regex) { + : ConnectionDetector(d), Thread(false /*callCallJava*/), mPath(path), mRegex(regex), + mLooper(new Looper(true /*allowNonCallback*/)), mInotifyFd(-1) { + if (mLooper == nullptr) { + return; + } + mInotifyFd = ::inotify_init1(IN_NONBLOCK); if (mInotifyFd < 0) { ALOGE("Cannot init inotify"); return; } - int wd = ::inotify_add_watch(mInotifyFd, path.c_str(), IN_CREATE | IN_DELETE | IN_MOVED_FROM); - if (wd < 0) { + int wd = ::inotify_add_watch(mInotifyFd, path.c_str(), IN_CREATE | IN_DELETE); + if (wd < 0 || !mLooper->addFd(mInotifyFd, POLL_IDENT, Looper::EVENT_INPUT, nullptr, nullptr)) { ::close(mInotifyFd); mInotifyFd = -1; ALOGE("Cannot setup watch on dir %s", path.c_str()); return; } - mPollFd.fd = wd; - mPollFd.events = POLLIN; - + // mLooper != null && mInotifyFd added to looper run("ddad_file"); } FileConnectionDetector::~FileConnectionDetector() { - if (mInotifyFd) { - requestExitAndWait(); + if (mInotifyFd > 0) { + requestExit(); + mLooper->wake(); + join(); ::close(mInotifyFd); } } @@ -153,63 +158,81 @@ void FileConnectionDetector::processExistingFiles() const { ::closedir(dirp); } -bool FileConnectionDetector::threadLoop() { +void FileConnectionDetector::handleInotifyData(ssize_t len, const char *data) { + const char *dataEnd = data + len; + const struct inotify_event *ev; + + // inotify adds paddings to guarantee the next read is aligned + for (; data < dataEnd; data += sizeof(struct inotify_event) + ev->len) { + ev = reinterpret_cast(data); + if (ev->mask & IN_ISDIR) { + continue; + } + + const std::string name(ev->name); + if (matches(name)) { + if (ev->mask & IN_CREATE) { + mDaemon->onConnectionChange(getFullName(name), true /*connected*/); + } + if (ev->mask & IN_DELETE) { + mDaemon->onConnectionChange(getFullName(name), false /*connected*/); + } + } + } +} + +bool FileConnectionDetector::readInotifyData() { struct { - struct inotify_event e; - uint8_t padding[NAME_MAX + 1]; - } ev; + struct inotify_event ev; + char padding[NAME_MAX + 1]; + } buffer; + + bool ret = true; + while (true) { + ssize_t len = ::read(mInotifyFd, &buffer, sizeof(buffer)); + if (len == -1 && errno == EAGAIN) { + // no more data + break; + } else if (len > static_cast(sizeof(struct inotify_event))) { + handleInotifyData(len, reinterpret_cast(&buffer)); + } else if (len < 0) { + ALOGE("read error: %s", ::strerror(errno)); + ret = false; + break; + } else { + // 0 <= len <= sizeof(struct inotify_event) + ALOGE("read return %zd, shorter than inotify_event size %zu", + len, sizeof(struct inotify_event)); + ret = false; + break; + } + } + return ret; +} +bool FileConnectionDetector::threadLoop() { + Looper::setForThread(mLooper); processExistingFiles(); + while(!Thread::exitPending()) { + int ret = mLooper->pollOnce(-1); - while (!Thread::exitPending()) { - int pollNum = ::poll(&mPollFd, 1, -1); - if (pollNum == -1) { - if (errno == EINTR) - continue; - ALOGE("inotify poll error: %s", ::strerror(errno)); + if (ret != Looper::POLL_WAKE && ret != POLL_IDENT) { + ALOGE("Unexpected value %d from pollOnce, quit", ret); + requestExit(); + break; } - if (pollNum > 0) { - if (! (mPollFd.revents & POLLIN)) { - continue; - } - - /* Inotify events are available */ - while (true) { - /* Read some events. */ - ssize_t len = ::read(mInotifyFd, &ev, sizeof ev); - if (len == -1 && errno != EAGAIN) { - ALOGE("read error: %s", ::strerror(errno)); - requestExit(); - break; - } - - /* If the nonblocking read() found no events to read, then - it returns -1 with errno set to EAGAIN. In that case, - we exit the loop. */ - if (len <= 0) { - break; - } - - if (ev.e.len && !(ev.e.mask & IN_ISDIR)) { - const std::string name(ev.e.name); - ALOGV("device %s state changed", name.c_str()); - if (matches(name)) { - if (ev.e.mask & IN_CREATE) { - mDaemon->onConnectionChange(getFullName(name), true /* connected*/); - } - - if (ev.e.mask & IN_DELETE || ev.e.mask & IN_MOVED_FROM) { - mDaemon->onConnectionChange(getFullName(name), false /* connected*/); - } - } - } + if (ret == POLL_IDENT) { + if (!readInotifyData()) { + requestExit(); } } } + mLooper->removeFd(mInotifyFd); ALOGD("FileConnectionDetection thread exited"); return false; } + } // namespace SensorHalExt } // namespace android diff --git a/modules/sensors/dynamic_sensor/ConnectionDetector.h b/modules/sensors/dynamic_sensor/ConnectionDetector.h index 712d832f..0ee1df29 100644 --- a/modules/sensors/dynamic_sensor/ConnectionDetector.h +++ b/modules/sensors/dynamic_sensor/ConnectionDetector.h @@ -19,6 +19,7 @@ #include "BaseDynamicSensorDaemon.h" #include +#include #include @@ -62,17 +63,20 @@ public: BaseDynamicSensorDaemon *d, const std::string &path, const std::string ®ex); virtual ~FileConnectionDetector(); private: + static constexpr int POLL_IDENT = 1; // implement virtual of Thread virtual bool threadLoop(); bool matches(const std::string &name) const; void processExistingFiles() const; + void handleInotifyData(ssize_t len, const char *data); + bool readInotifyData(); std::string getFullName(const std::string name) const; std::string mPath; std::regex mRegex; + sp mLooper; int mInotifyFd; - struct pollfd mPollFd; }; } // namespace SensorHalExt diff --git a/modules/sensors/dynamic_sensor/HidRawSensorDevice.cpp b/modules/sensors/dynamic_sensor/HidRawSensorDevice.cpp index 16e9060a..8aa4cf92 100644 --- a/modules/sensors/dynamic_sensor/HidRawSensorDevice.cpp +++ b/modules/sensors/dynamic_sensor/HidRawSensorDevice.cpp @@ -37,7 +37,7 @@ const std::unordered_set HidRawSensorDevice::sInterested{ sp HidRawSensorDevice::create(const std::string &devName) { sp device(new HidRawSensorDevice(devName)); - // remove +1 strong count added by constructor + // offset +1 strong count added by constructor device->decStrong(device.get()); if (device->mValid) { @@ -50,13 +50,15 @@ sp HidRawSensorDevice::create(const std::string &devName) { HidRawSensorDevice::HidRawSensorDevice(const std::string &devName) : RefBase(), HidRawDevice(devName, sInterested), Thread(false /*canCallJava*/), mValid(false) { - if (!HidRawDevice::isValid()) { - return; - } // create HidRawSensor objects from digest // HidRawSensor object will take sp as parameter, so increment strong count // to prevent "this" being destructed. this->incStrong(this); + + if (!HidRawDevice::isValid()) { + return; + } + for (const auto &digest : mDigestVector) { // for each usage - vec pair uint32_t usage = static_cast(digest.fullUsage); sp s(new HidRawSensor(this, usage, digest.packets)); -- cgit v1.2.3