From owner-freebsd-scsi@FreeBSD.ORG Fri Mar 5 01:34:01 2010 Return-Path: Delivered-To: freebsd-scsi@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 8A7511065670; Fri, 5 Mar 2010 01:34:01 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-iw0-f173.google.com (mail-iw0-f173.google.com [209.85.223.173]) by mx1.freebsd.org (Postfix) with ESMTP id 354F48FC13; Fri, 5 Mar 2010 01:34:00 +0000 (UTC) Received: by iwn3 with SMTP id 3so2327220iwn.13 for ; Thu, 04 Mar 2010 17:33:57 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:sender:received:in-reply-to :references:date:x-google-sender-auth:message-id:subject:from:to:cc :content-type; bh=emhZeRIzKs/UOjCNL10cTnHH2wQpsFUwFwvx229w+g8=; b=MsdV40fyLlofkTm9nduw24S7t4PP4ey743mjVy9/D34nN3lnHGbfz7XjJH3pLAWbUU DDbFGed3ovylsBDeSv4lvd0sPzEtK36rbj/KclP4P3zu6jolB3s8P3CSPoX/VyIBgwc/ NExroZUXHZ0V1GAn5XFAXKy4cqJOPHeIpJpHY= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type; b=QgSR8DeKgqrHPCaR4sDajCLgdkBdGyBKFO/7rC0QDbfZCv5d1WNK6B3Mpeu87GzCi6 Tgl7gYtXlZ9FscNT5kQUuNFRzQhV17JESIS+lQTEs6tRP163081T6avdcoq/i3Pzr/B/ K5hbBTnBbes0ha02LqSbGoYjaOXe1/skdmVhQ= MIME-Version: 1.0 Sender: asmrookie@gmail.com Received: by 10.231.154.8 with SMTP id m8mr101815ibw.2.1267752837632; Thu, 04 Mar 2010 17:33:57 -0800 (PST) In-Reply-To: <4B8F7B51.6050403@FreeBSD.org> References: <1267417389.00224447.1267407002@10.7.7.3> <1267554183.00225132.1267543801@10.7.7.3> <4B8F7B51.6050403@FreeBSD.org> Date: Fri, 5 Mar 2010 02:33:57 +0100 X-Google-Sender-Auth: e33e21368314cc1b Message-ID: <3bbf2fe11003041733x5325cdd8x5766551e22565a81@mail.gmail.com> From: Attilio Rao To: Alexander Motin Content-Type: text/plain; charset=UTF-8 Cc: freebsd-scsi@freebsd.org, "Justin T. Gibbs" , Matthew Jacob Subject: Re: How is supposed to be protected the units list? X-BeenThere: freebsd-scsi@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SCSI subsystem List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 05 Mar 2010 01:34:01 -0000 2010/3/4 Alexander Motin : > Hi. > > Attilio Rao wrote: >> 2010/3/1 Attilio Rao : >>> I have a question that I've been unable to reply reading the code. >>> Someone could point me to documentation explaining how the unit tailq >>> (within a struct periph_driver) is supposed to be locked? >>> I'm not sure how it is assured consistency of accesses to the list and >>> more important how is ensured that the periphs composing it doesn't go >>> away as I don't see any reference bump for objects inserted there. > > I am not sure that existing implementation is complete, but at least in > some places it is protected by xpt_lock_buses(), which is wrapper for > mtx_lock(&xsoftc.xpt_topo_lock). camperiphfree() called under the lock, > same as many (all?) traverses. Initially I thought about using xpt_lock_buses() but then I regret in order to avoid any unconvenient LOR. Using a new lock would decrease the chances to see this happening. >> I don't think the lists are protected at all so I made this simple >> patch taking advantage by a global lock: >> http://www.freebsd.org/~attilio/Sandvine/pdrv/pdrv_lock.diff >> >> The patch is simple enough but I just test-compiled it (will need some >> time to run in a debugging kernel, hope to do tonight) and maybe you >> can already give your opinions here. > > It is not obvious to me, how your new lock expected to interoperate with > existing xpt_topo_lock. I have feeling that it duplicates one in some > places. If you believe that second lock is needed there, then probably > first one should be partially removed. But then we should be careful to > not create LORs between them, as they are not natively nested. > > Changes in cam_xpt.c break splbreaknum meaning of temporary dropping > xpt_topo_lock. I have doubts whether it is required now at all now, but > leaving dead code it definitely not good. If you think that using xpt_topo_lock in all the other places accessing units that don't do right now is suitable and safe I'm fine with it. Otherwise the new global lock should be fine I assume. Thanks, Attilio -- Peace can only be achieved by understanding - A. Einstein