Date: Thu, 7 Feb 2008 12:07:55 -0800 From: Marcel Moolenaar <xcllnt@mac.com> To: Attilio Rao <attilio@FreeBSD.org> Cc: Kostik Belousov <kostikbel@gmail.com>, current@FreeBSD.org Subject: Re: Old LOR between devfs & devfsmount resurfacing? Message-ID: <9D3A97C3-F295-44B2-82AA-FC220E43C0E1@mac.com> In-Reply-To: <3bbf2fe10802071132q7bd92f99x685ab1ea23e37772@mail.gmail.com> References: <B269B28B-C66E-4AC6-A4D9-FBA378466F89@juniper.net> <20080207104354.GY57756@deviant.kiev.zoral.com.ua> <3bbf2fe10802070304r29cb8d2u1210fe285c917424@mail.gmail.com> <20080207110901.GZ57756@deviant.kiev.zoral.com.ua> <3bbf2fe10802070421m3a3152a3m6c9aa67d649107e4@mail.gmail.com> <20080207125252.GC57756@deviant.kiev.zoral.com.ua> <3bbf2fe10802070611v6c7714b5y18bef10d586944c4@mail.gmail.com> <4CB73483-B3BB-4819-B3B5-5F349D16C6F7@mac.com> <3bbf2fe10802071041j6d2748f9n16c28541da1e0430@mail.gmail.com> <3150AF40-205F-4424-814F-6A9305E698E7@mac.com> <3bbf2fe10802071132q7bd92f99x685ab1ea23e37772@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Feb 7, 2008, at 11:32 AM, Attilio Rao wrote: > 2008/2/7, Marcel Moolenaar <xcllnt@mac.com>: >> >> On Feb 7, 2008, at 10:41 AM, Attilio Rao wrote: >> >>> 2008/2/7, Marcel Moolenaar <xcllnt@mac.com>: >>>> On Feb 7, 2008, at 6:11 AM, Attilio Rao wrote: >>>> >>>>>>>>>>>> Correct lock order is devfs vnode -> devfs mount sx lock. >>>>>>>>>>>> When >>>>>>>>>>>> allocating new devfs vnode, see devfs_allocv(), the newly >>>>>>>>>>>> created >>>>>>>>>>>> vnode is locked while devfs mount lock already held (see >>>>>>>>>>>> line >>>>>>>>>>>> 250 of >>>>>>>>>>>> fs/devfs/devfs_vnops.c). Nonetheless, this cannot cause >>>>>>>>>>>> deadlock since >>>>>>>>>>>> no other thread can find the new vnode, and thus perform >>>>>>>>>>>> the >>>>>>>>>>>> other lock >>>>>>>>>>>> order for this vnode lock. >>>>>>>>>>>> >>>>>>>>>>>> The fix is to shut the witness in this particular case. >>>>>>>>>>>> Attilio, how to >>>>>>>>>>>> do this ? >>>>>>>>>>> >>>>>>>>>>> Just add LK_NOWITNESS for one of the lock involved in the >>>>>>>>>>> lockinit(). >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Then, we loss the useful reports of the actual LORs later, >>>>>>>>>> isn't it ? >>>>>>>>> >>>>>>>>> Another solution would be to rewamp BLESSING option which >>>>>>>>> allow to >>>>>>>>> 'bless' some LORs. >>>>>>>>> jhb and me, btw, didn't want to enable it because it could >>>>>>>>> lead >>>>>>>>> some >>>>>>>>> less experienced developer to hide LORs under this label and >>>>>>>>> this is >>>>>>>>> something we want to avoid. >>>>>>>> >>>>>>>> >>>>>>>> This LOR shall not be ignored globally. When real, it caused >>>>>>>> the >>>>>>>> easily >>>>>>>> reproducable lockup of the machine. >>>>>>>> >>>>>>>> It would be better to introduce some lockmgr flag to ignore >>>>>>>> _this_ locking. >>>>>>> >>>>>>> flag to pass where? >>>>>> To the lockmgr itself at the point of aquisition, like >>>>>> lockmgr(&lk, LK_EXCLUSIVE | LK_INTERLOCK | LK_NOWARN, >>>>>> &interlk, ...); >>>>> >>>>> No, I really want a general WITNESS support for this (as I also >>>>> think >>>>> that having something more fine grained than BLESSING will break >>>>> all >>>>> concerns jhb and me are considering now). >>>>> A simple way to do it would mean hard-coding file and line in a >>>>> witness table. While file is ok, line makes trouble so we should >>>>> find >>>>> an alternative way to do this. Otherwise we can consider skiping >>>>> checks for a whole function, this should be not so difficult to >>>>> achive. >>>>> >>>>> I need to think more about this. >>>> >>>> >>>> What about a linker set that lists file regions (based on line >>>> number). >>>> If you want to exclude a particular lock from WITNESS you can do >>>> something like this: >>>> WITNESS_REGION_START(function) >>>> lockmgr(...) >>>> WITNESS_REGION_END >>>> >>>> The WITNESS_REGION_START and WITNESS_WITNESS_END together create a >>>> region in the linker set and witness can check if a lock operation >>>> falls within that region. If yes, we can make it do something >>>> special >>>> by given the _START and/or _END a function pointer or we can make >>>> it >>>> ignore the operation by passing NULL or something. >>>> >>>> You can safely use file & line numbers in this case. Something >>>> along >>>> those lines... >>>> >>>> Thoughts? >>> >>> Really, if I wanted to pollute consumers code I would have use a lot >>> of simpler ideas. >> >> >> Where you see pollution, I see opportunity. In principle no code >> should have to use these region markers, so in the few cases >> it's needed it not only stands out, but also allows any sized >> region, possibly covering multiple functions to be marked. That's >> much less pollution and/or change than having to tag each and >> every lock operation. > > Yes, but what I mean is that it is enough to do something like: > > WITNESS_SKIPNEXTCHECK(); > lockmgr(...) > > and have the same result. Explicitly marking a region with START and END allows you to cover multiple functions and many locking operations. It's not at all the same as a tag per lock. A tag per lock is much more polluting that 2 markers that define a region (which can be the whole file). Also, if you only have the ability to skip the witness check, then you may still have something that's too limited. If we're going to sprinkle directives in the code relating to locking, we might as well have it do more than just tell witness to ignore the lock. We could have it do as much or little as call a function, which can do anything for us, including profiling, tracing and even extending the witness checks to include more than just lock ordering... Anyway: you got the gist. I'll leave you to it. -- Marcel Moolenaar xcllnt@mac.com
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?9D3A97C3-F295-44B2-82AA-FC220E43C0E1>