From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13945 invoked by alias); 17 Nov 2013 21:44:17 -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 13930 invoked by uid 89); 17 Nov 2013 21:44:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=AWL,BAYES_50,RDNS_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no version=3.3.2 X-HELO: mail-vb0-f53.google.com Received: from Unknown (HELO mail-vb0-f53.google.com) (209.85.212.53) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Sun, 17 Nov 2013 21:44:15 +0000 Received: by mail-vb0-f53.google.com with SMTP id x16so867616vbf.40 for ; Sun, 17 Nov 2013 13:44:07 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=3lZl319beNJcvcPojxhY3eXR9m8DFS2pPvw39mNaDSU=; b=OEdFIYsHw4wKyRwXyKBjuOP3moQmMvF7EqmyWnaGLXv5fZEh0ZF3KQgf8don4GcNJx CLJDZVHjQ0SKsRAw/FM9BaM3vVr3pSaroxag2d/WwLMeNb8ne2MKjtrDU+eFwd3J9K82 cQir+ZjgqT5W3s1U1llh1wuIHrlMUry0emBokXlTOdmAPv6mcvvqRAiE/j/VO28Ae1Vq yf7GIHpKSNGZCPypf6k+8CNDjC0M0299fJYYVWc1GpVRwTG0nlv4Knp9lNoRgVb7miGc wCjLYmzhBrveKp96LNOW/TGg7fOgWSoBXrwEEk/v015mVqtCYTEmV6nC0M0zWFHzmvuW 2O2A== X-Gm-Message-State: ALoCoQlvsmwCwbrmBVctRFiTEeqSL01V1F0QraGDat+xrgve7r0VMKsiy9pqHkV3klNoRUo3XE6SADU61Kby0ulSY4C9P0+HtFZkcRkgzqM/zsMUrpQaXyD75uQSmKG6L67jHSl14fhOITEC3C7UVYUXtQxJBXnQ3CnvgoKW3YqGUdmZML0y9BNmPsXCoCUBbleyAN4K4YYYg5lZMg3a3ZAhnkYqCoevdA== MIME-Version: 1.0 X-Received: by 10.220.86.69 with SMTP id r5mr12099491vcl.9.1384724647203; Sun, 17 Nov 2013 13:44:07 -0800 (PST) Received: by 10.52.163.52 with HTTP; Sun, 17 Nov 2013 13:44:07 -0800 (PST) In-Reply-To: <1383458049-20893-9-git-send-email-yao@codesourcery.com> References: <1383458049-20893-1-git-send-email-yao@codesourcery.com> <1383458049-20893-9-git-send-email-yao@codesourcery.com> Date: Sun, 17 Nov 2013 22:02:00 -0000 Message-ID: Subject: Re: [PATCH 08/10] Don't invalidate dcache when option stack-cache is changed From: Doug Evans To: Yao Qi Cc: gdb-patches Content-Type: text/plain; charset=ISO-8859-1 X-IsSubscribed: yes X-SW-Source: 2013-11/txt/msg00453.txt.bz2 On Sat, Nov 2, 2013 at 10:54 PM, Yao Qi wrote: > Nowadays, option "stack-cache" on->off and off->on transitions trigger > cache invalidation. It is not very necessary and not friendly to > using target_dcache for other purpose, such as code caching. > > Option "stack-cache" can be regarded as a switch, to decide whether to > read through cache when reading data from target. When this switch is > off, read from target. When the switch is on, read from cache. We > need to keep cache in sync, so when GDB writes data, cache should be > updated unconditionally. > > gdb: > > 2013-11-02 Yao Qi > > * target-dcache.c (stack_cache_enabled_p_1): Remove. > (set_stack_cache_enabled_p): Remove. > (_initialize_target_dcache): Update command register. > * target.c (memory_xfer_partial_1): Don't check > stack_cache_enabled. > --- > gdb/target-dcache.c | 24 ++++-------------------- > gdb/target.c | 4 ++-- > 2 files changed, 6 insertions(+), 22 deletions(-) > > diff --git a/gdb/target-dcache.c b/gdb/target-dcache.c > index a840c40..ce36bbb 100644 > --- a/gdb/target-dcache.c > +++ b/gdb/target-dcache.c > @@ -100,26 +100,10 @@ target_dcache_get_or_init (void) > } > > /* The option sets this. */ > -static int stack_cache_enabled_p_1 = 1; > -/* And set_stack_cache_enabled_p updates this. > - The reason for the separation is so that we don't flush the cache for > - on->on transitions. */ > -static int stack_cache_enabled_p = 1; > - > -/* This is called *after* the stack-cache has been set. > - Flush the cache for off->on and on->off transitions. > - There's no real need to flush the cache for on->off transitions, > - except cleanliness. */ > > -static void > -set_stack_cache_enabled_p (char *args, int from_tty, > - struct cmd_list_element *c) > -{ > - if (stack_cache_enabled_p != stack_cache_enabled_p_1) > - target_dcache_invalidate (); > +static int stack_cache_enabled_p = 1; > > - stack_cache_enabled_p = stack_cache_enabled_p_1; > -} > +/* Show option "stack-cache". */ > > static void > show_stack_cache_enabled_p (struct ui_file *file, int from_tty, > @@ -143,13 +127,13 @@ void > _initialize_target_dcache (void) > { > add_setshow_boolean_cmd ("stack-cache", class_support, > - &stack_cache_enabled_p_1, _("\ > + &stack_cache_enabled_p, _("\ > Set cache use for stack access."), _("\ > Show cache use for stack access."), _("\ > When on, use the data cache for all stack access, regardless of any\n\ > configured memory regions. This improves remote performance significantly.\n\ > By default, caching for stack access is on."), > - set_stack_cache_enabled_p, > + NULL, > show_stack_cache_enabled_p, > &setlist, &showlist); > I'm still not comfortable with this. This is optimizing a rare occurrence. Users aren't expected to be turning this on and off during a session. If one wanted to remove the cache invalidation for the off->on transition that seems reasonable, but if I do a backtrace, turn the caching optimization off, and then do another backtrace, I'd want the second one to not use the cache. YMMV. OTOH, if there was a command I could use to flush the cache after turning off the stack cache optimization, then I could see leaving the cache alone after the on->off transition. [I still think it'd be preferable to invalidate the cache, but I can live with asking users to live with having to manually flush the cache after turning off the optimization.] > diff --git a/gdb/target.c b/gdb/target.c > index 1212347..55c5d78 100644 > --- a/gdb/target.c > +++ b/gdb/target.c > @@ -1593,14 +1593,14 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object, > > /* Make sure the cache gets updated no matter what - if we are writing > to the stack. Even if this write is not tagged as such, we still need > - to update the cache. */ > + to update the cache. Update the cache to keep it in sync if it > + has been initialized. */ > > if (res > 0 > && inf != NULL > && writebuf != NULL > && target_dcache_init_p () > && !region->attrib.cache > - && stack_cache_enabled () > && object != TARGET_OBJECT_STACK_MEMORY) > { > DCACHE *dcache = target_dcache_get (); This part of the patch seems ok though.