From owner-freebsd-current@FreeBSD.ORG Mon Nov 7 09:36:07 2011 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 00894106564A for ; Mon, 7 Nov 2011 09:36:07 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-ww0-f50.google.com (mail-ww0-f50.google.com [74.125.82.50]) by mx1.freebsd.org (Postfix) with ESMTP id 29CEF8FC13 for ; Mon, 7 Nov 2011 09:36:05 +0000 (UTC) Received: by wwp14 with SMTP id 14so6370660wwp.31 for ; Mon, 07 Nov 2011 01:36:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; 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 :content-transfer-encoding; bh=c8P4FXpYWJqOFkcD4wORoP7H7hEsNdIc6jZFRTQXQ+8=; b=hTSSwNE678BszQp/YOTgu8hjZKH274Y9I4ty6foQ9AYqA1DlNMUQ+O1qk8+OD1rphU QVTxkSgzPrlooJI2R9zt7XuMzznEn9uwKHb3yYf3FlRmumcB6jKXbdar343eNS00rQaK 7P3ud1nhbpWanuy1pe+dXMIWRmEjP3y8CSTlc= MIME-Version: 1.0 Received: by 10.216.166.212 with SMTP id g62mr5257133wel.29.1320658565218; Mon, 07 Nov 2011 01:36:05 -0800 (PST) Sender: asmrookie@gmail.com Received: by 10.216.182.3 with HTTP; Mon, 7 Nov 2011 01:36:05 -0800 (PST) In-Reply-To: References: <4EB11C32.80106@FreeBSD.org> <4EB22938.4050803@rice.edu> <20111103132437.GV50300@deviant.kiev.zoral.com.ua> <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> Date: Mon, 7 Nov 2011 10:36:05 +0100 X-Google-Sender-Auth: BeRS1fA4sGozzyYh26DZ2tnLdg4 Message-ID: From: Attilio Rao To: Arnaud Lacombe Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Cc: "K. Macy" , Alan Cox , Andriy Gapon , freebsd-current@freebsd.org, Benjamin Kaduk , Kostik Belousov , Penta Upa Subject: Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3] X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 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: Mon, 07 Nov 2011 09:36:07 -0000 2011/11/7 Arnaud Lacombe : > Hi, > > On Sat, Nov 5, 2011 at 10:13 AM, Kostik Belousov wr= ote: >> 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. >> >> diff --git a/sys/nfsclient/nfs_bio.c b/sys/nfsclient/nfs_bio.c >> index 305c189..7264cd1 100644 >> --- a/sys/nfsclient/nfs_bio.c >> +++ b/sys/nfsclient/nfs_bio.c >> @@ -128,7 +128,7 @@ nfs_getpages(struct vop_getpages_args *ap) >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 * can only occur at the file EOF. >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 */ >> =C2=A0 =C2=A0 =C2=A0 =C2=A0VM_OBJECT_LOCK(object); >> - =C2=A0 =C2=A0 =C2=A0 if (pages[ap->a_reqpage]->valid !=3D 0) { >> + =C2=A0 =C2=A0 =C2=A0 if (vm_page_read_valid(pages[ap->a_reqpage]) !=3D= 0) { >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0for (i =3D 0; i <= npages; ++i) { >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0if (i !=3D ap->a_reqpage) { >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0vm_page_lock(pages[i]); >> @@ -198,16 +198,16 @@ nfs_getpages(struct vop_getpages_args *ap) >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0/* >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 * Read operation filled an entire page >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 */ >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 m->valid =3D VM_PAGE_BITS_ALL; >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 KASSERT(m->dirty =3D=3D 0, >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 vm_page_write_valid(m, VM_PAGE_BITS_ALL); >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 KASSERT(vm_page_read_dirty(m) =3D=3D 0, >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0("nfs_getpages: page %p is dirty", m)); >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0} else if (size >= toff) { >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0/* >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 * Read operation filled a partial page. >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 */ >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 m->valid =3D 0; >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 vm_page_write_valid(m, 0); >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0vm_page_set_valid(m, 0, size - toff); >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 KASSERT(m->dirty =3D=3D 0, >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 KASSERT(vm_page_read_dirty(m) =3D=3D 0, >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0("nfs_getpages: page %p is dirty", m)); >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0} else { >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0/* >> diff --git a/sys/vm/vm_page.c b/sys/vm/vm_page.c >> index 389aea5..2f41e70 100644 >> --- a/sys/vm/vm_page.c >> +++ b/sys/vm/vm_page.c >> @@ -2677,6 +2677,66 @@ vm_page_test_dirty(vm_page_t m) >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0vm_page_dirty(m); >> =C2=A0} >> >> +void >> +vm_page_lock_func(vm_page_t m, const char *file, int line) >> +{ >> + >> +#if LOCK_DEBUG > 0 || defined(MUTEX_NOINLINE) >> + =C2=A0 =C2=A0 =C2=A0 _mtx_lock_flags(vm_page_lockptr(m), 0, file, line= ); >> +#else >> + =C2=A0 =C2=A0 =C2=A0 __mtx_lock(vm_page_lockptr(m), 0, file, line); >> +#endif >> +} >> + > Why do you re-implement the wheel ? all the point of these assessors > is to hide implementation detail. IMO, you should restrict yourself to > the documented API from mutex(9), only. > > Oh, wait, you end-up using LOCK_FILE instead of just __FILE__, but > wait LOCK_FILE is either just __FILE__, or NULL, depending on > LOCK_DEBUG, but you wouldn't have those function without > INVARIANTS.... This whole LOCK_FILE/LOCK_LINE seem completely > fracked-up... If you don't want this code in INVARIANTS, but in > LOCK_DEBUG, only make it live only in the LOCK_DEBUG case. > > Btw, let me also question the use of non-inline functions. My impression is that you don't really understand the patch, thus your disrespectful words used here are really unjustified. I think that kib@ intention is just to get "the most official way" to pass down file and line to locking functions from the consumers. His patch is "technically right", but I would prefer something different (see below). LOCK_FILE and LOCK_LINE exist for reducing the space in .rodata section. Without INVARIANTS/WITNESS/etc. they will just be NULL and not pointing to a lot of datas that won't be used in the opposite case. I'm unsure if this replies to your concerns because you just criticize without making a real technical question in this post. >> +void >> +vm_page_unlock_func(vm_page_t m, const char *file, int line) >> +{ >> + >> +#if LOCK_DEBUG > 0 || defined(MUTEX_NOINLINE) >> + =C2=A0 =C2=A0 =C2=A0 _mtx_unlock_flags(vm_page_lockptr(m), 0, file, li= ne); >> +#else >> + =C2=A0 =C2=A0 =C2=A0 __mtx_unlock(vm_page_lockptr(m), curthread, 0, fi= le, line); >> +#endif >> +} Kostik, we usually catered this case by having interfaces directly specified in mutex.h in order to keep the implementation details "compact enough" (see the thread_lock() case for this). I'm unsure what you prefer here, at least for the locking functions you could move over there as there are cons and prons for both approaches really and I'm fine with both. Attilio --=20 Peace can only be achieved by understanding - A. Einstein