From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758664AbdELU7B (ORCPT ); Fri, 12 May 2017 16:59:01 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:34096 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758581AbdELU67 (ORCPT ); Fri, 12 May 2017 16:58:59 -0400 Date: Fri, 12 May 2017 13:58:58 -0700 From: Andrew Morton To: Leno Hou Cc: linux-kernel@vger.kernel.org, andy.shevchenko@gmail.com, hch@infradead.org Subject: Re: [PATCH v2] lib/btree.c: optimise the code by previously getpos function Message-Id: <20170512135858.701ce87f51c7fa255db7d642@linux-foundation.org> In-Reply-To: <1494495741-30760-1-git-send-email-lenohou@gmail.com> References: <1494495741-30760-1-git-send-email-lenohou@gmail.com> X-Mailer: Sylpheed 3.4.1 (GTK+ 2.24.23; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 11 May 2017 17:42:21 +0800 Leno Hou wrote: > This patch optimized the code by previously getpos function call. > Therefore, It's takes the convenience to understand logic of code. I would rewrite this changelog to read : Rework the getpos() helper function and use it to remove various : open-coded implemetnations of its funtionality. > ... > > @@ -466,7 +458,7 @@ static int btree_insert_level(struct btree_head *head, struct btree_geo *geo, > /* two identical keys are not allowed */ > BUG_ON(pos < fill && keycmp(geo, node, pos, key) == 0); > > - if (fill == geo->no_pairs) { > + if (fill < 0) { > /* need to split node */ > unsigned long *new; The code here is a bit awkward. : retry: : node = find_level(head, geo, key, level); : pos = getpos(geo, node, key); : fill = getfill(geo, node, pos); : /* two identical keys are not allowed */ : BUG_ON(pos < fill && keycmp(geo, node, pos, key) == 0); : : if (fill < 0) { If getpos() returns -ENOENT then we're passing -ENOENT into getfill(). That might happen to work OK (or it might go BUG) but it's ugly and unobvious. There's a similar issue in rebalance() and in btree_remove_level(): failed to update existing getpos() callers for the new getpos() return value semantics.