Date: Fri, 5 Mar 2010 02:33:57 +0100 From: Attilio Rao <attilio@freebsd.org> To: Alexander Motin <mav@freebsd.org> Cc: freebsd-scsi@freebsd.org, "Justin T. Gibbs" <gibbs@freebsd.org>, Matthew Jacob <mj@feral.com> Subject: Re: How is supposed to be protected the units list? Message-ID: <3bbf2fe11003041733x5325cdd8x5766551e22565a81@mail.gmail.com> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
2010/3/4 Alexander Motin <mav@freebsd.org>: > Hi. > > Attilio Rao wrote: >> 2010/3/1 Attilio Rao <attilio@freebsd.org>: >>> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3bbf2fe11003041733x5325cdd8x5766551e22565a81>