Modify Request Manager Duplication Checks

[Kartik Nema (kartnema)]

No description provided.

[qualcomm-ai-code-review-assistant]

Qualcomm AI Review

Click to expand Code Review

Reviewed Commits: 25c7093

• 25c7093: Modify Request Manager Duplication Checks

Signed-off-by: Kartik Nema kartnema@qti.qualcomm.com

Pull Request Overview

This PR adds a new mSource field to the Request class to track the source/origin of requests (specifically signal code and type). The changes also improve the duplicate request detection logic in RequestManager to consider the source field and fix the matching algorithm's return logic.

Files Changed Summary

File	Lines Changed	Issues Found	Highest Severity
modula/Common/Include/Request.h	+3	0	N/A
modula/Common/Request.cpp	+8	0	N/A
resource-tuner/core/RequestManager.cpp	+20	2	High
resource-tuner/signals/SignalHandler.cpp	+5	0	N/A

Critical Issues Identified

1. [High Severity - Functionality] Logic error in RequestManager::requestMatch() - The original implementation incorrectly returned true when no match was found, and false when resources didn't match. The PR fixes this but the fix reveals the original bug was critical.

2. [High Severity - Concurrency] Race condition in RequestManager::getRequestProcessingStatus() - Method accesses mActiveRequests without acquiring the shared lock, potentially causing data races in multi-threaded scenarios.

Positive Changes

• Proper initialization of mSource field in constructor
• Improved duplicate detection logic with source comparison
• Fixed inverted return logic in request matching

[Concurrency - High Severity] Race condition in getRequestProcessingStatus method

The getRequestProcessingStatus() method in RequestManager.cpp accesses the mActiveRequests map without acquiring any lock protection. This method is called from requestMatch() which holds a shared lock, but getRequestProcessingStatus() itself doesn't acquire the lock before accessing the shared data structure. This creates a race condition where the map could be modified by another thread between the lock release in the caller and the access in this method.

In a multi-threaded environment, this can lead to:

• Reading inconsistent/corrupted data
• Segmentation faults if the map is being modified
• Undefined behavior

The method should acquire mRequestMapMutex in shared mode before accessing mActiveRequests.

Fixed Code Snippet:

int8_t RequestManager::getRequestProcessingStatus(int64_t handle) {
    this->mRequestMapMutex.lock_shared();
    if(this->mActiveRequests.find(handle) != this->mActiveRequests.end()) {
        int8_t status = this->mActiveRequests[handle].second;
        this->mRequestMapMutex.unlock_shared();
        return status;
    }
    this->mRequestMapMutex.unlock_shared();
    return REQ_NOT_FOUND;
}

[Performance - Medium Severity] Redundant map lookup in requestMatch method

In the requestMatch() method at line 107-109, the code performs a find() operation on mActiveRequests and then immediately accesses the element using it->second.first. However, this lookup is redundant because getRequestProcessingStatus() was just called at line 97, which also performs a lookup on the same map with the same key.

This results in:

• Unnecessary hash computation and map traversal
• Degraded performance when checking multiple handles
• Inefficient use of CPU cycles

The code should be refactored to retrieve both the status and the request pointer in a single lookup operation, or getRequestProcessingStatus() should be modified to return both pieces of information.

Fixed Code Snippet:

for(int64_t handle: *clientHandles) {
    auto it = this->mActiveRequests.find(handle);
    if(it == this->mActiveRequests.end()) {
        continue;
    }

    int8_t requestProcessingStatus = it->second.second;
    // Only check for requests which haven't been completed or cancelled
    if((requestProcessingStatus & REQ_CANCELLED) || (requestProcessingStatus & REQ_COMPLETED)) {
        continue;
    }

    Request* targetRequest = it->second.first;
    if(targetRequest == nullptr) {
        continue;
    }
    // ... rest of the logic
}

---

⚠️ This review was generated using AI assistance. Please verify all suggestions before applying.

[qualcomm-ai-code-review-assistant]

Qualcomm AI Review

[qualcomm-ai-code-review-assistant]

[Performance - Medium Severity] Redundant map lookup in requestMatch method

In the requestMatch() method at line 107-109, the code performs a find() operation on mActiveRequests and then immediately accesses the element using it->second.first. However, this lookup is redundant because getRequestProcessingStatus() was just called at line 97, which also performs a lookup on the same map with the same key.

This results in:

• Unnecessary hash computation and map traversal
• Degraded performance when checking multiple handles
• Inefficient use of CPU cycles

The code should be refactored to retrieve both the status and the request pointer in a single lookup operation, or getRequestProcessingStatus() should be modified to return both pieces of information.

