public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
To: Christoph Lameter <clameter@sgi.com>
Cc: Pekka Enberg <penberg@cs.helsinki.fi>,
	akpm@linux-foundation.org, linux-kernel@vger.kernel.org,
	mingo@redhat.com
Subject: Re: [PATCH] SLUB: use have_arch_cmpxchg()
Date: Mon, 27 Aug 2007 16:25:02 -0400	[thread overview]
Message-ID: <20070827202501.GC5653@Krystal> (raw)
In-Reply-To: <Pine.LNX.4.64.0708271240350.5056@schroedinger.engr.sgi.com>

* Christoph Lameter (clameter@sgi.com) wrote:
> On Mon, 27 Aug 2007, Mathieu Desnoyers wrote:
> 
> > Hrm, actually, I don't think such have_arch_cmpxchg() macro will be
> > required at all because of the small performance hit disabling
> > preemption will have on the slow and fast paths. Let's compare, for each
> > of the slow path and fast path, what locking looks like on architecture
> > with and without local cmpxchg:
> > 
> > What we actually do here is:
> > 
> > fast path:
> > with local_cmpxchg:
> >   preempt_disable()
> >   preempt_enable()
> > without local_cmpxchg:
> >   preempt_disable()
> >   local_irq_save
> >   local_irq_restore
> >   preempt_enable()
> >     (we therefore disable preemption _and_ disable interrupts for
> >     nothing)
> 
> Hmmm..... This is a performance issue for preempt kernels.
> 
> > slow path:
> > both with and without local_cmpxchg():
> >   preempt_disable()
> >   preempt_enable()
> 
> Here we potentially loose our per cpu structure since the process may be 
> rescheduled.
> 

Yes, what you do here is more precisely:

  preempt_disable()
  local_irq_save()
  preempt_enable_no_resched()
  ...
  local_irq_restore()
  preempt_check_resched()


> >   local_irq_save()
> >   local_irq_restore()
> > 
> > 
> > Therefore, we would add preempt disable/enable to the fast path of
> > architectures where local_cmpxchg is emulated with irqs off. But since
> > preempt disable/enable is just a check counter increment/decrement with
> > barrier() and thread flag check, I doubt it would hurt performances
> > enough to justify the added complexity of disabling interrupts for the
> > whole fast path in this case.
> 
> One additional cacheline referenced in the hot path. Ok the counter may be 
> hot as well if this is a preemptible kernel. Nevertheless a cause for 
> concern.
> 

If kernel is non preemptible, the macros preempt_*() are defined to
nothing. And as you say, on preemptible kernels, these variables (thread
flags and preempt count) are very likely to be in cache, since they are
in the thread info struct, but it should still be kept in mind.

Mathieu

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

  reply	other threads:[~2007-08-27 20:31 UTC|newest]

