From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1195 invoked by alias); 8 Mar 2017 19:10:01 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 1167 invoked by uid 89); 8 Mar 2017 19:10:00 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS autolearn=ham version=3.3.2 spammy=contract X-HELO: sessmg23.ericsson.net Received: from sessmg23.ericsson.net (HELO sessmg23.ericsson.net) (193.180.251.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 08 Mar 2017 19:09:58 +0000 Received: from ESESSHC009.ericsson.se (Unknown_Domain [153.88.183.45]) by (Symantec Mail Security) with SMTP id 2C.F8.10506.40750C85; Wed, 8 Mar 2017 20:09:56 +0100 (CET) Received: from EUR01-VE1-obe.outbound.protection.outlook.com (153.88.183.145) by oa.msg.ericsson.com (153.88.183.45) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 8 Mar 2017 20:09:54 +0100 Authentication-Results: sourceware.org; dkim=none (message not signed) header.d=none;sourceware.org; dmarc=none action=none header.from=ericsson.com; Received: from [142.133.50.177] (192.75.88.130) by DB5PR07MB1717.eurprd07.prod.outlook.com (2603:10a6:0:12::22) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P384) id 15.1.961.8; Wed, 8 Mar 2017 19:09:52 +0000 Subject: Re: [PATCH 1/3] inf-ptrace: Do not stop memory transfers after a single word To: Andreas Arnez , References: <1488816060-20776-1-git-send-email-arnez@linux.vnet.ibm.com> <1488816060-20776-2-git-send-email-arnez@linux.vnet.ibm.com> From: Simon Marchi Message-ID: <06e5cf43-bb8b-6fa5-7201-414dc88388a3@ericsson.com> Date: Wed, 08 Mar 2017 19:10:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <1488816060-20776-2-git-send-email-arnez@linux.vnet.ibm.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-ClientProxiedBy: BN6PR03CA0062.namprd03.prod.outlook.com (2603:10b6:404:4c::24) To DB5PR07MB1717.eurprd07.prod.outlook.com (2603:10a6:0:12::22) X-MS-Office365-Filtering-Correlation-Id: c35ae170-587a-4a42-6db1-08d46656b3a7 X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:(22001);SRVR:DB5PR07MB1717; X-Microsoft-Exchange-Diagnostics: 1;DB5PR07MB1717;3:yw40tbIAHvhBMAqku0LhD8W9+vFw5yLNng2BD3U1f5lYlkKOMvN9Gui1VP4hBoitJfq+oH/bSXH1LTdRY1doW5vDca5TjDxXFK7f+qJSivnnxT0S4AwFX6DcePTBx+1Iy/WAmgPj/V53IOTb7RQWxd1gVEN2yoBvSRFBPH29KQnW0usOcHzwhKKH/MpzTJUuPwg0PBbwFeMDBJ7j8mRxoBBGmk4eJI2TllZrTEitpYBbEAwIWcSh9Oc2LsH8WFXhBoMKzobWKJ08+910AEdGRQ==;25:zx54pYIkiGzWaaclo5qJBrJP+soJAIvONCeydqgOvDCEnpyFSNjlP8ifyHFS3Me//9IYhDxPayydcCdp2i3KWs5cm8g+TPppKvOYNoSJt/GqYeD9wQrvz6zFkqJrbCdnLP3GYP3iI/FGpePNnAiI55a96t7CFZoC9kQvfS5l5C8hv/ORB2EBU7c9fGk5qJ2pf0pCbMkyp0gQcKsKU444H+HnOiovEtlRNWS9rKncVRP2crJ2Aab5bJMFYfbHdty5+U67AOcWmcN0oOsnjxcLRq6Lmv+W40CuB4VkMmMO+OeuGmCyJ69jYVHmW5numLGcUPruUTHDUHho5knFLeIjsBHuyqCM75qMNOR2GyS7f0tgcLLZPxLHesAIsnIT4OuOJY9JK73e3syS2dEf2Zwe18Du73bbSxnHIZ6hE64Qx/4K2yoRIxBvTq7dKT58inXZ5jtK/pIzmyCCdwzP9I6aEQ== X-Microsoft-Exchange-Diagnostics: 1;DB5PR07MB1717;31:qbRCTf+VwcECW1eLabLxqjznTrmOHVS8t016srPWezPDpR9nyr/vT2D7Xi52o9wf0X83mhZLAB3amAEGy7J/O6wH5J6hlWEFgkfIyjvmI/YET9iglse2pn2359v3ZmTto6XP5lZ5Dc7/UmaERpo6dagR+Za+pB4lJoisLZb4cyriIDxP0P+B/+HmnR3EKBYgPz23Rj1a/yLvJy+lF/86o/8jZ6lONP96AUcnieRXjWE4CSLEW/oVfSms1lbbg2A9;20:jb5KUdKETLNLp148jVTvWntBggEkB+z74UjPa8GORr0S6nRwLfk+wWBFtQDc9SeBP69rVM6ZrXPWhlaXkYnXcxv1C5zpP9KulbKC52BD0nFHk825a+UXVdyoul8w7WbdjC9DbEuYKiKsG2ENi1j3YEcp6ojiT7RjYRrkDDGYmHCeXDv/xbtSWaS/f9Yass3aAiIgZsvNfSzleh9W6lw3M0Bp4rCuj15I6iMTJZTCEkhazD/6w10OJxjZAw3cscd1MEIfXqZxG2US0cKVD12DSkIPctb2xqbMr9jBzNAnYEittBl/Chk4+HckNGkgG69rHHNBv1u+43UK0R07pwyWoA6y2MEtwNtn8wEUVNssEt56QVDysBQZa8UMEHtEW92u6vgSLMwZ02UlJFOObj9TAJHsg/PFcXj7ctsW+GZPtlsvEdMJa4SsiJ1pHwKxHekqDuBt/kDDrfFp9WPLXKktcNDV8CG4PsAVsyYWEFEScFCV9v/1SMRzLxl1pt+jZoN8 X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(6040375)(601004)(2401047)(5005006)(8121501046)(3002001)(10201501046)(6041248)(20161123560025)(20161123558025)(20161123555025)(20161123562025)(20161123564025)(6072148);SRVR:DB5PR07MB1717;BCL:0;PCL:0;RULEID:;SRVR:DB5PR07MB1717; X-Microsoft-Exchange-Diagnostics: 1;DB5PR07MB1717;4:K8KTenZ6Pehz644wsOJqovQZ6OtmEfYNHO1sWl4K5UGy6vV2dNS7NfCg+X/Nn7blbCWMMT5kSk96PPneaW6ah/vmk71QY0KO511hXSSN5BJXsT2yOUOwkWlZMdDf0QqdKm3YX6GfVVE3pZVbtvMJxs75ygbLHaOSTHUQL8a1OYvqDLPPQ19ueme4lF1katOJUAjcQ/VVI6FL4h77WZw/vqEEnTUjczkZz8oWYSRfnHY4vAjrYZtfKk9J8wgfUTwHCUkwLtglpRVnBYya3nf2ET621J9qKkXqhJFwh52nu+2RWHNdTPQESvOBDHegQ7P6D22v8+4rYLInapoQw/eMZpu0afZJ4qCXpwgQznmYqk6gg3UBoca5qQGY7UcBJUKXvUzcbs6ebhNT5iFQK1yYdzuYLpRw6MGkbeid4NxXpftfmGO2nV/mbGIjK4Lxo4jJ6GfO6P/4ymSvshUu20ymJaCQ0JUI96xBBV6Kgx5pc5OauLzVAoHMLHikD12eEEdHe6O++AD+VDhQCqv/mfev9DQyFRNZAXlBCo/mMVyS/ipLvfjebjQWKUPiA+Az/8o/PgZ08PVf9288vm89yJOkMcQQTuWy+1wRzi+hmv4joos= X-Forefront-PRVS: 02408926C4 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(4630300001)(6049001)(6009001)(39450400003)(377424004)(24454002)(377454003)(54534003)(6116002)(53546006)(38730400002)(6246003)(53936002)(230700001)(81166006)(8676002)(189998001)(76176999)(65826007)(3846002)(5660300001)(65806001)(229853002)(47776003)(50986999)(217423001)(6486002)(65956001)(66066001)(2950100002)(54356999)(6666003)(23746002)(305945005)(36756003)(2906002)(86362001)(31696002)(33646002)(83506001)(50466002)(7736002)(42186005)(31686004);DIR:OUT;SFP:1101;SCL:1;SRVR:DB5PR07MB1717;H:[142.133.50.177];FPR:;SPF:None;MLV:sfv;LANG:en; X-Microsoft-Exchange-Diagnostics: =?Windows-1252?Q?1;DB5PR07MB1717;23:0dUTDH0gnRSy0McGuiEE8DXWTpG6OrUXAQXlW?= =?Windows-1252?Q?pV5cEYu8S+e7/wXHmvkA//wvEo/WPyXw+/ldXOt4lRjXVzwxwZN2oEtv?= =?Windows-1252?Q?fUsyyNmY2ilwTJma/z2EXtltO0ZzMq6t0VVcHMO78/eqsJ2XuOxYZ0Ce?= =?Windows-1252?Q?Tjl+6nsDaO7nR3sEu3zVrX1UbLEf+GL7igsW/iQBZtAPJ3CEjl6w/uSI?= =?Windows-1252?Q?ZO+LTIlv/seM6CvyVOeiMXndNRjb6fcNWtghICvCvypkL5wLhfQo/ALL?= =?Windows-1252?Q?6v8Nm59AOI71dK9HwMSaqIGDXWZ7WiG9imWAo0cMKeXAaO3yfzlt2DLc?= =?Windows-1252?Q?Ef2zqyyN7WFNW64cWSsbORzoG/ndck+ew2pgwoqYRw2LwRcJbuPXVpqY?= =?Windows-1252?Q?wc8hosDCnAkgkNvvvzucDLs/PqPPwlzTJ0Xc73pfgVubSxJh//vlD44b?= =?Windows-1252?Q?MtPA+fkt6iX93lyip9jE2GW8/47nPrHc5CETRpl2eZ+GRthUByRYt88R?= =?Windows-1252?Q?QcMsTUjP1t6LFGh7jROtXU+q6Gz9j3h2a3GPn+9kZ2hLZGULwFtHRAKx?= =?Windows-1252?Q?Nsxf8xsyS+MbuqS9fZXuIdVgV1CvcLXCh/4c0LY9EVgIlMAvmrWdzXDA?= =?Windows-1252?Q?aL/qMiG0I4+5VxbtZKvWrWImYJ3d7GrMYWGQSlJfQ1zLJ3NYWeu3F2ma?= =?Windows-1252?Q?q7upo3XcICL6rAzaTVWrmmocdj0o1Jo0H074tNj2cJ2QrDIYIweji6Ql?= =?Windows-1252?Q?66eI06nN1pmfVKD69s55fhHx9IsezTFhNkuPaRO50OrS/dUNaXGGyF0N?= =?Windows-1252?Q?hzrdnvYixZhYFlR4p68Lh69Ic6vVkU/F+ECioHtZladSQkOmfMLcQi8r?= =?Windows-1252?Q?CGPhEuPpeEF5s/tv3mG+3jv95+L82JgcHfIW2koUmi7ekBgzh1Ca6mSg?= =?Windows-1252?Q?Sw7/Og6LUs5NPN/gXEdvqxdfj22ohyTBXKGrBDYOGzc/IW3Do9AKnFun?= =?Windows-1252?Q?oiYeeiYdsmOmI66nVmtrEXxX5BEfcUzV982UNvz66aPFqcGyaKMLxhrJ?= =?Windows-1252?Q?jQ9+/6xnj8nF2oXUSjw2rbVmSZtYVscJqITUEaq6xXhS7Sw6/J+xQvyw?= =?Windows-1252?Q?Ya44tEX00g4qPh+8FgRTeewnyThNVxuvctovjApcbvwKTPuZyzszoMjW?= =?Windows-1252?Q?nJmFeQ1VA=3D=3D?= X-Microsoft-Exchange-Diagnostics: 1;DB5PR07MB1717;6:JPN+gxa8GLSTmAT0t3B1lfh+k+m/AV7zsAyr3G1kWf13iBTefMF7mkLrJIfG2yAkC30zVh5ugeB7KpvOiTezoQNHYfUzGJUw4hCU02IRhWNogVZweNe6AstX18lqw4Z6hqkf5dfEdiEJU+OgWJFlds47PUqlU7W8g9GHqOf2XKnp5Do0fpf1WcZCYGDYM9nR81J5zd4+VuM1UinK1U3II2c3YfZk3nsbDbv9cCuFyIbTxVXj4x0dbYjni4N6D1hp543I81z87hZKQVHlvjgIhii21PhVareDytQvSg9xj9pgZNUmfRweNbC9Hj8UPzv1yalzcaPbEGMf0Gdhfhx75nJU7dx2MDzCjjV9boOfIt0hnDdeqRy+lXx+zkulm6gWCsAUGnbc1LiMU03moWcxqA==;5:FCC0dgQx0rQ9oMdt0XIjQzvRAQ950dEUV+1hhsIv9/VoPYub8g5tP2bcP4LYobkqgK9fERyfeflc3oS9DsH3t0FbzVCNbhb8kqKhbeX16UYkoWB3cFSIKYWA1P263AJaneCbunGQaccFfo56D4I6Zw==;24:7++EIwk7QUq+AtSRhnmVdHyXwi+AMVWQ32x0gBw2gHvhIDhll8moS91m4se1UE7Z1+8McfKMADibpbCNQLtObTt9KhONbylv/vpYpChW8jA= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1;DB5PR07MB1717;7:rkcolk+znS75jTSjA4tAI56fZEabtP6CBz2VoU6lzSdq/mJOPEdQ5CO181p3zn4HNCogT9B+o1RcgehxYcpR3UImYISpr1TviWoUWN+a2YumHUAmvb2mWtA4AjIlZy61PmUAFHLZ5z/AVejVqGQXT1N016kuqZJpjVfOU7ANSOtLi1a0izyURvzpN7s3lD29xoeNh8djWEazLdt3wCmiLus8cpHkivt5wJc/XuWtQX0uAVFUVd67Oh+fUMvv4LMm6jW81T+V/oBMuE1zfFj2XD/sIEHj7A3jAe2jSChXVxJzh2yZh9UprqSd18gIznwD7bUTkCReAJK54TZUV885Kw== X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Mar 2017 19:09:52.8553 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB5PR07MB1717 X-OriginatorOrg: ericsson.com X-IsSubscribed: yes X-SW-Source: 2017-03/txt/msg00111.txt.bz2 On 17-03-06 11:00 AM, Andreas Arnez wrote: > When inf_ptrace_xfer_partial performs a memory transfer via ptrace with > PT_READ_I, PT_WRITE_I (aka PTRACE_PEEKTEXT, PTRACE_POKETEXT), etc., then > it currently transfers at most one word. This behavior yields degraded > performance, particularly if the caller has significant preparation work > for each invocation. And indeed it has for writing, in > memory_xfer_partial in target.c, where all of the remaining data to be > transferred is copied to a temporary buffer each time, for breakpoint > shadow handling. Thus large writes have quadratic runtime and can take > hours. > > Note: On GNU/Linux targets GDB usually does not use > inf_ptrace_xfer_partial for large memory *reads* from the inferior, but > attempts a single read from /proc//mem instead. However, this is not > currently true for memory *writes*. So the problem occurs on GNU/Linux > when transferring large amounts of data *into* the inferior. > > This patch fixes the performance issue by attempting to fulfill the whole > transfer request in inf_ptrace_xfer_partial, using a loop around the > ptrace call. I think the idea is good. The xfer partial interface says that the target should transfer up to len bytes. Transferring 1 word at the time respects the contract, but we should try to be more efficient when possible. > gdb/ChangeLog: > > PR gdb/21220 > * inf-ptrace.c (inf_ptrace_xfer_partial): For memory transfers, > loop over ptrace peek/poke until end of buffer or error. > --- > gdb/inf-ptrace.c | 115 ++++++++++++++++++++++++------------------------------- > 1 file changed, 51 insertions(+), 64 deletions(-) > > diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c > index 21578742..2989259 100644 > --- a/gdb/inf-ptrace.c > +++ b/gdb/inf-ptrace.c > @@ -492,77 +492,64 @@ inf_ptrace_xfer_partial (struct target_ops *ops, enum target_object object, > } > #endif > { > - union > - { > - PTRACE_TYPE_RET word; > - gdb_byte byte[sizeof (PTRACE_TYPE_RET)]; > - } buffer; > - ULONGEST rounded_offset; > - ULONGEST partial_len; > - > - /* Round the start offset down to the next long word > - boundary. */ > - rounded_offset = offset & -(ULONGEST) sizeof (PTRACE_TYPE_RET); > - > - /* Since ptrace will transfer a single word starting at that > - rounded_offset the partial_len needs to be adjusted down to > - that (remember this function only does a single transfer). > - Should the required length be even less, adjust it down > - again. */ > - partial_len = (rounded_offset + sizeof (PTRACE_TYPE_RET)) - offset; > - if (partial_len > len) > - partial_len = len; > - > - if (writebuf) > + ULONGEST n; > + unsigned chunk; "unsigned" -> "unsigned int"? > + > + /* We transfer aligned words. Thus align OFFSET down to a word > + boundary and determine how many bytes to skip at the > + beginning. */ > + unsigned skip = offset & (sizeof (PTRACE_TYPE_RET) - 1); > + offset -= skip; > + > + for (n = 0; > + n < len; > + n += chunk, offset += sizeof (PTRACE_TYPE_RET), skip = 0) > { > - /* If OFFSET:PARTIAL_LEN is smaller than > - ROUNDED_OFFSET:WORDSIZE then a read/modify write will > - be needed. Read in the entire word. */ > - if (rounded_offset < offset > - || (offset + partial_len > - < rounded_offset + sizeof (PTRACE_TYPE_RET))) > - /* Need part of initial word -- fetch it. */ > - buffer.word = ptrace (PT_READ_I, pid, > - (PTRACE_TYPE_ARG3)(uintptr_t) > - rounded_offset, 0); > - > - /* Copy data to be written over corresponding part of > - buffer. */ > - memcpy (buffer.byte + (offset - rounded_offset), > - writebuf, partial_len); > - > - errno = 0; > - ptrace (PT_WRITE_D, pid, > - (PTRACE_TYPE_ARG3)(uintptr_t)rounded_offset, > - buffer.word); > - if (errno) > + /* Restrict to a chunk that fits in the current word. */ > + chunk = std::min (sizeof (PTRACE_TYPE_RET) - skip, len - n); > + > + /* Use a union for type punning. */ > + union > + { > + PTRACE_TYPE_RET word; > + gdb_byte byte[sizeof (PTRACE_TYPE_RET)]; > + } buf; > + > + /* Read the word, also when doing a partial word write. */ > + if (readbuf || chunk < sizeof (PTRACE_TYPE_RET)) Use != NULL or == NULL when checking pointers. > { > - /* Using the appropriate one (I or D) is necessary for > - Gould NP1, at least. */ > errno = 0; > - ptrace (PT_WRITE_I, pid, > - (PTRACE_TYPE_ARG3)(uintptr_t)rounded_offset, > - buffer.word); > + buf.word = ptrace (PT_READ_I, pid, > + (PTRACE_TYPE_ARG3)(uintptr_t) offset, > + 0); > if (errno) > - return TARGET_XFER_EOF; > + break; > + if (readbuf) > + memcpy (readbuf + n, buf.byte + skip, chunk); > + } > + if (writebuf) > + { > + memcpy (buf.byte + skip, writebuf + n, chunk); > + errno = 0; > + ptrace (PT_WRITE_D, pid, > + (PTRACE_TYPE_ARG3)(uintptr_t) offset, > + buf.word); > + if (errno) > + { > + /* Using the appropriate one (I or D) is necessary for > + Gould NP1, at least. */ > + errno = 0; > + ptrace (PT_WRITE_I, pid, > + (PTRACE_TYPE_ARG3)(uintptr_t) offset, > + buf.word); > + if (errno) > + break; > + } > } > } > > - if (readbuf) > - { > - errno = 0; > - buffer.word = ptrace (PT_READ_I, pid, > - (PTRACE_TYPE_ARG3)(uintptr_t)rounded_offset, > - 0); > - if (errno) > - return TARGET_XFER_EOF; > - /* Copy appropriate bytes out of the buffer. */ > - memcpy (readbuf, buffer.byte + (offset - rounded_offset), > - partial_len); > - } > - > - *xfered_len = partial_len; > - return TARGET_XFER_OK; > + *xfered_len = n; > + return n ? TARGET_XFER_OK : TARGET_XFER_EOF; This is not a comment specifically about your patch, since that's how it was already, but maybe it would be a good time to address this. I understand there's some level of overlap between the read and write (like the offset/skip computation), but I don't think that handling reading and writing in the same loop is very readable. It just adds a bunch of branches and makes it hard to follow. If that code was split in two functions (one for read, one for write), it would be way more straightforward. Simon