From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1762780AbYDVPh7 (ORCPT ); Tue, 22 Apr 2008 11:37:59 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1757368AbYDVPht (ORCPT ); Tue, 22 Apr 2008 11:37:49 -0400 Received: from smtp28.orange.fr ([80.12.242.99]:54527 "EHLO smtp28.orange.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753785AbYDVPhs convert rfc822-to-8bit (ORCPT ); Tue, 22 Apr 2008 11:37:48 -0400 X-ME-UUID: 20080422153745890.D96E7800004D@mwinf2803.orange.fr Message-ID: <480E0642.6080109@cosmosbay.com> Date: Tue, 22 Apr 2008 17:37:38 +0200 From: Eric Dumazet User-Agent: Thunderbird 1.5.0.14 (Windows/20071210) MIME-Version: 1.0 To: Andrea Arcangeli Cc: Christoph Lameter , Nick Piggin , Jack Steiner , Peter Zijlstra , kvm-devel@lists.sourceforge.net, Kanoj Sarcar , Roland Dreier , Steve Wise , linux-kernel@vger.kernel.org, Avi Kivity , linux-mm@kvack.org, Robin Holt , general@lists.openfabrics.org, Hugh Dickins , akpm@linux-foundation.org, Rusty Russell Subject: Re: [PATCH 01 of 12] Core of mmu notifiers References: <480DFC8A.8040105@cosmosbay.com> <20080422151529.GE24536@duo.random> In-Reply-To: <20080422151529.GE24536@duo.random> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Andrea Arcangeli a écrit : > On Tue, Apr 22, 2008 at 04:56:10PM +0200, Eric Dumazet wrote: > >> Andrea Arcangeli a écrit : >> >>> + >>> +static int mm_lock_cmp(const void *a, const void *b) >>> +{ >>> + cond_resched(); >>> + if ((unsigned long)*(spinlock_t **)a < >>> + (unsigned long)*(spinlock_t **)b) >>> + return -1; >>> + else if (a == b) >>> + return 0; >>> + else >>> + return 1; >>> +} >>> + >>> >> This compare function looks unusual... >> It should work, but sort() could be faster if the >> if (a == b) test had a chance to be true eventually... >> > > Hmm, are you saying my mm_lock_cmp won't return 0 if a==b? > I am saying your intent was probably to test else if ((unsigned long)*(spinlock_t **)a == (unsigned long)*(spinlock_t **)b) return 0; Because a and b are pointers to the data you want to compare. You need to dereference them. >> static int mm_lock_cmp(const void *a, const void *b) >> { >> unsigned long la = (unsigned long)*(spinlock_t **)a; >> unsigned long lb = (unsigned long)*(spinlock_t **)b; >> >> cond_resched(); >> if (la < lb) >> return -1; >> if (la > lb) >> return 1; >> return 0; >> } >> > > If your intent is to use the assumption that there are going to be few > equal entries, you should have used likely(la > lb) to signal it's > rarely going to return zero or gcc is likely free to do whatever it > wants with the above. Overall that function is such a slow path that > this is going to be lost in the noise. My suggestion would be to defer > microoptimizations like this after 1/12 will be applied to mainline. > > Thanks! > > Hum, it's not a micro-optimization, but a bug fix. :) Sorry if it was not clear From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: <480E0642.6080109@cosmosbay.com> Date: Tue, 22 Apr 2008 17:37:38 +0200 From: Eric Dumazet MIME-Version: 1.0 Subject: Re: [PATCH 01 of 12] Core of mmu notifiers References: <480DFC8A.8040105@cosmosbay.com> <20080422151529.GE24536@duo.random> In-Reply-To: <20080422151529.GE24536@duo.random> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 8BIT Sender: owner-linux-mm@kvack.org Return-Path: To: Andrea Arcangeli Cc: Christoph Lameter , Nick Piggin , Jack Steiner , Peter Zijlstra , kvm-devel@lists.sourceforge.net, Kanoj Sarcar , Roland Dreier , Steve Wise , linux-kernel@vger.kernel.org, Avi Kivity , linux-mm@kvack.org, Robin Holt , general@lists.openfabrics.org, Hugh Dickins , akpm@linux-foundation.org, Rusty Russell List-ID: Andrea Arcangeli a ecrit : > On Tue, Apr 22, 2008 at 04:56:10PM +0200, Eric Dumazet wrote: > >> Andrea Arcangeli a ecrit : >> >>> + >>> +static int mm_lock_cmp(const void *a, const void *b) >>> +{ >>> + cond_resched(); >>> + if ((unsigned long)*(spinlock_t **)a < >>> + (unsigned long)*(spinlock_t **)b) >>> + return -1; >>> + else if (a == b) >>> + return 0; >>> + else >>> + return 1; >>> +} >>> + >>> >> This compare function looks unusual... >> It should work, but sort() could be faster if the >> if (a == b) test had a chance to be true eventually... >> > > Hmm, are you saying my mm_lock_cmp won't return 0 if a==b? > I am saying your intent was probably to test else if ((unsigned long)*(spinlock_t **)a == (unsigned long)*(spinlock_t **)b) return 0; Because a and b are pointers to the data you want to compare. You need to dereference them. >> static int mm_lock_cmp(const void *a, const void *b) >> { >> unsigned long la = (unsigned long)*(spinlock_t **)a; >> unsigned long lb = (unsigned long)*(spinlock_t **)b; >> >> cond_resched(); >> if (la < lb) >> return -1; >> if (la > lb) >> return 1; >> return 0; >> } >> > > If your intent is to use the assumption that there are going to be few > equal entries, you should have used likely(la > lb) to signal it's > rarely going to return zero or gcc is likely free to do whatever it > wants with the above. Overall that function is such a slow path that > this is going to be lost in the noise. My suggestion would be to defer > microoptimizations like this after 1/12 will be applied to mainline. > > Thanks! > > Hum, it's not a micro-optimization, but a bug fix. :) Sorry if it was not clear -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org