Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751886AbYKGGIN (ORCPT ); Fri, 7 Nov 2008 01:08:13 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751189AbYKGGH4 (ORCPT ); Fri, 7 Nov 2008 01:07:56 -0500 Received: from smtp1.linux-foundation.org ([140.211.169.13]:36378 "EHLO smtp1.linux-foundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751073AbYKGGHz (ORCPT ); Fri, 7 Nov 2008 01:07:55 -0500 Date: Thu, 6 Nov 2008 22:05:30 -0800 From: Andrew Morton To: Mathieu Desnoyers Cc: Linus Torvalds , Ingo Molnar , Peter Zijlstra , linux-kernel@vger.kernel.org, Nicolas Pitre , Ralf Baechle , benh@kernel.crashing.org, paulus@samba.org, David Miller , Ingo Molnar , Thomas Gleixner , Steven Rostedt , linux-arch@vger.kernel.org Subject: Re: [RFC patch 08/18] cnt32_to_63 should use smp_rmb() Message-Id: <20081106220530.5b0e3a96.akpm@linux-foundation.org> In-Reply-To: <20081107053349.861709786@polymtl.ca> References: <20081107052336.652868737@polymtl.ca> <20081107053349.861709786@polymtl.ca> X-Mailer: Sylpheed 2.4.8 (GTK+ 2.12.5; x86_64-redhat-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 Content-Length: 1267 Lines: 38 On Fri, 07 Nov 2008 00:23:44 -0500 Mathieu Desnoyers wrote: > #define cnt32_to_63(cnt_lo) \ > ({ \ > - static volatile u32 __m_cnt_hi; \ > + static u32 __m_cnt_hi; \ > union cnt32_to_63 __x; \ > __x.hi = __m_cnt_hi; \ > + smp_rmb(); /* read __m_cnt_hi before mmio cnt_lo */ \ > __x.lo = (cnt_lo); \ > if (unlikely((s32)(__x.hi ^ __x.lo) < 0)) \ > __m_cnt_hi = __x.hi = (__x.hi ^ 0x80000000) + (__x.hi >> 31); \ Oh dear. We have a macro which secretly maintains per-instantiation-site global state? And doesn't even implement locking to protect that state? I mean, the darned thing is called from sched_clock(), which can be concurrently called on separate CPUs and which can be called from interrupt context (with an arbitrary nesting level!) while it was running in process context. Who let that thing into Linux? Look: /* * Caller must provide locking to protect *caller_state */ u32 cnt32_to_63(u32 *caller_state, u32 cnt_lo); But even that looks pretty crappy. -- 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/