Fixed Code Snippet

for(int64_t handle: *clientHandles) {
    auto it = this->mActiveRequests.find(handle);
    if(it == this->mActiveRequests.end()) {
        continue;
    }

    int8_t requestProcessingStatus = it->second.second;
    // Only check for requests which haven't been completed or cancelled
    if((requestProcessingStatus & REQ_CANCELLED) || (requestProcessingStatus & REQ_COMPLETED)) {
        continue;
    }

    Request* targetRequest = it->second.first;
    if(targetRequest == nullptr) {
        continue;
    }
    // ... rest of the logic
}

[qualcomm-ai-code-review-assistant]

Qualcomm AI Review

Click to expand Code Review

Reviewed commit: 384143f "Modify Request Manager Duplication Checks

Signed-off-by: Kartik Nema kartnema@qti.qualcomm.com"

[CONCURRENCY] Race condition in requestMatch() - High Severity

In RequestManager::requestMatch(), the method calls getRequestProcessingStatus() which accesses mActiveRequests without holding any lock, but the calling method holds only a shared lock. Later in the same method, mActiveRequests is accessed directly at line 117-119. This creates a race condition where the map could be modified between the status check and the actual access, potentially causing undefined behavior or crashes.

The getRequestProcessingStatus() method (lines 271-277) directly accesses mActiveRequests without acquiring any lock, which is unsafe when called from requestMatch() which only holds a shared lock acquired later in shouldRequestBeAdded().

Fixed Code Snippet:

int8_t RequestManager::requestMatch(Request* request) {
    int32_t clientTID = request->getClientTID();

    std::shared_ptr<ClientDataManager> clientDataManager =
            ClientDataManager::getInstance();

    std::unordered_set<int64_t>* clientHandles =
        clientDataManager->getRequestsByClientID(clientTID);

    if(clientHandles == nullptr || clientHandles->size() == 0) {
        return false;
    }

    // Acquire lock before accessing mActiveRequests
    this->mRequestMapMutex.lock_shared();
    
    for(int64_t handle: *clientHandles) {
        auto it = this->mActiveRequests.find(handle);
        if(it == this->mActiveRequests.end()) {
            continue;
        }
        
        int8_t requestProcessingStatus = it->second.second;
        
        int8_t clientPermission =
            clientDataManager->getClientLevelByID(request->getClientPID());

        if(clientPermission == PERMISSION_SYSTEM) {
            if((requestProcessingStatus & REQ_CANCELLED) ||
               (requestProcessingStatus & REQ_COMPLETED)) {
                continue;
            }
        }

        Request* targetRequest = it->second.first;
        if(targetRequest == nullptr) {
            continue;
        }

        if(request->getSource() != targetRequest->getSource()) {
            continue;
        }

        if(request->getResourcesCount() != targetRequest->getResourcesCount()) {
            continue;
        }

        if(!request->getResDlMgr()->matchAgainst(
            targetRequest->getResDlMgr(), resourceCmpPolicy)) {
            continue;
        }
        
        this->mRequestMapMutex.unlock_shared();
        return true;
    }

    this->mRequestMapMutex.unlock_shared();
    return false;
}

[FUNCTIONALITY] Incorrect duplicate detection logic in shouldRequestBeAdded() - High Severity

In RequestManager::shouldRequestBeAdded() at line 174, the method calls requestMatch() to check for duplicates. However, there's a critical issue with lock management: the method acquires a shared lock at line 167, then calls requestMatch() which now needs to acquire its own lock (as per the fix above), and finally releases the lock at line 176.

The current implementation has shouldRequestBeAdded() holding a shared lock while calling requestMatch(), which would cause a deadlock if requestMatch() tries to acquire the same lock. The lock should be released before calling requestMatch() since requestMatch() will handle its own locking.

Fixed Code Snippet:

int8_t RequestManager::shouldRequestBeAdded(Request* request) {
    //sanity check.
    if(!isSane(request)) return false;

    this->mRequestMapMutex.lock_shared();
    if(this->mActiveRequests.size() >= UrmSettings::metaConfigs.mMaxConcurrentRequests) {
        this->mRequestMapMutex.unlock_shared();
        return false;
    }
    this->mRequestMapMutex.unlock_shared();

    // Check for duplicates - requestMatch handles its own locking
    int8_t duplicateFound = this->requestMatch(request);

    return !duplicateFound;
}

---

⚠️ This review was generated using AI assistance. Please verify all suggestions before applying.

[qualcomm-ai-code-review-assistant]

Error Summary

All 1 suggested inline comments were outside the diff context and were skipped

Qualcomm AI Review