From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:57754 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1727060AbfAPAGM (ORCPT ); Tue, 15 Jan 2019 19:06:12 -0500 Received: from pps.filterd (m0098413.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id x0FNwuFK109904 for ; Tue, 15 Jan 2019 19:06:11 -0500 Received: from e11.ny.us.ibm.com (e11.ny.us.ibm.com [129.33.205.201]) by mx0b-001b2d01.pphosted.com with ESMTP id 2q1rm2b4ha-1 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=NOT) for ; Tue, 15 Jan 2019 19:06:10 -0500 Received: from localhost by e11.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 16 Jan 2019 00:06:10 -0000 Date: Tue, 15 Jan 2019 16:06:07 -0800 From: "Paul E. McKenney" Subject: Re: [PATCH 0/6] Simplify hash_resize.c Reply-To: paulmck@linux.ibm.com References: <20190114021024.GP1215@linux.ibm.com> <20190115013337.GA25108@linux.ibm.com> <20190115174304.GB1215@linux.ibm.com> <239642cb-8c5a-980f-5512-ea2b8cce09ed@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <239642cb-8c5a-980f-5512-ea2b8cce09ed@gmail.com> Message-Id: <20190116000607.GD1215@linux.ibm.com> Sender: perfbook-owner@vger.kernel.org List-ID: To: Akira Yokosawa Cc: perfbook@vger.kernel.org On Wed, Jan 16, 2019 at 07:15:15AM +0900, Akira Yokosawa wrote: > On 2019/01/15 09:43:04 -0800, Paul E. McKenney wrote: > > On Wed, Jan 16, 2019 at 12:32:52AM +0900, Akira Yokosawa wrote: > >> On 2019/01/14 17:33:37 -0800, Paul E. McKenney wrote: > >>> On Sun, Jan 13, 2019 at 06:10:24PM -0800, Paul E. McKenney wrote: > >>>> On Mon, Jan 14, 2019 at 08:28:27AM +0900, Akira Yokosawa wrote: > >>>>> >From 7b69a9b37ba9a73a50aad5cbb097235ddfe75870 Mon Sep 17 00:00:00 2001 > >>>>> From: Akira Yokosawa > >>>>> Date: Mon, 14 Jan 2019 07:25:14 +0900 > >>>>> Subject: [PATCH 0/6] Simplify hash_resize.c > >>>>> > >>>>> Hi Paul, > >>>>> > >>>>> This patch set updates hash_resize.c, which you suggested for me to > >>>>> take over, and the related text. > >>>>> > >>>>> I added a couple of Quick Quizzes as well, and in one of them, > >>>>> I included your version of hash_resize.c. > >>>>> > >>>>> Patch #1 updates hash_resize.c in the way I suggested. Note that > >>>>> in this update, "#ifndef FCV_SNIPPET" blocks are used to hide > >>>>> code for debugging (hash value checks) in code snippets. > >>>>> This code can actually be compiled with "-DFCV_SNIPPET" to see > >>>>> the performance without hash value checks. > >>>>> > >>>>> Patch #2 adds a couple of Quick Quizzes. Your version of hash_resize.c > >>>>> is added as hash_resize_s.c. > >>>>> > >>>>> Patch #3 removes unnecessary folding in a code snippet. > >>>>> > >>>>> Patch #4 adjusts a few sentence to the simpler approach. > >>>>> > >>>>> Patch #5 adds another Quick Quiz. > >>>>> > >>>>> Patch #6 adds an "#error" directive for the lack of rcu_barrier(). > >>>>> > >>>>> All of the updates in text would need your native eyes to be polished. > >>>> > >>>> Nice! Queued and pushed, thank you! I will review and send any > >>>> needed update by end of tomorrow, Pacific Time. > >>> > >>> Here is a summary of changes: > >>> > >>> o Move the ht_lock_state structure definition to follow the ht > >>> structure, matching the order of discussion in the text. > >>> Everything seems to build and run OK with this change. > >>> Also illustrates the advantages of line labels! ;-) > >>> > >>> o I considered inlining ht_search_bucket() into its only caller > >>> in hashtab_lookup(), but decided against it. More flexibility > >>> for change with it as is. > >>> > >>> o I added ht_search_bucket() to the in-text list of things > >>> permitting lookups and modifications to run concurrently with > >>> a resize operation. > >>> > >>> o I tweaked the description of hashtab_lock_mod(), hashtab_unlock_mod(), > >>> hashtab_add(), and hashtab_del(). > >>> > >>> o I tweaked the QQ asking about searches missing adds during resizes. > >>> > >>> o English is strange, so "Less Changes" must become "Fewer Changes". > >> > >> Sigh, I thought I knew this rule... Thanks for the fix. > >> > >>> > >>> o I put the long labels on a single line out of paranoia. > >>> > >>> I pushed these out on a working branch named paulmck.2019.01.14a. Please > >>> let me know whether I messed anything up, and once it looks good to you, > >>> I will pull these two commits into the master branch. > >> > >> There is an irrelevant clause added in an answer to a QQ. > >> See below for my suggestion. It doesn't sound fluent enough, though. > > > > Good catch! > > > > How does the following amended patch look? > > It looks perfect! Very good, thank you! I have merged it and pushed the result. Thanx, Paul > Thanks, Akira > > > > > Thanx, Paul > > > > ------------------------------------------------------------------------ > > > > commit 731c855ab11b9601a40324c6ef6b5ced6d7833e9 > > Author: Akira Yokosawa > > Date: Tue Jan 15 23:52:27 2019 +0900 > > > > datastruct/hash: Fix irrelevant clause in answer to quick quiz > > > > hashtab_lookup() is not affected by the reordering of ->hbp[] and > > ->hls_idx[]. Instead, mention the increase of cost at the earlier > > explanation of hashtab_lookup(). > > > > Signed-off-by: Akira Yokosawa > > [ paulmck: Wordsmithing. ] > > Signed-off-by: Paul E. McKenney > > > > diff --git a/datastruct/datastruct.tex b/datastruct/datastruct.tex > > index d1fde2c7535e..985021e927a8 100644 > > --- a/datastruct/datastruct.tex > > +++ b/datastruct/datastruct.tex > > @@ -1189,13 +1189,15 @@ a concurrent resize operation. > > either the old bucket if it is not resized yet, or to the new > > bucket if it has been resized, and \co{hashtab_del()} removes > > the specified element from any buckets into which it has been inserted. > > - \co{hashtab_lookup()} searches the new bucket if the search > > - of the old bucket fails. > > + The \co{hashtab_lookup()} function searches the new bucket > > + if the search of the old bucket fails, which has the disadvantage > > + of adding overhead to the lookup fastpath. > > The alternative \co{hashtab_lock_mod()} returns the locking > > state of the new bucket in \co{->hbp[0]} and \co{->hls_idx[0]} > > - if resize operation is in progress. > > - This reordering simplifies \co{hashtab_add()}, but at the expense > > - of an extra check in \co{hashtab_lookup()}. > > + if resize operation is in progress, instead of the perhaps > > + more natural choice of \co{->hbp[1]} and \co{->hls_idx[1]}. > > + However, this less-natural choice has the advantage of simplifying > > + \co{hashtab_add()}. > > > > Further analysis of the code is left as an exercise for the reader. > > } \QuickQuizEnd > > >