From owner-freebsd-current@FreeBSD.ORG Tue May 14 20:13:10 2013 Return-Path: Delivered-To: current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id ACEF6408 for ; Tue, 14 May 2013 20:13:10 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-we0-x231.google.com (mail-we0-x231.google.com [IPv6:2a00:1450:400c:c03::231]) by mx1.freebsd.org (Postfix) with ESMTP id 46965926 for ; Tue, 14 May 2013 20:13:10 +0000 (UTC) Received: by mail-we0-f177.google.com with SMTP id q58so849954wes.36 for ; Tue, 14 May 2013 13:13:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=x-received:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=roLNx0hEc3/w6pfPWumbDXI24VPyAbPY7pW0BDcU33Q=; b=jATAI+MEJVYujAf9cV7BhWdqewzqZPb3kZGPYCMSKsjWNR32D2FpVy7FlE07HIDAWe m2kDGzOssy+nJI4MsUD94cvQOaFr8ZGFuPiLf0WmiQseTe/xeFdFYpsLlfYboxLXdLXX RQveHo4xWzyunq571EXuh0IhHycaVYHKRCu2DFy7VAtTPVsg1p8+MCPeNJXY72uX0tvS PqpEw0lmgsuKAEZu1BugtZdH2NASjbRJRPop+TYlj1lhI7927gDARHONjrH0usEFc1Sx eun+icOE3xDrz+74gIMJtqlvYBbGwJ4KlvDrYfSY26+UMYh5+FghKjp0tpagTNe7w4AR GUVg== X-Received: by 10.180.183.76 with SMTP id ek12mr9864200wic.30.1368562389138; Tue, 14 May 2013 13:13:09 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id fv11sm12066908wic.11.2013.05.14.13.13.07 for (version=TLSv1 cipher=RC4-SHA bits=128/128); Tue, 14 May 2013 13:13:07 -0700 (PDT) Date: Tue, 14 May 2013 22:13:05 +0200 From: Mateusz Guzik To: Konstantin Belousov Subject: Re: sysvshm: replace Giant with a local sx lock Message-ID: <20130514201305.GA15195@dft-labs.eu> References: <20130423203823.GA6346@dft-labs.eu> <20130423205532.GE67273@kib.kiev.ua> <20130423213621.GB6346@dft-labs.eu> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20130423213621.GB6346@dft-labs.eu> User-Agent: Mutt/1.5.20 (2009-06-14) Cc: current@freebsd.org X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 14 May 2013 20:13:10 -0000 On Tue, Apr 23, 2013 at 11:36:21PM +0200, Mateusz Guzik wrote: > On Tue, Apr 23, 2013 at 11:55:32PM +0300, Konstantin Belousov wrote: > > On Tue, Apr 23, 2013 at 10:38:23PM +0200, Mateusz Guzik wrote: > > > I would like to replace Giant with a local sx lock in sysvshm code. > > > Looked really straightforward so maybe I missed something. > > > > At very least, the shmget_existing() is no longer functional. > > The sx is owned around tsleep(), and thus a progress cannot be made > > by other thread, which needs the same sx lock. > > > > Use of the SHMSEG_REMOVED in the shmget_allocate_segment() does > > not make any sense in your patch, since sleeping malloc allocation > > owns sx and prevent other threads from finding the segment. > > > > I did not looked further. > > Thank you for review, I definitely skimmed too fast. > > Looks like this code has some bugs as it is already, e.g. kern_shmat > does not re-check for NULL p->p_vmspace->vm_shm after malloc. > So I produced 2 patches. First one is straightforward: remove shmrealloc as it is a no-op anyway: http://people.freebsd.org/~mjg/patches/sysvshm-remove-shmrealloc.patch Second one replaces Giant with sx lock and removes current support for allocations that dropped Giant. Unfortunately I didn't have any good ideas how to split this patch: http://people.freebsd.org/~mjg/patches/sysvshm-giant-sx2.patch Bugs in current support: - p->p_vmspace->vm_shm allocation race (2 threads) in shmat - vm_map_find can drop Giant, not taken into account in shmat - lack of clean up if vm_pager_allocate fails While it is possible to fix all these problems, I think sx lock that is not dropped is ok to use here and it simplifies the implementation. Is this acceptable? -- Mateusz Guzik