Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754764Ab3J0RMH (ORCPT ); Sun, 27 Oct 2013 13:12:07 -0400 Received: from mail-oa0-f73.google.com ([209.85.219.73]:38342 "EHLO mail-oa0-f73.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754278Ab3J0RMF (ORCPT ); Sun, 27 Oct 2013 13:12:05 -0400 From: Greg Thelen To: Tejun Heo Cc: Christoph Lameter , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , Johannes Weiner , Michal Hocko , Balbir Singh , KAMEZAWA Hiroyuki , handai.szj@taobao.com, Andrew Morton , x86@kernel.org, linux-kernel@vger.kernel.org, cgroups@vger.kernel.org, linux-mm@kvack.org Subject: Re: [PATCH 2/3] percpu counter: cast this_cpu_sub() adjustment References: <1382859876-28196-1-git-send-email-gthelen@google.com> <1382859876-28196-3-git-send-email-gthelen@google.com> Date: Sun, 27 Oct 2013 10:12:02 -0700 In-Reply-To: <1382859876-28196-3-git-send-email-gthelen@google.com> (Greg Thelen's message of "Sun, 27 Oct 2013 00:44:35 -0700") Message-ID: User-Agent: Gnus/5.130007 (Ma Gnus v0.7) Emacs/23.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4778 Lines: 133 On Sun, Oct 27 2013, Greg Thelen wrote: > this_cpu_sub() is implemented as negation and addition. > > This patch casts the adjustment to the counter type before negation to > sign extend the adjustment. This helps in cases where the counter > type is wider than an unsigned adjustment. An alternative to this > patch is to declare such operations unsupported, but it seemed useful > to avoid surprises. > > This patch specifically helps the following example: > unsigned int delta = 1 > preempt_disable() > this_cpu_write(long_counter, 0) > this_cpu_sub(long_counter, delta) > preempt_enable() > > Before this change long_counter on a 64 bit machine ends with value > 0xffffffff, rather than 0xffffffffffffffff. This is because > this_cpu_sub(pcp, delta) boils down to this_cpu_add(pcp, -delta), > which is basically: > long_counter = 0 + 0xffffffff > > Also apply the same cast to: > __this_cpu_sub() > this_cpu_sub_return() > and __this_cpu_sub_return() > > All percpu_test.ko passes, especially the following cases which > previously failed: > > l -= ui_one; > __this_cpu_sub(long_counter, ui_one); > CHECK(l, long_counter, -1); > > l -= ui_one; > this_cpu_sub(long_counter, ui_one); > CHECK(l, long_counter, -1); > CHECK(l, long_counter, 0xffffffffffffffff); > > ul -= ui_one; > __this_cpu_sub(ulong_counter, ui_one); > CHECK(ul, ulong_counter, -1); > CHECK(ul, ulong_counter, 0xffffffffffffffff); > > ul = this_cpu_sub_return(ulong_counter, ui_one); > CHECK(ul, ulong_counter, 2); > > ul = __this_cpu_sub_return(ulong_counter, ui_one); > CHECK(ul, ulong_counter, 1); > > Signed-off-by: Greg Thelen > --- > arch/x86/include/asm/percpu.h | 3 ++- > include/linux/percpu.h | 8 ++++---- > lib/percpu_test.c | 2 +- > 3 files changed, 7 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/include/asm/percpu.h b/arch/x86/include/asm/percpu.h > index 0da5200..b3e18f8 100644 > --- a/arch/x86/include/asm/percpu.h > +++ b/arch/x86/include/asm/percpu.h > @@ -128,7 +128,8 @@ do { \ > do { \ > typedef typeof(var) pao_T__; \ > const int pao_ID__ = (__builtin_constant_p(val) && \ > - ((val) == 1 || (val) == -1)) ? (val) : 0; \ > + ((val) == 1 || (val) == -1)) ? \ > + (int)(val) : 0; \ > if (0) { \ > pao_T__ pao_tmp__; \ > pao_tmp__ = (val); \ > diff --git a/include/linux/percpu.h b/include/linux/percpu.h > index cc88172..c74088a 100644 > --- a/include/linux/percpu.h > +++ b/include/linux/percpu.h > @@ -332,7 +332,7 @@ do { \ > #endif > > #ifndef this_cpu_sub > -# define this_cpu_sub(pcp, val) this_cpu_add((pcp), -(val)) > +# define this_cpu_sub(pcp, val) this_cpu_add((pcp), -(typeof(pcp))(val)) > #endif > > #ifndef this_cpu_inc > @@ -418,7 +418,7 @@ do { \ > # define this_cpu_add_return(pcp, val) __pcpu_size_call_return2(this_cpu_add_return_, pcp, val) > #endif > > -#define this_cpu_sub_return(pcp, val) this_cpu_add_return(pcp, -(val)) > +#define this_cpu_sub_return(pcp, val) this_cpu_add_return(pcp, -(typeof(pcp))(val)) > #define this_cpu_inc_return(pcp) this_cpu_add_return(pcp, 1) > #define this_cpu_dec_return(pcp) this_cpu_add_return(pcp, -1) > > @@ -586,7 +586,7 @@ do { \ > #endif > > #ifndef __this_cpu_sub > -# define __this_cpu_sub(pcp, val) __this_cpu_add((pcp), -(val)) > +# define __this_cpu_sub(pcp, val) __this_cpu_add((pcp), -(typeof(pcp))(val)) > #endif > > #ifndef __this_cpu_inc > @@ -668,7 +668,7 @@ do { \ > __pcpu_size_call_return2(__this_cpu_add_return_, pcp, val) > #endif > > -#define __this_cpu_sub_return(pcp, val) __this_cpu_add_return(pcp, -(val)) > +#define __this_cpu_sub_return(pcp, val) __this_cpu_add_return(pcp, -(typeof(pcp))(val)) > #define __this_cpu_inc_return(pcp) __this_cpu_add_return(pcp, 1) > #define __this_cpu_dec_return(pcp) __this_cpu_add_return(pcp, -1) > > diff --git a/lib/percpu_test.c b/lib/percpu_test.c > index 1ebeb44..8ab4231 100644 > --- a/lib/percpu_test.c > +++ b/lib/percpu_test.c > @@ -118,7 +118,7 @@ static int __init percpu_test_init(void) > CHECK(ul, ulong_counter, 2); > > ul = __this_cpu_sub_return(ulong_counter, ui_one); > - CHECK(ul, ulong_counter, 0); > + CHECK(ul, ulong_counter, 1); > > preempt_enable(); Oops. This update to percpu_test.c doesn't belong in this patch. It should be moved to the earlier patch 1/3. I'll repost the series with this update and the suggested changes to patch titles. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/