Date: Mon, 07 Nov 2011 11:45:38 -0600 From: Alan Cox <alc@rice.edu> To: Kostik Belousov <kostikbel@gmail.com> Cc: mdf@freebsd.org, "K. Macy" <kmacy@freebsd.org>, Andriy Gapon <avg@freebsd.org>, freebsd-current@freebsd.org, Benjamin Kaduk <kaduk@mit.edu>, Penta Upa <bsdboot@gmail.com> Subject: Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3] Message-ID: <4EB81942.70501@rice.edu> In-Reply-To: <20111106124331.GP50300@deviant.kiev.zoral.com.ua> References: <4EB2D48E.1030102@rice.edu> <20111104100828.GG50300@deviant.kiev.zoral.com.ua> <4EB40015.5040100@rice.edu> <20111104153004.GK50300@deviant.kiev.zoral.com.ua> <4EB4095D.3030303@rice.edu> <20111104160339.GM50300@deviant.kiev.zoral.com.ua> <20111105141306.GW50300@deviant.kiev.zoral.com.ua> <CAMBSHm86TaJnRRgmPA_t7tiPfQsPyoTqz3ymdHSY1H3t5G864Q@mail.gmail.com> <20111105151530.GX50300@deviant.kiev.zoral.com.ua> <4EB595FA.4020500@rice.edu> <20111106124331.GP50300@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On 11/06/2011 06:43, Kostik Belousov wrote: > On Sat, Nov 05, 2011 at 03:00:58PM -0500, Alan Cox wrote: >> On 11/05/2011 10:15, Kostik Belousov wrote: >>> On Sat, Nov 05, 2011 at 07:37:48AM -0700, mdf@freebsd.org wrote: >>>> On Sat, Nov 5, 2011 at 7:13 AM, Kostik Belousov<kostikbel@gmail.com> >>>> wrote: >>>>> On Fri, Nov 04, 2011 at 06:03:39PM +0200, Kostik Belousov wrote: >>>>> >>>>> Below is the KBI patch after vm_page_bits_t merge is done. >>>>> Again, I did not spent time converting all in-tree consumers >>>> >from the (potentially) loadable modules to the new KPI until it >>>>> is agreed upon. >>>> I like my bikeshed orange... >>>> >>>> I would think a more canonical name would be get/set rather than >>>> read/write, especially since these operations aren't reading and >>>> writing the page (neither are they getting the page, but at least set >>>> is pretty unambiguous). >>> Look at the vm_page.h:385. vm_page_set_valid() is already taken. >> I don't feel good about creating an interface under which we have >> functions named vm_page_set_valid() and vm_page_write_valid(). I think >> that we should take a step back and look at the whole of set of >> functions that exist for manipulating the page's valid and dirty field >> and see if we can come up with a logical schema for naming them. I >> wouldn't then be surprised if this results in renaming some of the >> existing functions. >> >> However, this should not delay the changes to address the vm_page_lock >> problem. I had two questions about that part of your patch. First, is >> there any reason that you didn't include vm_page_lockptr()? If we >> created the page locking macros that you, jhb@, and I were talking about >> last week, then vm_page_lockptr() would be required. Second, there >> seems to be precedent for naming the locking functions _vm_page_lock() >> instead of vm_page_lock_func(), for example, the mutex code, i.e., >> sys/mutex.h, and the vm map locking functions. > I think vm_page_lockptr() should be included when some kind of > reloc-iterating macros are actually introduced into the tree. And, > I have a hope that they can be wrapped around a function with the > signature like > void vm_page_relock(vm_page_t locked_page, vm_page_t unlocked_page); > which moves the lock from locked_page to unlocked_page. > Ok. That sounds reasonable. > Regarding the _vm_page_lock() vs. vm_page_lock_func(), the mutex.h has > a lot of violations in regard of the namespaces, IMO. The __* namespace > is reserved for the language implementation, so our freestanding program > (kernel) ignores the requirements of the C standard with the names like > __mtx_lock_spin(). Using the name _vm_page_lock() is valid, but makes > it not unreasonable for other developers to introduce reserved names. > So I decided to use the suffixes. vm_map.h locking is free of these > violations. > Ok. I'll offer one final suggestion. Please consider an alternative suffix to "func". Perhaps, "kbi" or "KBI". In other words, something that hints at the function's reason for existing. Alan
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4EB81942.70501>