From owner-freebsd-arch@FreeBSD.ORG Sat Nov 11 13:32:24 2006 Return-Path: X-Original-To: freebsd-arch@freebsd.org Delivered-To: freebsd-arch@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id CF08916A49E for ; Sat, 11 Nov 2006 13:32:24 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from wx-out-0506.google.com (wx-out-0506.google.com [66.249.82.227]) by mx1.FreeBSD.org (Postfix) with ESMTP id 3B5B443D5F for ; Sat, 11 Nov 2006 13:32:21 +0000 (GMT) (envelope-from asmrookie@gmail.com) Received: by wx-out-0506.google.com with SMTP id s18so669571wxc for ; Sat, 11 Nov 2006 05:32:21 -0800 (PST) DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=beta; d=gmail.com; h=received:message-id:date:from:sender:to:subject:cc:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references:x-google-sender-auth; b=oofqBI+3y9NHLfK19KLqi63BkwzrYrpLg5c2+Y2rsOnj1h1WlSEkJ++k24IRqTnUq576jv2nyDKSxg0q+zKlsnGsDG87dcK9UJMMQdhOfliJw5QSnvjkZzrNMRTQqSx3reP8AgzyJPDJ4QYo09h1Kb0XjMPotp7PIZAze7KkOw4= Received: by 10.70.117.3 with SMTP id p3mr5308926wxc.1163251941271; Sat, 11 Nov 2006 05:32:21 -0800 (PST) Received: by 10.70.12.2 with HTTP; Sat, 11 Nov 2006 05:32:21 -0800 (PST) Message-ID: <3bbf2fe10611110532j5e1fb1f9t2e4399650abaac12@mail.gmail.com> Date: Sat, 11 Nov 2006 14:32:21 +0100 From: "Attilio Rao" Sender: asmrookie@gmail.com To: "Suleiman Souhlal" In-Reply-To: <455591E0.5070509@FreeBSD.org> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <3bbf2fe10610181518k68356528i154267c0bd1b1a77@mail.gmail.com> <3bbf2fe10611051857m4c644ad2o7d71a86e46eaf9a8@mail.gmail.com> <455591E0.5070509@FreeBSD.org> X-Google-Sender-Auth: 2945fec0e10f4f88 Cc: freebsd-current@freebsd.org, kmacy@freebsd.org, freebsd-arch@freebsd.org Subject: Re: sx locks rewriting - needs testers X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 11 Nov 2006 13:32:24 -0000 2006/11/11, Suleiman Souhlal : > Attilio Rao wrote: > > 2006/10/18, Attilio Rao : > > > >> In my P4 branch: //depot/user/attilio/attilio_smpng/... > >> you can find a sx locks rewriting using the optimized semantic of > >> rwlocks; in the end this might result in a valuable performance > >> improvement. > > Excellent work. > Do you have any benchmark results? You might have a hard time convincing > someone to commit this if you don't show that it's worth it, especially > if you consider how much it complicates the code. I'm going to produce them but before I would really add the support for adaptive spinning. jhb@ has a patch [1] that might reduce it but we need more benchmarks. I'm trying to provide them too. > > After have received very positive stress-test feedbacks from pho@ > > (that I would like to thank for his patience and work) I went ahead > > and rather completed the implementation. Now this is ready to be > > reviewed and possibly committed into the CVS. > > Even if I plan a longer work on this branch (about syncronizing > > primitives), I think it is time for a revision of the work done until > > now from SMPng people (jhb, kmacy, etc.) and possibily an inclusion > > into HEAD (patch actually is 58k...). > > > > Some few hints about the patch: > > - LOCK_DEBUG adds a dependence between sx.h and lock.h (as for rwlocks > > and mutex) and a the new options SXLOCK_NOINLINE is added > > - XFS locking is still disabled in the patch (I hope to do it for > > tomorrow, I'm in GMT+1). > > - Possibly the sleepqueue interface modifies and new flags might be > > documented in the manpages (and NOTES file too, in order to reflect > > SXLOCK_NOINLINE inclusion). > > - It misses still of the adaptive spinning code, but it can be > > inserted after without problems. > > We might want to avoid adaptive spinning, since sx locks may be held for > quite long periods of time. I'm not sure I got it. Adaptive spinning really only acts if the exclusive holder is running on another CPU. BTW, it needs more evaluations. > > You can download the code directly from perforce > > (//depot/user/attilio/attilio_smpng/...) but patches are available > > here: > > http://users.gufi.org/~rookie/works/patches/smpng06112006.diff > > http://users.gufi.org/~rookie/works/patches/_sx.h > > > > I hope you will enjoy it (feedbacks, ideas, comments would be very > > appreciated). > > > > Attilio > > A few comments: > --- //depot/vendor/freebsd/src/sys/i386/include/param.h 2006/01/09 06:10:20 > +++ //depot/user/attilio/attilio_smpng/i386/include/param.h 2006/10/03 > 21:33:06 > @@ -109,6 +109,15 @@ > #endif > > /* > + * Define our own cache alignment mask for syncronizing primitives. > Pentium4 > + * and Xeon want a 128-byte wide aligned syncronizing primitive in order to > + * minimize cache bus traffic on CPUs cache lines movements. > + */ > +#ifndef SYNC_ALIGN > +#define SYNC_ALIGN (128 - 1) > +#endif > + > > Please also add this to amd64, and maybe rename it to UMA_ALIGN_SYNC. > On other architectures, have it be the same as UMA_ALIGN_CACHE. This > way, you don't have to define SYNC_ALIGN in every C file that uses it. > > While there, you might also want to make UMA_ALIGN_CACHE the correct > size for i386/amd64 (64 bytes, on most machines, i believe?). Ideally, > this would be a variable set at boot time, depending on what the CPU > reports. 32 for ia32, 64 for amd64, I guess. BTW, I think you are right, I will modify. > I also feel that the part that makes turnstiles and sleepqueues 128 byte > aligned should be broken up as as a separate patch/commit, as it doesn't > really have much to do with your sx rewriting, as I understand it. Yes, but this branch contains all the modifies I'm doing in order to improve our syncronizing primitives. I'm a little bit scared about patch dimensions... (actually is 60k and it necessarily needs to grow). > --- //depot/vendor/freebsd/src/sys/kern/kern_sx.c 2006/08/15 18:31:36 > +++ //depot/user/attilio/attilio_smpng/kern/kern_sx.c 2006/11/06 02:24:27 > @@ -78,13 +50,8 @@ > sx_init(struct sx *sx, const char *description) > { > > - sx->sx_lock = mtx_pool_find(mtxpool_lockbuilder, sx); > - sx->sx_cnt = 0; > - cv_init(&sx->sx_shrd_cv, description); > - sx->sx_shrd_wcnt = 0; > - cv_init(&sx->sx_excl_cv, description); > - sx->sx_excl_wcnt = 0; > - sx->sx_xholder = NULL; > + sx->sx_lock = SX_UNHELD; > + sx->sx_desc = "sx lock"; > lock_init(&sx->sx_object, &lock_class_sx, description, NULL, > LO_WITNESS | LO_RECURSABLE | LO_SLEEPABLE | LO_UPGRADABLE); > > Shouldn't it be sx->sx_desc = description; ? right, thanks. > Keep up the good work! I hope to see some benchmark results and to see > it committed soon! :-) Thanks a lot for your feedbacks, Attilio -- Peace can only be achieved by understanding - A. Einstein