Thread overview: 115+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-08-20 20:15 [patch 00/23] cmpxchg_local and cmpxchg64_local implementation Mathieu Desnoyers
2007-08-20 20:15 ` [patch 01/23] Fall back on interrupt disable in cmpxchg8b on 80386 and 80486 Mathieu Desnoyers
2007-08-20 20:32   ` Christoph Lameter
2007-08-20 20:41     ` Mathieu Desnoyers
2007-08-20 20:46       ` Christoph Lameter
2007-08-20 21:29         ` Mathieu Desnoyers
2007-08-20 21:49           ` Christoph Lameter
2007-08-20 21:54             ` Mathieu Desnoyers
2007-08-20 22:07               ` Christoph Lameter
2007-08-20 22:29                 ` Mathieu Desnoyers
2007-08-21 17:38                   ` [PATCH] SLUB Use cmpxchg() everywhere Mathieu Desnoyers
2007-08-21 17:38                 ` [PATCH] SLUB use cmpxchg_local Mathieu Desnoyers
2007-08-21 17:44                   ` Mathieu Desnoyers
2007-08-21 21:10                     ` Christoph Lameter
2007-08-21 23:21                     ` Mathieu Desnoyers
2007-08-21 23:35                       ` Christoph Lameter
2007-08-21 23:38                       ` Christoph Lameter
2007-08-21 20:41                   ` Mathieu Desnoyers
2007-08-21 21:36                     ` Christoph Lameter
2007-08-21 21:08                   ` Christoph Lameter
2007-08-21 23:12                     ` Mathieu Desnoyers
2007-08-21 23:17                       ` Christoph Lameter
2007-08-21 23:39                         ` Mathieu Desnoyers
2007-08-21 23:41                           ` Christoph Lameter
2007-08-21 23:47                             ` Mathieu Desnoyers
2007-08-21 23:51                               ` Christoph Lameter
2007-08-22  0:03                                 ` Mathieu Desnoyers
2007-08-22  0:11                                   ` Christoph Lameter
2007-08-22  0:26                                     ` Mathieu Desnoyers
2007-08-22  0:34                                       ` Christoph Lameter
2007-08-22  1:18                                         ` Mathieu Desnoyers
2007-08-22 15:00                                         ` [PATCH] define have_arch_cmpxchg() Mathieu Desnoyers
2007-08-22 18:50                                           ` Christoph Lameter
2007-08-22 15:02                                         ` [PATCH] SLUB: use have_arch_cmpxchg() Mathieu Desnoyers
2007-08-22 16:24                                           ` Pekka Enberg
2007-08-27 14:56                                             ` Mathieu Desnoyers
2007-08-27 19:43                                               ` Christoph Lameter
2007-08-27 20:25                                                 ` Mathieu Desnoyers [this message]
2007-08-22  1:28                                   ` [PATCH] SLUB use cmpxchg_local Andi Kleen
2007-08-22  0:38                                     ` Mathieu Desnoyers
2007-08-22  1:06                                       ` Christoph Lameter
2007-08-22  1:12                                         ` Mathieu Desnoyers
2007-08-22  9:39                                         ` Andi Kleen
2007-08-22 13:45                                         ` Mathieu Desnoyers
2007-08-22 13:46                                           ` Andi Kleen
2007-08-22 18:54                                             ` Christoph Lameter
2007-08-22 19:25                                           ` Christoph Lameter
2007-08-22 20:09                                             ` Mathieu Desnoyers
2007-08-22 20:19                                               ` Christoph Lameter
2007-08-22 20:29                                                 ` Mathieu Desnoyers
2007-08-22 20:33                                                   ` Christoph Lameter
2007-08-22 20:38                                                   ` Christoph Lameter
2007-08-21 23:14                   ` Christoph Lameter
2007-08-21 23:23                     ` Mathieu Desnoyers
2007-08-21 23:50                       ` Mathieu Desnoyers
2007-08-27  6:52                     ` Peter Zijlstra
2007-08-27 19:39                       ` Christoph Lameter
2007-08-27 20:22                         ` Mathieu Desnoyers
2007-08-27 20:26                           ` Christoph Lameter
2007-08-27 20:39                             ` Mathieu Desnoyers
2007-08-27 21:04                               ` Christoph Lameter
2007-08-27 21:10                                 ` Mathieu Desnoyers
2007-08-27 21:23                                   ` Christoph Lameter
2007-08-27 21:38                                     ` Mathieu Desnoyers
2007-08-27 22:12                                       ` Christoph Lameter
2007-08-27 22:27                                         ` Mathieu Desnoyers
2007-08-27 22:29                                           ` Christoph Lameter
2007-08-28  1:26                                           ` Christoph Lameter
2007-08-28 12:07                                             ` Mathieu Desnoyers
2007-08-28 19:42                                               ` Christoph Lameter
2007-09-04 20:02                                             ` Mathieu Desnoyers
2007-09-04 20:03                                             ` [PATCH] local_t protection (critical section) Mathieu Desnoyers
2007-09-04 20:04                                             ` [PATCH] slub - Use local_t protection Mathieu Desnoyers
2007-09-04 20:45                                               ` Christoph Lameter
2007-09-05 13:03                                                 ` Mathieu Desnoyers
2007-09-05 13:04                                                 ` [PATCH] local_t protection (critical section) Mathieu Desnoyers
2007-09-12 22:33                                                   ` Christoph Lameter
2007-09-12 23:00                                                     ` Mathieu Desnoyers
2007-09-05 13:06                                                 ` [PATCH] slub - Use local_t protection Mathieu Desnoyers
2007-09-12 22:28                                                   ` Christoph Lameter
2007-08-27 22:15                         ` [PATCH] SLUB use cmpxchg_local Christoph Lameter
2007-08-28  7:12                           ` Peter Zijlstra
2007-08-28 19:36                             ` Christoph Lameter
2007-08-28 19:46                               ` Peter Zijlstra
2007-08-20 20:15 ` [patch 02/23] Add cmpxchg_local to asm-generic for per cpu atomic operations Mathieu Desnoyers
2007-08-20 20:15 ` [patch 03/23] Add cmpxchg_local to arm Mathieu Desnoyers
2007-08-20 20:15 ` [patch 04/23] Add cmpxchg_local to avr32 Mathieu Desnoyers
2007-08-20 20:15 ` [patch 05/23] Add cmpxchg_local to blackfin, replace __cmpxchg by generic cmpxchg Mathieu Desnoyers
2007-08-20 20:15 ` [patch 06/23] Add cmpxchg_local to cris Mathieu Desnoyers
2007-08-20 20:15 ` [patch 07/23] Add cmpxchg_local to frv Mathieu Desnoyers
2007-08-20 20:15 ` [patch 08/23] Add cmpxchg_local to h8300 Mathieu Desnoyers
2007-08-20 20:15 ` [patch 09/23] Add cmpxchg_local, cmpxchg64 and cmpxchg64_local to ia64 Mathieu Desnoyers
2007-08-20 20:15 ` [patch 10/23] New cmpxchg_local (optimized for UP case) for m32r Mathieu Desnoyers
2007-08-21  9:36   ` Hirokazu Takata
2007-08-20 20:15 ` [patch 11/23] Fix m32r __xchg Mathieu Desnoyers
2007-08-21  9:39   ` Hirokazu Takata
2007-08-20 20:15 ` [patch 12/23] local_t m32r use architecture specific cmpxchg_local Mathieu Desnoyers
2007-08-21  9:34   ` Hirokazu Takata
2007-08-21 14:01     ` Mathieu Desnoyers
2007-08-20 20:15 ` [patch 13/23] Add cmpxchg_local to m86k Mathieu Desnoyers
2007-08-20 20:15 ` [patch 14/23] Add cmpxchg_local to m68knommu Mathieu Desnoyers
2007-08-20 20:15 ` [patch 15/23] Add cmpxchg_local to parisc Mathieu Desnoyers
2007-08-20 20:15 ` [patch 16/23] Add cmpxchg_local to ppc Mathieu Desnoyers
2007-08-20 20:15 ` [patch 17/23] Add cmpxchg_local to s390 Mathieu Desnoyers
2007-08-20 20:15 ` [patch 18/23] Add cmpxchg_local to sh, use generic cmpxchg() instead of cmpxchg_u32 Mathieu Desnoyers
2007-08-20 20:15 ` [patch 19/23] Add cmpxchg_local to sh64 Mathieu Desnoyers
2007-08-20 20:15 ` [patch 20/23] Add cmpxchg_local to sparc, move __cmpxchg to system.h Mathieu Desnoyers
2007-08-20 20:15 ` [patch 21/23] Add cmpxchg_local to sparc64 Mathieu Desnoyers
2007-08-20 23:34   ` Julian Calaby
2007-08-20 23:36     ` Christoph Lameter
2007-08-20 23:42       ` Julian Calaby
2007-08-20 23:43     ` [patch 21/23] Add cmpxchg_local to sparc64 (update) Mathieu Desnoyers
2007-08-20 20:15 ` [patch 22/23] Add cmpxchg_local to v850 Mathieu Desnoyers
2007-08-20 20:15 ` [patch 23/23] Add cmpxchg_local to xtensa Mathieu Desnoyers
2007-08-20 20:29 ` [patch 00/23] cmpxchg_local and cmpxchg64_local implementation Mathieu Desnoyers

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20070827202501.GC5653@Krystal \
    --to=mathieu.desnoyers@polymtl.ca \
    --cc=akpm@linux-foundation.org \
    --cc=clameter@sgi.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=penberg@cs.helsinki.fi \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox