From 58e5daaed8b446bdcf937a5eb368d6623d33f423 Mon Sep 17 00:00:00 2001 From: Ganesh Mahendran Date: Wed, 11 Oct 2017 18:05:13 +0800 Subject: binder: remove unnecessary err check In function IPCThreadState::transact(), data.errorCheck() will be executed twice. Since IPCThreadState::transact() is the critical path for binder call, it is better to do data.errorCheck() once. This patch removes the first check at the beginning of IPCThreadState::transact(), the effect of this change is that LOG_ONEWAY(...) will be executed in error case("data.errorCheck() != NO_ERROR") I think this is not a problem. As "data.errorCheck() == NO_ERROR" is the normal case(which will execute LOG_ONEWAY()), and even in error case, we print a log about src/dst pid is not a bad idea. Change-Id: I7b892a2294774c55ce0df56edee6a820f82c6f13 Signed-off-by: Ganesh Mahendran --- libs/binder/IPCThreadState.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/libs/binder/IPCThreadState.cpp b/libs/binder/IPCThreadState.cpp index 1c3fab4057..ba9bf61f8c 100644 --- a/libs/binder/IPCThreadState.cpp +++ b/libs/binder/IPCThreadState.cpp @@ -571,7 +571,7 @@ status_t IPCThreadState::transact(int32_t handle, uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags) { - status_t err = data.errorCheck(); + status_t err; flags |= TF_ACCEPT_FDS; @@ -582,11 +582,9 @@ status_t IPCThreadState::transact(int32_t handle, << indent << data << dedent << endl; } - if (err == NO_ERROR) { - LOG_ONEWAY(">>>> SEND from pid %d uid %d %s", getpid(), getuid(), - (flags & TF_ONE_WAY) == 0 ? "READ REPLY" : "ONE WAY"); - err = writeTransactionData(BC_TRANSACTION, flags, handle, code, data, NULL); - } + LOG_ONEWAY(">>>> SEND from pid %d uid %d %s", getpid(), getuid(), + (flags & TF_ONE_WAY) == 0 ? "READ REPLY" : "ONE WAY"); + err = writeTransactionData(BC_TRANSACTION, flags, handle, code, data, NULL); if (err != NO_ERROR) { if (reply) reply->setError(err); -- cgit v1.2.3 From 0494d6ab6f4b85eb28eeb71737398ba0b862a822 Mon Sep 17 00:00:00 2001 From: Ganesh Mahendran Date: Thu, 2 Nov 2017 09:38:02 +0800 Subject: binder: send BC_REPLY and BC_FREE_BUFFER together In current BR_TRANSACTION handling logic, BC_REPLY and BC_FREE_BUFFER are sent to kernel seperately which may introduce latency for freeing buffer. I think it's better to free buffer asap, this change fixes this by putting BC_REPLY and BC_FREE_BUFFER in the same buffer. And then call waitForResponse() to send them to kernel together. After this, function sendReply() will not be called by anyone. So sendReply() is also removed. Below is the test result of "binderThroughputTest -w 100" Env: android 7.1.2, 2G ram --------------------- ---> base: iterations per sec: 31917.7 collecting results average:2.83375ms worst:572.205ms best:0.054219ms 50%: 2.14844 90%: 5.27344 95%: 6.83594 99%: 12.3047 iterations per sec: 32142.3 collecting results average:2.8186ms worst:550.884ms best:0.054948ms 50%: 2.53906 90%: 5.27344 95%: 6.83594 99%: 11.1328 iterations per sec: 31704.5 collecting results average:2.85129ms worst:600.984ms best:0.053854ms 50%: 2.53906 90%: 5.27344 95%: 6.83594 99%: 12.3047 iterations per sec: 31932.3 collecting results average:2.82549ms worst:563.098ms best:0.052396ms 50%: 2.53906 90%: 5.66406 95%: 6.83594 99%: 11.1328 ---> patched: iterations per sec: 32062.5 collecting results average:2.85475ms worst:599.177ms best:0.054895ms 50%: 2.14844 90%: 5.27344 95%: 6.44531 99%: 12.6953 iterations per sec: 32241.7 collecting results average:2.82851ms worst:634.984ms best:0.05651ms 50%: 2.53906 90%: 5.66406 95%: 6.83594 99%: 10.7422 iterations per sec: 31939 collecting results average:2.86589ms worst:628.861ms best:0.058645ms 50%: 2.53906 90%: 5.66406 95%: 6.83594 99%: 11.1328 iterations per sec: 32149.4 collecting results average:2.80785ms worst:658.889ms best:0.055573ms 50%: 2.53906 90%: 5.66406 95%: 6.83594 99%: 10.7422 It seems patched is better. Change-Id: I0ef88864f4dbc63f3cdd2eba345b5dce6140b2d3 Suggested-by: Martijn Coenen Signed-off-by: Ganesh Mahendran --- libs/binder/IPCThreadState.cpp | 124 ++++++++++++++-------------- libs/binder/include/binder/IPCThreadState.h | 1 - 2 files changed, 62 insertions(+), 63 deletions(-) diff --git a/libs/binder/IPCThreadState.cpp b/libs/binder/IPCThreadState.cpp index ba9bf61f8c..50974ef74f 100644 --- a/libs/binder/IPCThreadState.cpp +++ b/libs/binder/IPCThreadState.cpp @@ -718,16 +718,6 @@ IPCThreadState::~IPCThreadState() { } -status_t IPCThreadState::sendReply(const Parcel& reply, uint32_t flags) -{ - status_t err; - status_t statusBuffer; - err = writeTransactionData(BC_REPLY, flags, -1, 0, reply, &statusBuffer); - if (err < NO_ERROR) return err; - - return waitForResponse(NULL, NULL); -} - status_t IPCThreadState::waitForResponse(Parcel *reply, status_t *acquireResult) { uint32_t cmd; @@ -1050,68 +1040,78 @@ status_t IPCThreadState::executeCommand(int32_t cmd) "Not enough command data for brTRANSACTION"); if (result != NO_ERROR) break; - Parcel buffer; - buffer.ipcSetDataReference( - reinterpret_cast(tr.data.ptr.buffer), - tr.data_size, - reinterpret_cast(tr.data.ptr.offsets), - tr.offsets_size/sizeof(binder_size_t), freeBuffer, this); + Parcel reply; + { + Parcel buffer; + const pid_t origPid = mCallingPid; + const uid_t origUid = mCallingUid; + const int32_t origStrictModePolicy = mStrictModePolicy; + const int32_t origTransactionBinderFlags = mLastTransactionBinderFlags; + + buffer.ipcSetDataReference( + reinterpret_cast(tr.data.ptr.buffer), + tr.data_size, + reinterpret_cast(tr.data.ptr.offsets), + tr.offsets_size/sizeof(binder_size_t), freeBuffer, this); + + mCallingPid = tr.sender_pid; + mCallingUid = tr.sender_euid; + mLastTransactionBinderFlags = tr.flags; + + //ALOGI(">>>> TRANSACT from pid %d uid %d\n", mCallingPid, mCallingUid); + + status_t error; + IF_LOG_TRANSACTIONS() { + TextOutput::Bundle _b(alog); + alog << "BR_TRANSACTION thr " << (void*)pthread_self() + << " / obj " << tr.target.ptr << " / code " + << TypeCode(tr.code) << ": " << indent << buffer + << dedent << endl + << "Data addr = " + << reinterpret_cast(tr.data.ptr.buffer) + << ", offsets addr=" + << reinterpret_cast(tr.data.ptr.offsets) << endl; + } + if (tr.target.ptr) { + // We only have a weak reference on the target object, so we must first try to + // safely acquire a strong reference before doing anything else with it. + if (reinterpret_cast( + tr.target.ptr)->attemptIncStrong(this)) { + error = reinterpret_cast(tr.cookie)->transact(tr.code, buffer, + &reply, tr.flags); + reinterpret_cast(tr.cookie)->decStrong(this); + } else { + error = UNKNOWN_TRANSACTION; + } - const pid_t origPid = mCallingPid; - const uid_t origUid = mCallingUid; - const int32_t origStrictModePolicy = mStrictModePolicy; - const int32_t origTransactionBinderFlags = mLastTransactionBinderFlags; + } else { + error = the_context_object->transact(tr.code, buffer, &reply, tr.flags); + } - mCallingPid = tr.sender_pid; - mCallingUid = tr.sender_euid; - mLastTransactionBinderFlags = tr.flags; + //ALOGI("<<<< TRANSACT from pid %d restore pid %d uid %d\n", + // mCallingPid, origPid, origUid); - //ALOGI(">>>> TRANSACT from pid %d uid %d\n", mCallingPid, mCallingUid); + if ((tr.flags & TF_ONE_WAY) == 0) { + LOG_ONEWAY("Sending reply to %d!", mCallingPid); + if (error < NO_ERROR) reply.setError(error); - Parcel reply; - status_t error; - IF_LOG_TRANSACTIONS() { - TextOutput::Bundle _b(alog); - alog << "BR_TRANSACTION thr " << (void*)pthread_self() - << " / obj " << tr.target.ptr << " / code " - << TypeCode(tr.code) << ": " << indent << buffer - << dedent << endl - << "Data addr = " - << reinterpret_cast(tr.data.ptr.buffer) - << ", offsets addr=" - << reinterpret_cast(tr.data.ptr.offsets) << endl; - } - if (tr.target.ptr) { - // We only have a weak reference on the target object, so we must first try to - // safely acquire a strong reference before doing anything else with it. - if (reinterpret_cast( - tr.target.ptr)->attemptIncStrong(this)) { - error = reinterpret_cast(tr.cookie)->transact(tr.code, buffer, - &reply, tr.flags); - reinterpret_cast(tr.cookie)->decStrong(this); + status_t statusBuffer; + result = writeTransactionData(BC_REPLY, 0, -1, 0, reply, &statusBuffer); } else { - error = UNKNOWN_TRANSACTION; + LOG_ONEWAY("NOT sending reply to %d!", mCallingPid); } - } else { - error = the_context_object->transact(tr.code, buffer, &reply, tr.flags); + mCallingPid = origPid; + mCallingUid = origUid; + mStrictModePolicy = origStrictModePolicy; + mLastTransactionBinderFlags = origTransactionBinderFlags; } - //ALOGI("<<<< TRANSACT from pid %d restore pid %d uid %d\n", - // mCallingPid, origPid, origUid); - - if ((tr.flags & TF_ONE_WAY) == 0) { - LOG_ONEWAY("Sending reply to %d!", mCallingPid); - if (error < NO_ERROR) reply.setError(error); - sendReply(reply, 0); - } else { - LOG_ONEWAY("NOT sending reply to %d!", mCallingPid); - } + if (result != NO_ERROR) + break; - mCallingPid = origPid; - mCallingUid = origUid; - mStrictModePolicy = origStrictModePolicy; - mLastTransactionBinderFlags = origTransactionBinderFlags; + if ((tr.flags & TF_ONE_WAY) == 0) + waitForResponse(NULL, NULL); IF_LOG_TRANSACTIONS() { TextOutput::Bundle _b(alog); diff --git a/libs/binder/include/binder/IPCThreadState.h b/libs/binder/include/binder/IPCThreadState.h index 245607e74e..9fb13bbe95 100644 --- a/libs/binder/include/binder/IPCThreadState.h +++ b/libs/binder/include/binder/IPCThreadState.h @@ -93,7 +93,6 @@ private: IPCThreadState(); ~IPCThreadState(); - status_t sendReply(const Parcel& reply, uint32_t flags); status_t waitForResponse(Parcel *reply, status_t *acquireResult=NULL); status_t talkWithDriver(bool doReceive=true); -- cgit v1.2.3