From fd1e8a0328169ee2c485055a4f976fa76d690af0 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Wed, 21 Jul 2021 23:30:29 +0000 Subject: libbinder: dec refs include count This is in preparation for changing how/when we decrement refcounts. The difference in this CL is that the wire format includes a count with an easy mechanism to set the count we want. The difference in behavior from this CL is that sometimes, multiple decStrong refcounts will be combined into one. In the future, we'll want to combine multiple of these. Bug: 182940634 Test: binderRpcTest (on host and also on Pixel 3) Change-Id: I2339a8e0fff3c35ce3c5f562d590da7a12b6670a --- libs/binder/RpcState.cpp | 59 ++++++++++++++++++++++++++++++++---------------- 1 file changed, 40 insertions(+), 19 deletions(-) (limited to 'libs/binder/RpcState.cpp') diff --git a/libs/binder/RpcState.cpp b/libs/binder/RpcState.cpp index df935fe9e3..1838e81dd6 100644 --- a/libs/binder/RpcState.cpp +++ b/libs/binder/RpcState.cpp @@ -163,10 +163,17 @@ status_t RpcState::onBinderEntering(const sp& session, uint64_t addr _l.unlock(); + // if this is a local binder, then we want to get rid of all refcounts + // (tell the other process it can drop the binder when it wants to - we + // have a local sp<>, so we will drop it when we want to as well). if + // this is a remote binder, then we need to hold onto one refcount until + // it is dropped in BpBinder::onLastStrongRef + size_t targetRecd = (*out)->localBinder() ? 0 : 1; + // We have timesRecd RPC refcounts, but we only need to hold on to one // when we keep the object. All additional dec strongs are sent // immediately, we wait to send the last one in BpBinder::onLastDecStrong. - return session->sendDecStrong(address); + return session->sendDecStrongToTarget(address, targetRecd); } // we don't know about this binder, so the other side of the connection @@ -567,32 +574,43 @@ status_t RpcState::waitForReply(const sp& connection, return OK; } -status_t RpcState::sendDecStrong(const sp& connection, - const sp& session, uint64_t addr) { +status_t RpcState::sendDecStrongToTarget(const sp& connection, + const sp& session, uint64_t addr, + size_t target) { + RpcDecStrong body = { + .address = RpcWireAddress::fromRaw(addr), + }; + { std::lock_guard _l(mNodeMutex); if (mTerminated) return DEAD_OBJECT; // avoid fatal only, otherwise races auto it = mNodeForAddress.find(addr); LOG_ALWAYS_FATAL_IF(it == mNodeForAddress.end(), "Sending dec strong on unknown address %" PRIu64, addr); - LOG_ALWAYS_FATAL_IF(it->second.timesRecd <= 0, "Bad dec strong %" PRIu64, addr); - it->second.timesRecd--; + LOG_ALWAYS_FATAL_IF(it->second.timesRecd < target, "Can't dec count of %zu to %zu.", + it->second.timesRecd, target); + + // typically this happens when multiple threads send dec refs at the + // same time - the transactions will get combined automatically + if (it->second.timesRecd == target) return OK; + + body.amount = it->second.timesRecd - target; + it->second.timesRecd = target; + LOG_ALWAYS_FATAL_IF(nullptr != tryEraseNode(it), "Bad state. RpcState shouldn't own received binder"); } RpcWireHeader cmd = { .command = RPC_COMMAND_DEC_STRONG, - .bodySize = sizeof(RpcWireAddress), + .bodySize = sizeof(RpcDecStrong), }; if (status_t status = rpcSend(connection, session, "dec ref header", &cmd, sizeof(cmd)); status != OK) return status; - if (status_t status = rpcSend(connection, session, "dec ref body", &addr, sizeof(addr)); - status != OK) - return status; - return OK; + + return rpcSend(connection, session, "dec ref body", &body, sizeof(body)); } status_t RpcState::getAndExecuteCommand(const sp& connection, @@ -912,16 +930,15 @@ status_t RpcState::processDecStrong(const sp& connect status != OK) return status; - if (command.bodySize != sizeof(RpcWireAddress)) { - ALOGE("Expecting %zu but got %" PRId32 " bytes for RpcWireAddress. Terminating!", - sizeof(RpcWireAddress), command.bodySize); + if (command.bodySize != sizeof(RpcDecStrong)) { + ALOGE("Expecting %zu but got %" PRId32 " bytes for RpcDecStrong. Terminating!", + sizeof(RpcDecStrong), command.bodySize); (void)session->shutdownAndWait(false); return BAD_VALUE; } - RpcWireAddress* address = reinterpret_cast(commandData.data()); - - uint64_t addr = RpcWireAddress::toRaw(*address); + RpcDecStrong* body = reinterpret_cast(commandData.data()); + uint64_t addr = RpcWireAddress::toRaw(body->address); std::unique_lock _l(mNodeMutex); auto it = mNodeForAddress.find(addr); if (it == mNodeForAddress.end()) { @@ -939,15 +956,19 @@ status_t RpcState::processDecStrong(const sp& connect return BAD_VALUE; } - if (it->second.timesSent == 0) { - ALOGE("No record of sending binder, but requested decStrong: %" PRIu64, addr); + if (it->second.timesSent < body->amount) { + ALOGE("Record of sending binder %zu times, but requested decStrong for %" PRIu64 " of %u", + it->second.timesSent, addr, body->amount); return OK; } LOG_ALWAYS_FATAL_IF(it->second.sentRef == nullptr, "Inconsistent state, lost ref for %" PRIu64, addr); - it->second.timesSent--; + LOG_RPC_DETAIL("Processing dec strong of %" PRIu64 " by %u from %zu", addr, body->amount, + it->second.timesSent); + + it->second.timesSent -= body->amount; sp tempHold = tryEraseNode(it); _l.unlock(); tempHold = nullptr; // destructor may make binder calls on this session -- cgit v1.2.3-59-g8ed1b From d8083310534b157c7e303083df15fe9b362e4913 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Wed, 22 Sep 2021 13:37:10 -0700 Subject: libbinder: oneway calls delay refcounting For oneway calls, we might be processing many at the same time. We can avoid sending any decref calls until we finish processing all oneway transactions. So, for instance, if we send 100 transactions, and they can all be processed at the same time, we'll only write one response to update the refcount in the caller process. Note: this was written to try to fix the deadlock in "libbinder: RPC avoid poll", but it does not. Still, I think it's a good improvement. Bug: 182940634 Test: binderRpcTest Change-Id: Ieb5374127cc79222b09f5efb12436211f609c9bb --- libs/binder/Parcel.cpp | 3 +++ libs/binder/RpcState.cpp | 60 ++++++++++++++++++++++++++++++++++++------------ libs/binder/RpcState.h | 7 ++++++ 3 files changed, 55 insertions(+), 15 deletions(-) (limited to 'libs/binder/RpcState.cpp') diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp index 6644187507..6ce09226d9 100644 --- a/libs/binder/Parcel.cpp +++ b/libs/binder/Parcel.cpp @@ -291,6 +291,9 @@ status_t Parcel::unflattenBinder(sp* out) const if (status_t status = mSession->state()->onBinderEntering(mSession, addr, &binder); status != OK) return status; + if (status_t status = mSession->state()->flushExcessBinderRefs(mSession, addr, binder); + status != OK) + return status; } return finishUnflattenBinder(binder, out); diff --git a/libs/binder/RpcState.cpp b/libs/binder/RpcState.cpp index 1838e81dd6..1df94a66f3 100644 --- a/libs/binder/RpcState.cpp +++ b/libs/binder/RpcState.cpp @@ -152,7 +152,7 @@ status_t RpcState::onBinderEntering(const sp& session, uint64_t addr return BAD_VALUE; } - std::unique_lock _l(mNodeMutex); + std::lock_guard _l(mNodeMutex); if (mTerminated) return DEAD_OBJECT; if (auto it = mNodeForAddress.find(address); it != mNodeForAddress.end()) { @@ -160,20 +160,7 @@ status_t RpcState::onBinderEntering(const sp& session, uint64_t addr // implicitly have strong RPC refcount, since we received this binder it->second.timesRecd++; - - _l.unlock(); - - // if this is a local binder, then we want to get rid of all refcounts - // (tell the other process it can drop the binder when it wants to - we - // have a local sp<>, so we will drop it when we want to as well). if - // this is a remote binder, then we need to hold onto one refcount until - // it is dropped in BpBinder::onLastStrongRef - size_t targetRecd = (*out)->localBinder() ? 0 : 1; - - // We have timesRecd RPC refcounts, but we only need to hold on to one - // when we keep the object. All additional dec strongs are sent - // immediately, we wait to send the last one in BpBinder::onLastDecStrong. - return session->sendDecStrongToTarget(address, targetRecd); + return OK; } // we don't know about this binder, so the other side of the connection @@ -194,6 +181,36 @@ status_t RpcState::onBinderEntering(const sp& session, uint64_t addr return OK; } +status_t RpcState::flushExcessBinderRefs(const sp& session, uint64_t address, + const sp& binder) { + std::unique_lock _l(mNodeMutex); + if (mTerminated) return DEAD_OBJECT; + + auto it = mNodeForAddress.find(address); + + LOG_ALWAYS_FATAL_IF(it == mNodeForAddress.end(), "Can't be deleted while we hold sp<>"); + LOG_ALWAYS_FATAL_IF(it->second.binder != binder, + "Caller of flushExcessBinderRefs using inconsistent arguments"); + + // if this is a local binder, then we want to get rid of all refcounts + // (tell the other process it can drop the binder when it wants to - we + // have a local sp<>, so we will drop it when we want to as well). if + // this is a remote binder, then we need to hold onto one refcount until + // it is dropped in BpBinder::onLastStrongRef + size_t targetRecd = binder->localBinder() ? 0 : 1; + + // We have timesRecd RPC refcounts, but we only need to hold on to one + // when we keep the object. All additional dec strongs are sent + // immediately, we wait to send the last one in BpBinder::onLastDecStrong. + if (it->second.timesRecd != targetRecd) { + _l.unlock(); + + return session->sendDecStrongToTarget(address, targetRecd); + } + + return OK; +} + size_t RpcState::countBinders() { std::lock_guard _l(mNodeMutex); return mNodeForAddress.size(); @@ -841,6 +858,12 @@ processTransactInternalTailCall: } } + // Binder refs are flushed for oneway calls only after all calls which are + // built up are executed. Otherwise, they fill up the binder buffer. + if (addr != 0 && replyStatus == OK && !oneway) { + replyStatus = flushExcessBinderRefs(session, addr, target); + } + if (oneway) { if (replyStatus != OK) { ALOGW("Oneway call failed with error: %d", replyStatus); @@ -889,6 +912,13 @@ processTransactInternalTailCall: goto processTransactInternalTailCall; } } + + // done processing all the async commands on this binder that we can, so + // write decstrongs on the binder + if (addr != 0 && replyStatus == OK) { + return flushExcessBinderRefs(session, addr, target); + } + return OK; } diff --git a/libs/binder/RpcState.h b/libs/binder/RpcState.h index b21677084c..42e95e0c10 100644 --- a/libs/binder/RpcState.h +++ b/libs/binder/RpcState.h @@ -129,6 +129,13 @@ public: */ [[nodiscard]] status_t onBinderEntering(const sp& session, uint64_t address, sp* out); + /** + * Called on incoming binders to update refcounting information. This should + * only be called when it is done as part of making progress on a + * transaction. + */ + [[nodiscard]] status_t flushExcessBinderRefs(const sp& session, uint64_t address, + const sp& binder); size_t countBinders(); void dump(); -- cgit v1.2.3-59-g8ed1b