Re: Using Instruction Pointer address in debug interfaces [Was: Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]]
On 12/1/11 5:04 PM, Arnaud Lacombe wrote: Hi, On Mon, Nov 14, 2011 at 5:14 AM, Andriy Gapon wrote: on 14/11/2011 02:38 Arnaud Lacombe said the following: you (committers) I wonder how it would work out if you were made a committer and couldn't say "you (committers)" any more... :-) The real question is rather whether or not I would accept such a role, or better, would I ever be proposed such a role ? I doubt someone praising the Bazaar, openly challenging core and historical members, would fit in the Cathedral, even if my work is only ever gonna be in the `projects/' subdirectory. That's really funny.. we are the bazaar. Any developer can pretty quickly get to be a committer (*) and after mentorship (6 months) can commit to any part of the code. Linux is the catherderal. It has a pope, a whole array of bishops and then priests and finally millions of serfs. (*) we propose people to get commit bits on how useful they seem to be rather that their political views. - Arnaud ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org" ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Using Instruction Pointer address in debug interfaces [Was: Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]]
on 02/12/2011 03:04 Arnaud Lacombe said the following: > Hi, > > On Mon, Nov 14, 2011 at 5:14 AM, Andriy Gapon wrote: >> on 14/11/2011 02:38 Arnaud Lacombe said the following: >>> you (committers) >> >> I wonder how it would work out if you were made a committer and couldn't say >> "you (committers)" any more... :-) >> > The real question is rather whether or not I would accept such a role, [*] That would be solely up to you. > or better, would I ever be proposed such a role ? That's up to you, current committers and core. > I doubt someone praising the Bazaar, openly challenging core and > historical members, would fit in the Cathedral, even if my work is > only ever gonna be in the `projects/' subdirectory. Your ideals are not that important if you are useful to the project, follow its policies and can get along with other developers. And challenging is a healthy thing as long as it stays constructive and focused on the code, not on the persons/personalities. [*] - I still think that what I asked was _the_ real question. If you want just to show off and keep pointing out how them committers are not worthy instead of nicely cooperating with them, then "you" and "committers" will always be apart, I'd guess. -- Andriy Gapon ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Using Instruction Pointer address in debug interfaces [Was: Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]]
Hi, On Mon, Nov 14, 2011 at 5:14 AM, Andriy Gapon wrote: > on 14/11/2011 02:38 Arnaud Lacombe said the following: >> you (committers) > > I wonder how it would work out if you were made a committer and couldn't say > "you (committers)" any more... :-) > The real question is rather whether or not I would accept such a role, or better, would I ever be proposed such a role ? I doubt someone praising the Bazaar, openly challenging core and historical members, would fit in the Cathedral, even if my work is only ever gonna be in the `projects/' subdirectory. - Arnaud ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
2011/11/20 Kostik Belousov : > On Sun, Nov 20, 2011 at 08:22:38PM +0100, Attilio Rao wrote: >> 2011/11/20 Kostik Belousov : >> > On Sun, Nov 20, 2011 at 08:04:21PM +0100, Attilio Rao wrote: >> >> This other patch converts sx to a similar interface which cleans up >> >> vm_map.c: >> >> http://www.freebsd.org/~attilio/sxfileline.patch >> >> >> >> What do you think about it? >> > >> > This one only changes the KBI ? Note that _sx suffix is not reserved. >> >> In which sense? >> If you want to keep the shim support for KLD (thus the hard path) you >> will always need to keep an hard function and thus you still need a >> macro acting as a gate between the 'hard function' (or KLD version, if >> you prefer) and the fast case, that is where the "_" suffix came from. > > As I see, right now kernel exports e.g. _sx_try_slock() for the hard path. > After the patch, it will export sx_try_slock_() for the same purpose. > The old modules, which call _sx_try_slock(), cannot be loaded into > the patched kernel. Am I reading the patch wrong ? We shouldn't be concerned about it for -CURRENT, when MFCing this patch I'll just make: #define sx_try_slock_ _sx_try_slock rather than renaming the function. Attilio -- Peace can only be achieved by understanding - A. Einstein ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
On Sun, Nov 20, 2011 at 08:22:38PM +0100, Attilio Rao wrote: > 2011/11/20 Kostik Belousov : > > On Sun, Nov 20, 2011 at 08:04:21PM +0100, Attilio Rao wrote: > >> This other patch converts sx to a similar interface which cleans up > >> vm_map.c: > >> http://www.freebsd.org/~attilio/sxfileline.patch > >> > >> What do you think about it? > > > > This one only changes the KBI ? Note that _sx suffix is not reserved. > > In which sense? > If you want to keep the shim support for KLD (thus the hard path) you > will always need to keep an hard function and thus you still need a > macro acting as a gate between the 'hard function' (or KLD version, if > you prefer) and the fast case, that is where the "_" suffix came from. As I see, right now kernel exports e.g. _sx_try_slock() for the hard path. After the patch, it will export sx_try_slock_() for the same purpose. The old modules, which call _sx_try_slock(), cannot be loaded into the patched kernel. Am I reading the patch wrong ? pgperrxtbJjmU.pgp Description: PGP signature
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
2011/11/20 Kostik Belousov : > On Sun, Nov 20, 2011 at 08:04:21PM +0100, Attilio Rao wrote: >> This other patch converts sx to a similar interface which cleans up vm_map.c: >> http://www.freebsd.org/~attilio/sxfileline.patch >> >> What do you think about it? > > This one only changes the KBI ? Note that _sx suffix is not reserved. In which sense? If you want to keep the shim support for KLD (thus the hard path) you will always need to keep an hard function and thus you still need a macro acting as a gate between the 'hard function' (or KLD version, if you prefer) and the fast case, that is where the "_" suffix came from. Attilio -- Peace can only be achieved by understanding - A. Einstein ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
On Sun, Nov 20, 2011 at 08:04:21PM +0100, Attilio Rao wrote: > This other patch converts sx to a similar interface which cleans up vm_map.c: > http://www.freebsd.org/~attilio/sxfileline.patch > > What do you think about it? This one only changes the KBI ? Note that _sx suffix is not reserved. pgpCdjEU3nGge.pgp Description: PGP signature
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
2011/11/20 Attilio Rao : > 2011/11/18 Attilio Rao : >> 2011/11/18 Attilio Rao : >>> 2011/11/18 Kostik Belousov : On Fri, Nov 18, 2011 at 11:40:28AM +0100, Attilio Rao wrote: > 2011/11/16 Kostik Belousov : > > On Tue, Nov 15, 2011 at 07:15:01PM +0100, Attilio Rao wrote: > >> 2011/11/7 Kostik Belousov : > >> > On Mon, Nov 07, 2011 at 11:45:38AM -0600, Alan Cox wrote: > >> >> 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. > >> > > >> > Sure. Below is the extraction of only vm_page_lock() bits, together > >> > with the suggested rename. When Attilio provides the promised > >> > simplification > >> > of the mutex KPI, this can be reduced. > >> > >> My tentative patch is here: > >> http://www.freebsd.org/~attilio/mutexfileline.patch > >> > >> I need to make more compile testing later, but it already compiles > >> GENERIC + modules fine on HEAD. > >> > >> The patch provides a common entrypoint, option independent, for both > >> fast case and debug/compat case. > >> Additively, it almost entirely fixes the standard violation of the > >> reserved namespace, as you described (the notable exception being the > >> macro used in the fast path, that I want to fix as well, but in a > >> separate commit). > >> > >> Now the file/line couplet can be passed to the "_" suffix variant of > >> the flag functions. > > Yes, this is exactly KPI that I would use when available for the > > vm_page_lock() patch. > > > >> > >> eadler@ reviewed the mutex.h comment. > >> > >> Please let me know what you think about it, as long as we agree on the > >> patch I'll commit it. > > But I also agree with John that imposing large churn due to the > > elimination > > of the '__' prefix is too late now. At least it will make the change > > non-MFCable. Besides, we already lived with the names for 10+ years. > > > > I will be happy to have the part of the patch that exports the > > mtx_XXX_(mtx, > > file, line) defines which can be used without taking care of LOCK_DEBUG > > or MUTEX_NOINLINE in the consumer code. > > Ok, this patch should just add the compat stub: > http://www.freebsd.org/~attilio/mutexfileline2.patch Am I right that I would use mtx_lock_(mtx, file, line) etc ? If yes, I am fine with it. >>> >>> Yes that is correct. >>> >>> However, I'm a bit confused on one aspect: would you mind using >>> _mtx_lock_flags() instead? >>> If you don't mind the "underscore namespace violation" I think I can >>> make a much smaller patch against HEAD for it. >>> >>> Otherwise, the one now posted should be ok. >> >> After thinking more about it, I think that is basically the shorter >> version I can came up with. >> >> Please consider: >> http://www.freebsd.org/~attilio/mutexfileline2.patch > > This is now committed as r227758,227759, you can update your patch now. This other patch converts sx to a similar interface which cleans up vm_map.c: http://www.freebsd.org/~attilio/sxfileline.patch What do you think about it? Attilio -- Peace can only be achieved by understanding - A. Einstein ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
It looks good to me. Attilio 2011/11/20 Kostik Belousov : > On Sun, Nov 20, 2011 at 07:02:14PM +0100, Attilio Rao wrote: >> 2011/11/20 Kostik Belousov : >> > +#define vm_page_lock_assert(m, a) \ >> > + vm_page_lock_assert_KBI((m), (a), LOCK_FILE, LOCK_LINE) >> >> I think you should put the "\" in the last tab and also, for >> consistency, you may want to use __FILE__ and __LINE__ for assert (or >> maybe I should also switch mutex.h to use LOCK_FILE and LOCK_LINE at >> some point?). > I never saw the requirement for the backslash. I am consistent with > PA_UNLOCK_COND() several lines above. > > Changed assert to use __FILE/LINE__. > >> >> > +#else >> > +#define vm_page_lock_assert(m, a) >> > +#endif >> > +#else /* KLD_MODULE */ >> >> This should be /* !KLD_MODULE */, I guess? > Changed. > >> >> > #define vm_page_lockptr(m) >> >> This is not defined for the KLD_MODULE case? > Yes, explicitely. This was discussed. > http://lists.freebsd.org/pipermail/freebsd-current/2011-November/029009.html > > diff --git a/sys/vm/vm_page.c b/sys/vm/vm_page.c > index d592ac0..74e5126 100644 > --- a/sys/vm/vm_page.c > +++ b/sys/vm/vm_page.c > @@ -2843,6 +2843,34 @@ vm_page_test_dirty(vm_page_t m) > vm_page_dirty(m); > } > > +void > +vm_page_lock_KBI(vm_page_t m, const char *file, int line) > +{ > + > + mtx_lock_flags_(vm_page_lockptr(m), 0, file, line); > +} > + > +void > +vm_page_unlock_KBI(vm_page_t m, const char *file, int line) > +{ > + > + mtx_unlock_flags_(vm_page_lockptr(m), 0, file, line); > +} > + > +int > +vm_page_trylock_KBI(vm_page_t m, const char *file, int line) > +{ > + > + return (mtx_trylock_flags_(vm_page_lockptr(m), 0, file, line)); > +} > + > +void > +vm_page_lock_assert_KBI(vm_page_t m, int a, const char *file, int line) > +{ > + > + mtx_assert_(vm_page_lockptr(m), a, file, line); > +} > + > int so_zerocp_fullpage = 0; > > /* > diff --git a/sys/vm/vm_page.h b/sys/vm/vm_page.h > index 151710d..1fab735 100644 > --- a/sys/vm/vm_page.h > +++ b/sys/vm/vm_page.h > @@ -218,11 +218,23 @@ extern struct vpglocks pa_lock[]; > > #define PA_LOCK_ASSERT(pa, a) mtx_assert(PA_LOCKPTR(pa), (a)) > > +#ifdef KLD_MODULE > +#define vm_page_lock(m) vm_page_lock_KBI((m), LOCK_FILE, > LOCK_LINE) > +#define vm_page_unlock(m) vm_page_unlock_KBI((m), LOCK_FILE, > LOCK_LINE) > +#define vm_page_trylock(m) vm_page_trylock_KBI((m), LOCK_FILE, > LOCK_LINE) > +#ifdef INVARIANTS > +#define vm_page_lock_assert(m, a) \ > + vm_page_lock_assert_KBI((m), (a), __FILE__, __LINE__) > +#else > +#define vm_page_lock_assert(m, a) > +#endif > +#else /* !KLD_MODULE */ > #define vm_page_lockptr(m) (PA_LOCKPTR(VM_PAGE_TO_PHYS((m > #define vm_page_lock(m) mtx_lock(vm_page_lockptr((m))) > #define vm_page_unlock(m) mtx_unlock(vm_page_lockptr((m))) > #define vm_page_trylock(m) mtx_trylock(vm_page_lockptr((m))) > #define vm_page_lock_assert(m, a) > mtx_assert(vm_page_lockptr((m)), (a)) > +#endif > > #define vm_page_queue_free_mtx vm_page_queue_free_lock.data > /* > @@ -405,6 +417,11 @@ void vm_page_cowfault (vm_page_t); > int vm_page_cowsetup(vm_page_t); > void vm_page_cowclear (vm_page_t); > > +void vm_page_lock_KBI(vm_page_t m, const char *file, int line); > +void vm_page_unlock_KBI(vm_page_t m, const char *file, int line); > +int vm_page_trylock_KBI(vm_page_t m, const char *file, int line); > +void vm_page_lock_assert_KBI(vm_page_t m, int a, const char *file, int line); > + > #ifdef INVARIANTS > void vm_page_object_lock_assert(vm_page_t m); > #define VM_PAGE_OBJECT_LOCK_ASSERT(m) vm_page_object_lock_assert(m) > -- Peace can only be achieved by understanding - A. Einstein ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
On Sun, Nov 20, 2011 at 07:02:14PM +0100, Attilio Rao wrote: > 2011/11/20 Kostik Belousov : > > +#define vm_page_lock_assert(m, a) \ > > + vm_page_lock_assert_KBI((m), (a), LOCK_FILE, LOCK_LINE) > > I think you should put the "\" in the last tab and also, for > consistency, you may want to use __FILE__ and __LINE__ for assert (or > maybe I should also switch mutex.h to use LOCK_FILE and LOCK_LINE at > some point?). I never saw the requirement for the backslash. I am consistent with PA_UNLOCK_COND() several lines above. Changed assert to use __FILE/LINE__. > > > +#else > > +#define vm_page_lock_assert(m, a) > > +#endif > > +#else /* KLD_MODULE */ > > This should be /* !KLD_MODULE */, I guess? Changed. > > > #define vm_page_lockptr(m) > > This is not defined for the KLD_MODULE case? Yes, explicitely. This was discussed. http://lists.freebsd.org/pipermail/freebsd-current/2011-November/029009.html diff --git a/sys/vm/vm_page.c b/sys/vm/vm_page.c index d592ac0..74e5126 100644 --- a/sys/vm/vm_page.c +++ b/sys/vm/vm_page.c @@ -2843,6 +2843,34 @@ vm_page_test_dirty(vm_page_t m) vm_page_dirty(m); } +void +vm_page_lock_KBI(vm_page_t m, const char *file, int line) +{ + + mtx_lock_flags_(vm_page_lockptr(m), 0, file, line); +} + +void +vm_page_unlock_KBI(vm_page_t m, const char *file, int line) +{ + + mtx_unlock_flags_(vm_page_lockptr(m), 0, file, line); +} + +int +vm_page_trylock_KBI(vm_page_t m, const char *file, int line) +{ + + return (mtx_trylock_flags_(vm_page_lockptr(m), 0, file, line)); +} + +void +vm_page_lock_assert_KBI(vm_page_t m, int a, const char *file, int line) +{ + + mtx_assert_(vm_page_lockptr(m), a, file, line); +} + int so_zerocp_fullpage = 0; /* diff --git a/sys/vm/vm_page.h b/sys/vm/vm_page.h index 151710d..1fab735 100644 --- a/sys/vm/vm_page.h +++ b/sys/vm/vm_page.h @@ -218,11 +218,23 @@ extern struct vpglocks pa_lock[]; #definePA_LOCK_ASSERT(pa, a) mtx_assert(PA_LOCKPTR(pa), (a)) +#ifdef KLD_MODULE +#definevm_page_lock(m) vm_page_lock_KBI((m), LOCK_FILE, LOCK_LINE) +#definevm_page_unlock(m) vm_page_unlock_KBI((m), LOCK_FILE, LOCK_LINE) +#definevm_page_trylock(m) vm_page_trylock_KBI((m), LOCK_FILE, LOCK_LINE) +#ifdef INVARIANTS +#definevm_page_lock_assert(m, a) \ +vm_page_lock_assert_KBI((m), (a), __FILE__, __LINE__) +#else +#definevm_page_lock_assert(m, a) +#endif +#else /* !KLD_MODULE */ #definevm_page_lockptr(m) (PA_LOCKPTR(VM_PAGE_TO_PHYS((m #definevm_page_lock(m) mtx_lock(vm_page_lockptr((m))) #definevm_page_unlock(m) mtx_unlock(vm_page_lockptr((m))) #definevm_page_trylock(m) mtx_trylock(vm_page_lockptr((m))) #definevm_page_lock_assert(m, a) mtx_assert(vm_page_lockptr((m)), (a)) +#endif #definevm_page_queue_free_mtx vm_page_queue_free_lock.data /* @@ -405,6 +417,11 @@ void vm_page_cowfault (vm_page_t); int vm_page_cowsetup(vm_page_t); void vm_page_cowclear (vm_page_t); +void vm_page_lock_KBI(vm_page_t m, const char *file, int line); +void vm_page_unlock_KBI(vm_page_t m, const char *file, int line); +int vm_page_trylock_KBI(vm_page_t m, const char *file, int line); +void vm_page_lock_assert_KBI(vm_page_t m, int a, const char *file, int line); + #ifdef INVARIANTS void vm_page_object_lock_assert(vm_page_t m); #defineVM_PAGE_OBJECT_LOCK_ASSERT(m) vm_page_object_lock_assert(m) pgpRqyzyLnGBT.pgp Description: PGP signature
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
2011/11/20 Kostik Belousov : > On Sun, Nov 20, 2011 at 05:37:33PM +0100, Attilio Rao wrote: >> 2011/11/18 Attilio Rao : >> > Please consider: >> > http://www.freebsd.org/~attilio/mutexfileline2.patch >> >> This is now committed as r227758,227759, you can update your patch now. > Here is it. > > diff --git a/sys/vm/vm_page.c b/sys/vm/vm_page.c > index d592ac0..74e5126 100644 > --- a/sys/vm/vm_page.c > +++ b/sys/vm/vm_page.c > @@ -2843,6 +2843,34 @@ vm_page_test_dirty(vm_page_t m) > vm_page_dirty(m); > } > > +void > +vm_page_lock_KBI(vm_page_t m, const char *file, int line) > +{ > + > + mtx_lock_flags_(vm_page_lockptr(m), 0, file, line); > +} > + > +void > +vm_page_unlock_KBI(vm_page_t m, const char *file, int line) > +{ > + > + mtx_unlock_flags_(vm_page_lockptr(m), 0, file, line); > +} > + > +int > +vm_page_trylock_KBI(vm_page_t m, const char *file, int line) > +{ > + > + return (mtx_trylock_flags_(vm_page_lockptr(m), 0, file, line)); > +} > + > +void > +vm_page_lock_assert_KBI(vm_page_t m, int a, const char *file, int line) > +{ > + > + mtx_assert_(vm_page_lockptr(m), a, file, line); > +} > + > int so_zerocp_fullpage = 0; > > /* > diff --git a/sys/vm/vm_page.h b/sys/vm/vm_page.h > index 151710d..fe0295b 100644 > --- a/sys/vm/vm_page.h > +++ b/sys/vm/vm_page.h > @@ -218,11 +218,23 @@ extern struct vpglocks pa_lock[]; > > #define PA_LOCK_ASSERT(pa, a) mtx_assert(PA_LOCKPTR(pa), (a)) > > +#ifdef KLD_MODULE > +#define vm_page_lock(m) vm_page_lock_KBI((m), LOCK_FILE, > LOCK_LINE) > +#define vm_page_unlock(m) vm_page_unlock_KBI((m), LOCK_FILE, > LOCK_LINE) > +#define vm_page_trylock(m) vm_page_trylock_KBI((m), LOCK_FILE, > LOCK_LINE) > +#ifdef INVARIANTS > +#define vm_page_lock_assert(m, a) \ > + vm_page_lock_assert_KBI((m), (a), LOCK_FILE, LOCK_LINE) I think you should put the "\" in the last tab and also, for consistency, you may want to use __FILE__ and __LINE__ for assert (or maybe I should also switch mutex.h to use LOCK_FILE and LOCK_LINE at some point?). > +#else > +#define vm_page_lock_assert(m, a) > +#endif > +#else /* KLD_MODULE */ This should be /* !KLD_MODULE */, I guess? > #define vm_page_lockptr(m) This is not defined for the KLD_MODULE case? Attilio -- Peace can only be achieved by understanding - A. Einstein ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
On Sun, Nov 20, 2011 at 05:37:33PM +0100, Attilio Rao wrote: > 2011/11/18 Attilio Rao : > > Please consider: > > http://www.freebsd.org/~attilio/mutexfileline2.patch > > This is now committed as r227758,227759, you can update your patch now. Here is it. diff --git a/sys/vm/vm_page.c b/sys/vm/vm_page.c index d592ac0..74e5126 100644 --- a/sys/vm/vm_page.c +++ b/sys/vm/vm_page.c @@ -2843,6 +2843,34 @@ vm_page_test_dirty(vm_page_t m) vm_page_dirty(m); } +void +vm_page_lock_KBI(vm_page_t m, const char *file, int line) +{ + + mtx_lock_flags_(vm_page_lockptr(m), 0, file, line); +} + +void +vm_page_unlock_KBI(vm_page_t m, const char *file, int line) +{ + + mtx_unlock_flags_(vm_page_lockptr(m), 0, file, line); +} + +int +vm_page_trylock_KBI(vm_page_t m, const char *file, int line) +{ + + return (mtx_trylock_flags_(vm_page_lockptr(m), 0, file, line)); +} + +void +vm_page_lock_assert_KBI(vm_page_t m, int a, const char *file, int line) +{ + + mtx_assert_(vm_page_lockptr(m), a, file, line); +} + int so_zerocp_fullpage = 0; /* diff --git a/sys/vm/vm_page.h b/sys/vm/vm_page.h index 151710d..fe0295b 100644 --- a/sys/vm/vm_page.h +++ b/sys/vm/vm_page.h @@ -218,11 +218,23 @@ extern struct vpglocks pa_lock[]; #definePA_LOCK_ASSERT(pa, a) mtx_assert(PA_LOCKPTR(pa), (a)) +#ifdef KLD_MODULE +#definevm_page_lock(m) vm_page_lock_KBI((m), LOCK_FILE, LOCK_LINE) +#definevm_page_unlock(m) vm_page_unlock_KBI((m), LOCK_FILE, LOCK_LINE) +#definevm_page_trylock(m) vm_page_trylock_KBI((m), LOCK_FILE, LOCK_LINE) +#ifdef INVARIANTS +#definevm_page_lock_assert(m, a) \ +vm_page_lock_assert_KBI((m), (a), LOCK_FILE, LOCK_LINE) +#else +#definevm_page_lock_assert(m, a) +#endif +#else /* KLD_MODULE */ #definevm_page_lockptr(m) (PA_LOCKPTR(VM_PAGE_TO_PHYS((m #definevm_page_lock(m) mtx_lock(vm_page_lockptr((m))) #definevm_page_unlock(m) mtx_unlock(vm_page_lockptr((m))) #definevm_page_trylock(m) mtx_trylock(vm_page_lockptr((m))) #definevm_page_lock_assert(m, a) mtx_assert(vm_page_lockptr((m)), (a)) +#endif #definevm_page_queue_free_mtx vm_page_queue_free_lock.data /* @@ -405,6 +417,11 @@ void vm_page_cowfault (vm_page_t); int vm_page_cowsetup(vm_page_t); void vm_page_cowclear (vm_page_t); +void vm_page_lock_KBI(vm_page_t m, const char *file, int line); +void vm_page_unlock_KBI(vm_page_t m, const char *file, int line); +int vm_page_trylock_KBI(vm_page_t m, const char *file, int line); +void vm_page_lock_assert_KBI(vm_page_t m, int a, const char *file, int line); + #ifdef INVARIANTS void vm_page_object_lock_assert(vm_page_t m); #defineVM_PAGE_OBJECT_LOCK_ASSERT(m) vm_page_object_lock_assert(m) pgpyNMDth73tZ.pgp Description: PGP signature
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
2011/11/18 Attilio Rao : > 2011/11/18 Attilio Rao : >> 2011/11/18 Kostik Belousov : >>> On Fri, Nov 18, 2011 at 11:40:28AM +0100, Attilio Rao wrote: 2011/11/16 Kostik Belousov : > On Tue, Nov 15, 2011 at 07:15:01PM +0100, Attilio Rao wrote: >> 2011/11/7 Kostik Belousov : >> > On Mon, Nov 07, 2011 at 11:45:38AM -0600, Alan Cox wrote: >> >> 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. >> > >> > Sure. Below is the extraction of only vm_page_lock() bits, together >> > with the suggested rename. When Attilio provides the promised >> > simplification >> > of the mutex KPI, this can be reduced. >> >> My tentative patch is here: >> http://www.freebsd.org/~attilio/mutexfileline.patch >> >> I need to make more compile testing later, but it already compiles >> GENERIC + modules fine on HEAD. >> >> The patch provides a common entrypoint, option independent, for both >> fast case and debug/compat case. >> Additively, it almost entirely fixes the standard violation of the >> reserved namespace, as you described (the notable exception being the >> macro used in the fast path, that I want to fix as well, but in a >> separate commit). >> >> Now the file/line couplet can be passed to the "_" suffix variant of >> the flag functions. > Yes, this is exactly KPI that I would use when available for the > vm_page_lock() patch. > >> >> eadler@ reviewed the mutex.h comment. >> >> Please let me know what you think about it, as long as we agree on the >> patch I'll commit it. > But I also agree with John that imposing large churn due to the > elimination > of the '__' prefix is too late now. At least it will make the change > non-MFCable. Besides, we already lived with the names for 10+ years. > > I will be happy to have the part of the patch that exports the > mtx_XXX_(mtx, > file, line) defines which can be used without taking care of LOCK_DEBUG > or MUTEX_NOINLINE in the consumer code. Ok, this patch should just add the compat stub: http://www.freebsd.org/~attilio/mutexfileline2.patch >>> Am I right that I would use mtx_lock_(mtx, file, line) etc ? >>> If yes, I am fine with it. >> >> Yes that is correct. >> >> However, I'm a bit confused on one aspect: would you mind using >> _mtx_lock_flags() instead? >> If you don't mind the "underscore namespace violation" I think I can >> make a much smaller patch against HEAD for it. >> >> Otherwise, the one now posted should be ok. > > After thinking more about it, I think that is basically the shorter > version I can came up with. > > Please consider: > http://www.freebsd.org/~attilio/mutexfileline2.patch This is now committed as r227758,227759, you can update your patch now. Attilio -- Peace can only be achieved by understanding - A. Einstein ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
2011/11/18 Attilio Rao : > 2011/11/18 Kostik Belousov : >> On Fri, Nov 18, 2011 at 11:40:28AM +0100, Attilio Rao wrote: >>> 2011/11/16 Kostik Belousov : >>> > On Tue, Nov 15, 2011 at 07:15:01PM +0100, Attilio Rao wrote: >>> >> 2011/11/7 Kostik Belousov : >>> >> > On Mon, Nov 07, 2011 at 11:45:38AM -0600, Alan Cox wrote: >>> >> >> 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. >>> >> > >>> >> > Sure. Below is the extraction of only vm_page_lock() bits, together >>> >> > with the suggested rename. When Attilio provides the promised >>> >> > simplification >>> >> > of the mutex KPI, this can be reduced. >>> >> >>> >> My tentative patch is here: >>> >> http://www.freebsd.org/~attilio/mutexfileline.patch >>> >> >>> >> I need to make more compile testing later, but it already compiles >>> >> GENERIC + modules fine on HEAD. >>> >> >>> >> The patch provides a common entrypoint, option independent, for both >>> >> fast case and debug/compat case. >>> >> Additively, it almost entirely fixes the standard violation of the >>> >> reserved namespace, as you described (the notable exception being the >>> >> macro used in the fast path, that I want to fix as well, but in a >>> >> separate commit). >>> >> >>> >> Now the file/line couplet can be passed to the "_" suffix variant of >>> >> the flag functions. >>> > Yes, this is exactly KPI that I would use when available for the >>> > vm_page_lock() patch. >>> > >>> >> >>> >> eadler@ reviewed the mutex.h comment. >>> >> >>> >> Please let me know what you think about it, as long as we agree on the >>> >> patch I'll commit it. >>> > But I also agree with John that imposing large churn due to the >>> > elimination >>> > of the '__' prefix is too late now. At least it will make the change >>> > non-MFCable. Besides, we already lived with the names for 10+ years. >>> > >>> > I will be happy to have the part of the patch that exports the >>> > mtx_XXX_(mtx, >>> > file, line) defines which can be used without taking care of LOCK_DEBUG >>> > or MUTEX_NOINLINE in the consumer code. >>> >>> Ok, this patch should just add the compat stub: >>> http://www.freebsd.org/~attilio/mutexfileline2.patch >> Am I right that I would use mtx_lock_(mtx, file, line) etc ? >> If yes, I am fine with it. > > Yes that is correct. > > However, I'm a bit confused on one aspect: would you mind using > _mtx_lock_flags() instead? > If you don't mind the "underscore namespace violation" I think I can > make a much smaller patch against HEAD for it. > > Otherwise, the one now posted should be ok. After thinking more about it, I think that is basically the shorter version I can came up with. Please consider: http://www.freebsd.org/~attilio/mutexfileline2.patch as a possible commit candidate for me. Attilio -- Peace can only be achieved by understanding - A. Einstein ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
On Fri, Nov 18, 2011 at 11:56:55AM +0100, Attilio Rao wrote: > 2011/11/18 Kostik Belousov : > > On Fri, Nov 18, 2011 at 11:40:28AM +0100, Attilio Rao wrote: > >> 2011/11/16 Kostik Belousov : > >> > On Tue, Nov 15, 2011 at 07:15:01PM +0100, Attilio Rao wrote: > >> >> 2011/11/7 Kostik Belousov : > >> >> > On Mon, Nov 07, 2011 at 11:45:38AM -0600, Alan Cox wrote: > >> >> >> 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. > >> >> > > >> >> > Sure. Below is the extraction of only vm_page_lock() bits, together > >> >> > with the suggested rename. When Attilio provides the promised > >> >> > simplification > >> >> > of the mutex KPI, this can be reduced. > >> >> > >> >> My tentative patch is here: > >> >> http://www.freebsd.org/~attilio/mutexfileline.patch > >> >> > >> >> I need to make more compile testing later, but it already compiles > >> >> GENERIC + modules fine on HEAD. > >> >> > >> >> The patch provides a common entrypoint, option independent, for both > >> >> fast case and debug/compat case. > >> >> Additively, it almost entirely fixes the standard violation of the > >> >> reserved namespace, as you described (the notable exception being the > >> >> macro used in the fast path, that I want to fix as well, but in a > >> >> separate commit). > >> >> > >> >> Now the file/line couplet can be passed to the "_" suffix variant of > >> >> the flag functions. > >> > Yes, this is exactly KPI that I would use when available for the > >> > vm_page_lock() patch. > >> > > >> >> > >> >> eadler@ reviewed the mutex.h comment. > >> >> > >> >> Please let me know what you think about it, as long as we agree on the > >> >> patch I'll commit it. > >> > But I also agree with John that imposing large churn due to the > >> > elimination > >> > of the '__' prefix is too late now. At least it will make the change > >> > non-MFCable. Besides, we already lived with the names for 10+ years. > >> > > >> > I will be happy to have the part of the patch that exports the > >> > mtx_XXX_(mtx, > >> > file, line) defines which can be used without taking care of LOCK_DEBUG > >> > or MUTEX_NOINLINE in the consumer code. > >> > >> Ok, this patch should just add the compat stub: > >> http://www.freebsd.org/~attilio/mutexfileline2.patch > > Am I right that I would use mtx_lock_(mtx, file, line) etc ? > > If yes, I am fine with it. > > Yes that is correct. > > However, I'm a bit confused on one aspect: would you mind using > _mtx_lock_flags() instead? > If you don't mind the "underscore namespace violation" I think I can > make a much smaller patch against HEAD for it. _mtx_lock_flags() is fine. The reserved names start with __ or _[A-Z]. > > Otherwise, the one now posted should be ok. > > Attilio > > > -- > Peace can only be achieved by understanding - A. Einstein pgpMbNbc9HZKI.pgp Description: PGP signature
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
2011/11/18 Kostik Belousov : > On Fri, Nov 18, 2011 at 11:40:28AM +0100, Attilio Rao wrote: >> 2011/11/16 Kostik Belousov : >> > On Tue, Nov 15, 2011 at 07:15:01PM +0100, Attilio Rao wrote: >> >> 2011/11/7 Kostik Belousov : >> >> > On Mon, Nov 07, 2011 at 11:45:38AM -0600, Alan Cox wrote: >> >> >> 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. >> >> > >> >> > Sure. Below is the extraction of only vm_page_lock() bits, together >> >> > with the suggested rename. When Attilio provides the promised >> >> > simplification >> >> > of the mutex KPI, this can be reduced. >> >> >> >> My tentative patch is here: >> >> http://www.freebsd.org/~attilio/mutexfileline.patch >> >> >> >> I need to make more compile testing later, but it already compiles >> >> GENERIC + modules fine on HEAD. >> >> >> >> The patch provides a common entrypoint, option independent, for both >> >> fast case and debug/compat case. >> >> Additively, it almost entirely fixes the standard violation of the >> >> reserved namespace, as you described (the notable exception being the >> >> macro used in the fast path, that I want to fix as well, but in a >> >> separate commit). >> >> >> >> Now the file/line couplet can be passed to the "_" suffix variant of >> >> the flag functions. >> > Yes, this is exactly KPI that I would use when available for the >> > vm_page_lock() patch. >> > >> >> >> >> eadler@ reviewed the mutex.h comment. >> >> >> >> Please let me know what you think about it, as long as we agree on the >> >> patch I'll commit it. >> > But I also agree with John that imposing large churn due to the elimination >> > of the '__' prefix is too late now. At least it will make the change >> > non-MFCable. Besides, we already lived with the names for 10+ years. >> > >> > I will be happy to have the part of the patch that exports the >> > mtx_XXX_(mtx, >> > file, line) defines which can be used without taking care of LOCK_DEBUG >> > or MUTEX_NOINLINE in the consumer code. >> >> Ok, this patch should just add the compat stub: >> http://www.freebsd.org/~attilio/mutexfileline2.patch > Am I right that I would use mtx_lock_(mtx, file, line) etc ? > If yes, I am fine with it. Yes that is correct. However, I'm a bit confused on one aspect: would you mind using _mtx_lock_flags() instead? If you don't mind the "underscore namespace violation" I think I can make a much smaller patch against HEAD for it. Otherwise, the one now posted should be ok. Attilio -- Peace can only be achieved by understanding - A. Einstein ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
On Fri, Nov 18, 2011 at 11:40:28AM +0100, Attilio Rao wrote: > 2011/11/16 Kostik Belousov : > > On Tue, Nov 15, 2011 at 07:15:01PM +0100, Attilio Rao wrote: > >> 2011/11/7 Kostik Belousov : > >> > On Mon, Nov 07, 2011 at 11:45:38AM -0600, Alan Cox wrote: > >> >> 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. > >> > > >> > Sure. Below is the extraction of only vm_page_lock() bits, together > >> > with the suggested rename. When Attilio provides the promised > >> > simplification > >> > of the mutex KPI, this can be reduced. > >> > >> My tentative patch is here: > >> http://www.freebsd.org/~attilio/mutexfileline.patch > >> > >> I need to make more compile testing later, but it already compiles > >> GENERIC + modules fine on HEAD. > >> > >> The patch provides a common entrypoint, option independent, for both > >> fast case and debug/compat case. > >> Additively, it almost entirely fixes the standard violation of the > >> reserved namespace, as you described (the notable exception being the > >> macro used in the fast path, that I want to fix as well, but in a > >> separate commit). > >> > >> Now the file/line couplet can be passed to the "_" suffix variant of > >> the flag functions. > > Yes, this is exactly KPI that I would use when available for the > > vm_page_lock() patch. > > > >> > >> eadler@ reviewed the mutex.h comment. > >> > >> Please let me know what you think about it, as long as we agree on the > >> patch I'll commit it. > > But I also agree with John that imposing large churn due to the elimination > > of the '__' prefix is too late now. At least it will make the change > > non-MFCable. Besides, we already lived with the names for 10+ years. > > > > I will be happy to have the part of the patch that exports the mtx_XXX_(mtx, > > file, line) defines which can be used without taking care of LOCK_DEBUG > > or MUTEX_NOINLINE in the consumer code. > > Ok, this patch should just add the compat stub: > http://www.freebsd.org/~attilio/mutexfileline2.patch Am I right that I would use mtx_lock_(mtx, file, line) etc ? If yes, I am fine with it. > > I'll make more test-compiling later in the day, if you agree on it I > will commit the patch tomorrow. > > Attilio > > > -- > Peace can only be achieved by understanding - A. Einstein pgp0D7fXhQgea.pgp Description: PGP signature
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
2011/11/16 Kostik Belousov : > On Tue, Nov 15, 2011 at 07:15:01PM +0100, Attilio Rao wrote: >> 2011/11/7 Kostik Belousov : >> > On Mon, Nov 07, 2011 at 11:45:38AM -0600, Alan Cox wrote: >> >> 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. >> > >> > Sure. Below is the extraction of only vm_page_lock() bits, together >> > with the suggested rename. When Attilio provides the promised >> > simplification >> > of the mutex KPI, this can be reduced. >> >> My tentative patch is here: >> http://www.freebsd.org/~attilio/mutexfileline.patch >> >> I need to make more compile testing later, but it already compiles >> GENERIC + modules fine on HEAD. >> >> The patch provides a common entrypoint, option independent, for both >> fast case and debug/compat case. >> Additively, it almost entirely fixes the standard violation of the >> reserved namespace, as you described (the notable exception being the >> macro used in the fast path, that I want to fix as well, but in a >> separate commit). >> >> Now the file/line couplet can be passed to the "_" suffix variant of >> the flag functions. > Yes, this is exactly KPI that I would use when available for the > vm_page_lock() patch. > >> >> eadler@ reviewed the mutex.h comment. >> >> Please let me know what you think about it, as long as we agree on the >> patch I'll commit it. > But I also agree with John that imposing large churn due to the elimination > of the '__' prefix is too late now. At least it will make the change > non-MFCable. Besides, we already lived with the names for 10+ years. > > I will be happy to have the part of the patch that exports the mtx_XXX_(mtx, > file, line) defines which can be used without taking care of LOCK_DEBUG > or MUTEX_NOINLINE in the consumer code. Ok, this patch should just add the compat stub: http://www.freebsd.org/~attilio/mutexfileline2.patch I'll make more test-compiling later in the day, if you agree on it I will commit the patch tomorrow. Attilio -- Peace can only be achieved by understanding - A. Einstein ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
On Tue, Nov 15, 2011 at 07:15:01PM +0100, Attilio Rao wrote: > 2011/11/7 Kostik Belousov : > > On Mon, Nov 07, 2011 at 11:45:38AM -0600, Alan Cox wrote: > >> 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. > > > > Sure. Below is the extraction of only vm_page_lock() bits, together > > with the suggested rename. When Attilio provides the promised simplification > > of the mutex KPI, this can be reduced. > > My tentative patch is here: > http://www.freebsd.org/~attilio/mutexfileline.patch > > I need to make more compile testing later, but it already compiles > GENERIC + modules fine on HEAD. > > The patch provides a common entrypoint, option independent, for both > fast case and debug/compat case. > Additively, it almost entirely fixes the standard violation of the > reserved namespace, as you described (the notable exception being the > macro used in the fast path, that I want to fix as well, but in a > separate commit). > > Now the file/line couplet can be passed to the "_" suffix variant of > the flag functions. Yes, this is exactly KPI that I would use when available for the vm_page_lock() patch. > > eadler@ reviewed the mutex.h comment. > > Please let me know what you think about it, as long as we agree on the > patch I'll commit it. But I also agree with John that imposing large churn due to the elimination of the '__' prefix is too late now. At least it will make the change non-MFCable. Besides, we already lived with the names for 10+ years. I will be happy to have the part of the patch that exports the mtx_XXX_(mtx, file, line) defines which can be used without taking care of LOCK_DEBUG or MUTEX_NOINLINE in the consumer code. pgpOwIEH4H9gF.pgp Description: PGP signature
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
On Sunday, November 06, 2011 11:42:04 am Kostik Belousov wrote: > On Sun, Nov 06, 2011 at 07:22:51AM -0800, m...@freebsd.org wrote: > > On Sun, Nov 6, 2011 at 4:43 AM, Kostik Belousov wrote: > > > 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. > > > > I'm pretty sure that when the C standard says, "the implementation", > > they're referring to the compiler and OS it runs on. Which makes the > > FreeBSD kernel part of "the implementation", which is precisely why so > > many headers have defines that start with __ and then, if certain > > posix defines are set, also uses non-__ versions of the name. > > For libc providing parts, required by standard, you are right. > But our kernel is a freestanding program using a compiler, so in-kernel > uses of the reserved namespace is a violation. I don't buy that argument at all. We have a libc for the kernel, it's called libkern and we own that, too. We depend on using _ and __ prefixes all over the kernel and trying to change that now would be excessively gratuitous. -- John Baldwin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
2011/11/15 : > On Tue, Nov 15, 2011 at 10:15 AM, Attilio Rao wrote: >> 2011/11/7 Kostik Belousov : >>> On Mon, Nov 07, 2011 at 11:45:38AM -0600, Alan Cox wrote: 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. >>> >>> Sure. Below is the extraction of only vm_page_lock() bits, together >>> with the suggested rename. When Attilio provides the promised simplification >>> of the mutex KPI, this can be reduced. >> >> My tentative patch is here: >> http://www.freebsd.org/~attilio/mutexfileline.patch >> >> I need to make more compile testing later, but it already compiles >> GENERIC + modules fine on HEAD. >> >> The patch provides a common entrypoint, option independent, for both >> fast case and debug/compat case. >> Additively, it almost entirely fixes the standard violation of the >> reserved namespace, as you described (the notable exception being the >> macro used in the fast path, that I want to fix as well, but in a >> separate commit). >> >> Now the file/line couplet can be passed to the "_" suffix variant of >> the flag functions. >> >> eadler@ reviewed the mutex.h comment. >> >> Please let me know what you think about it, as long as we agree on the >> patch I'll commit it. > > Out of curiosity, why are function names explicitly spelled out in > panic and log messages, instead of using %s and __func__? I've seen > this all around FreeBSD, and if there's no reason otherwise, I'd just > as soon change to a version that doesn't need updating when the > function names change. I prefer the __func__ stuff as well but bde isn't in favor of it because it is more difficult to grep for the message in that case. I'm not sure I'd buy his point on this, honestly, but that is why. Attilio -- Peace can only be achieved by understanding - A. Einstein ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
On Tue, Nov 15, 2011 at 10:15 AM, Attilio Rao wrote: > 2011/11/7 Kostik Belousov : >> On Mon, Nov 07, 2011 at 11:45:38AM -0600, Alan Cox wrote: >>> 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. >> >> Sure. Below is the extraction of only vm_page_lock() bits, together >> with the suggested rename. When Attilio provides the promised simplification >> of the mutex KPI, this can be reduced. > > My tentative patch is here: > http://www.freebsd.org/~attilio/mutexfileline.patch > > I need to make more compile testing later, but it already compiles > GENERIC + modules fine on HEAD. > > The patch provides a common entrypoint, option independent, for both > fast case and debug/compat case. > Additively, it almost entirely fixes the standard violation of the > reserved namespace, as you described (the notable exception being the > macro used in the fast path, that I want to fix as well, but in a > separate commit). > > Now the file/line couplet can be passed to the "_" suffix variant of > the flag functions. > > eadler@ reviewed the mutex.h comment. > > Please let me know what you think about it, as long as we agree on the > patch I'll commit it. Out of curiosity, why are function names explicitly spelled out in panic and log messages, instead of using %s and __func__? I've seen this all around FreeBSD, and if there's no reason otherwise, I'd just as soon change to a version that doesn't need updating when the function names change. Thanks, matthew ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
2011/11/7 Kostik Belousov : > On Mon, Nov 07, 2011 at 11:45:38AM -0600, Alan Cox wrote: >> 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. > > Sure. Below is the extraction of only vm_page_lock() bits, together > with the suggested rename. When Attilio provides the promised simplification > of the mutex KPI, this can be reduced. My tentative patch is here: http://www.freebsd.org/~attilio/mutexfileline.patch I need to make more compile testing later, but it already compiles GENERIC + modules fine on HEAD. The patch provides a common entrypoint, option independent, for both fast case and debug/compat case. Additively, it almost entirely fixes the standard violation of the reserved namespace, as you described (the notable exception being the macro used in the fast path, that I want to fix as well, but in a separate commit). Now the file/line couplet can be passed to the "_" suffix variant of the flag functions. eadler@ reviewed the mutex.h comment. Please let me know what you think about it, as long as we agree on the patch I'll commit it. Attilio -- Peace can only be achieved by understanding - A. Einstein ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Using Instruction Pointer address in debug interfaces [Was: Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]]
on 14/11/2011 02:38 Arnaud Lacombe said the following: > you (committers) I wonder how it would work out if you were made a committer and couldn't say "you (committers)" any more... :-) I.e. is it possible to change your mindset from "me (and us) versus you" to just "us"? The lines between committers and contributors and users are very blurry and are mostly in people's heads rather than in reality. -- Andriy Gapon ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Using Instruction Pointer address in debug interfaces [Was: Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]]
Hi, On Wed, Nov 9, 2011 at 12:39 PM, Julian Elischer wrote: > On 11/8/11 9:29 PM, Arnaud Lacombe wrote: > [...] > >> However, if you want to know, my heart tends to be with BSDs. >> Unfortunately, it's a sad love-story where your Beloved keeps >> deceiving you day after day. You want to change small bits at a time, >> make several iteration of progress to make things brighter, but your >> Beloved refuses any change because of too much inertia. Sad. > > mostly it's because you keep attacking your loved one with a steak knife. > flowers might get you further. Well, it would would seem that keeping sending patch is what you consider "attacking your loved one with a steak knife", because yes, this is what I will keep doing. >>> >>> so you are used to doing it that way.. but don't expect us to change just >>> because that's what Linux does. >>> >> again, mentioning Linux is totally irrelevant. Use of Instruction >> Pointer are implementation details for a not so intrusive solution to >> the problem I pointed out, and which you are totally missing. > > since these modules were written many new options have come. Maybe this is the real problem, FreeBSD is unable to keep up and to make interfaces written +10 years ago evolves. Worst, you (committers) keep on taking decision based on changes made 10 to 12 years ago that are totally irrelevant today, making these decisions nothing but plain bad. >> well, you're lucky FreeBSD supports your device! Lately, we got lately >> a shiny multi-queue network cards with bypass mechanism... that is not >> supported in FreeBSD. So currently, we got an expensive paper-weight. > > well write a driver for it.. > my time is limited, and you (FreeBSD folks) seem to love making it even busier by your inability to make some parts of the system evolve, or by taking bad decision. This generally happens when I try to optimize some of our internal code path, hit a system bottleneck, try to prove the system is wrong, and then eventually, think about optimizing it, implement the solution one or twice, and get slammed hard when I go public with both the proof of performance hit/non-correctness/incompleteness and the correction. Unfortunately, the time complexity of the whole process is 2^O(n) :( > what do you think I'm doing with the driver I'm > talking about? > I wrote several bypass network card drivers when I was at cisco/ironport.. > it's not rocket science, > though it would be nice if we were to come up with a standard interface for > bypass interfaces. indeed. > That is a different topic though.. > indeed. >>> 1/ half the time freebsd will just immediatly assert on something and >>> present you with the bug.. done. >>> >> well, certainly not from a release build; assertion are disabled. > > During development. we NEVER have bugs in production ;-) > [sic.] >> [0]: I am able to crash any kernel between 7-STABLE to 9.STABLE within >> minutes, with the right pattern and (mainstream and well supported) >> hardware. > > and who have you reported that to? bug number? > http://lists.freebsd.org/pipermail/freebsd-net/2011-September/029722.html I suspect PR/155597 and a few other are related. >> [3]: FreeBSD (8-STABLE) is way to limited and un-integrated to be >> anywhere but useful, not to speak about kernel bug which leave the >> system so fracked up that you have no other choice but to hard-reboot. > > bug number? > usb/156725, amd64/156726 - Arnaud ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Using Instruction Pointer address in debug interfaces [Was: Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]]
On 10/11/2011, at 4:09, Julian Elischer wrote: > well write a driver for it.. what do you think I'm doing with the driver I'm > talking about? > I wrote several bypass network card drivers when I was at cisco/ironport.. > it's not rocket science, > though it would be nice if we were to come up with a standard interface for > bypass interfaces. > That is a different topic though.. http://info.iet.unipi.it/~luigi/netmap/ Perhaps? -- Daniel O'Connor software and network engineer for Genesis Software - http://www.gsoft.com.au "The nice thing about standards is that there are so many of them to choose from." -- Andrew Tanenbaum GPG Fingerprint - 5596 B766 97C0 0E94 4347 295E E593 DC20 7B3F CE8C ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Using Instruction Pointer address in debug interfaces [Was: Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]]
On 11/8/11 9:29 PM, Arnaud Lacombe wrote: Hi, On Tue, Nov 8, 2011 at 9:35 PM, Julian Elischer wrote: On 11/8/11 5:52 PM, Arnaud Lacombe wrote: Hi, On Tue, Nov 8, 2011 at 7:08 PM, Julian Elischer wrote: On 11/8/11 10:49 AM, Arnaud Lacombe wrote: Hi, To avoid future complaints about the fact that I would be only "talk" without "action", I did implement what I suggested above. As it is quite a large patch-set, I will not post it directly here, however, it is available on github: https://github.com/lacombar/freebsd/tree/master/topic/kern-lock-debug It convert a bunch of debug interface to use the caller instruction pointer, as well as a proof-of-concept teaching printf(9) to convert IP to symbol_name+offset. It translates in a direct saving of about +250kB on i386's GENERIC, just in kernel text size. Even the worst case, ie LOCK_DEBUG == 0, translates to a save of +80kB. Please note that this is still WIP code. A couple of comments. Firstly, the idea of a printf method to print the IP as symbol+offset is an interesting idea that should be followed up in its own right. FWIW, I have no credit in this idea. It has been in Linux for ages and ages. yeah as I said at work I use linux and BSD... the linux stuff that just prints out IP really annoys me. the list stuff and netgraph debug (which should be off in any production system) this is, I guess, where we do not agree. You find it acceptable to run totally different code in production and during debug. I do not. This is completely insane, even more nowadays where heavy parallelism increases the likelihood of races, and subtle change in the code, even optimization, can cause total behavioral change (ie. Heisenbug). For the record, we have been tracking for more than 2 months (first occurrences happened a year ago) an mbuf corruption in the network stack, present in all released code since at least FreeBSD 7[0]. Each time we think it is fixed, we are proven wrong by customers within a few days when the system crashes again. Even the last attempt which was believed to be bullet-proof failed and crashes daily. All that to say that production code should embed enough facilities to allow the system to be fully debugged with a runtime cost as low as possible, and a code-size cost as low as possible[1]. I should be able to connect on a production machine, turn a knob, an see what is going wrong, without the customer noticing. In the worst case, when you have to enable debug-only code, it must not be done by making the non-debug case more expensive, but wrap around. The whole original point of the patches was that LOCK_FILE and LOCK_LINE are a bad answer to a wrong problem. `__FILE__, __LINE__' and the bloat introduced is not the problem, `const char *file, int line' in way too much prototypes is. in netgraph if you turn off debugging you should not have any char * file stuff. it should all go away. ( he says from memory, not actually looking) Now, you make me realize that `const char *file, int line' should just be removed, not replaced by `unsigned long' or anything else. It's likely to be done in another iteration. just require you to be able to see the console. and have sources nearby. if you need the IP use gdb. "console debugging" is yet another abomination which should be hunted down. Just try to do any useful work at high-pps on a serial console... if you want to use gdb, then use gdb and if you want to use ktr, then use that. don't just declare the rest of the universe incompetent. these things are usually there for the developer of the module and done in whatever way is most convenient fo rthem. it's just what you are used to. You are obviously from the dark side ^H^H^H^H^H^H linux. My obedience is totally irrelevant to the problem. However, if you want to know, my heart tends to be with BSDs. Unfortunately, it's a sad love-story where your Beloved keeps deceiving you day after day. You want to change small bits at a time, make several iteration of progress to make things brighter, but your Beloved refuses any change because of too much inertia. Sad. mostly it's because you keep attacking your loved one with a steak knife. flowers might get you further. so you are used to doing it that way.. but don't expect us to change just because that's what Linux does. again, mentioning Linux is totally irrelevant. Use of Instruction Pointer are implementation details for a not so intrusive solution to the problem I pointed out, and which you are totally missing. since these modules were written many new options have come. for example the option to throw out a stack backtrace is new. For netgraph however, when I was debugging it, a file/line was exactly what I needed for the type of error I was looking for at the time. Now, please answer this: do you find any of the bloat to the non-debug case (ie. passing a NULL pointer and a 0 integer, when `LOCK_DEBUG == 0') worth the extra debugability comfort to be acceptable
Re: Using Instruction Pointer address in debug interfaces [Was: Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]]
On 11/9/11, Arnaud Lacombe wrote: > Hi, > > On Tue, Nov 8, 2011 at 9:35 PM, Julian Elischer wrote: >> On 11/8/11 5:52 PM, Arnaud Lacombe wrote: >>> >>> Hi, >>> >>> On Tue, Nov 8, 2011 at 7:08 PM, Julian Elischer >>> wrote: On 11/8/11 10:49 AM, Arnaud Lacombe wrote: > > Hi, > To avoid future complaints about the fact that I would be only "talk" > without "action", I did implement what I suggested above. As it is > quite a large patch-set, I will not post it directly here, however, it > is available on github: > > https://github.com/lacombar/freebsd/tree/master/topic/kern-lock-debug > > It convert a bunch of debug interface to use the caller instruction > pointer, as well as a proof-of-concept teaching printf(9) to convert > IP to symbol_name+offset. > > It translates in a direct saving of about +250kB on i386's GENERIC, > just in kernel text size. Even the worst case, ie LOCK_DEBUG == 0, > translates to a save of +80kB. > > Please note that this is still WIP code. A couple of comments. Firstly, the idea of a printf method to print the IP as symbol+offset is an interesting idea that should be followed up in its own right. >>> FWIW, I have no credit in this idea. It has been in Linux for ages and >>> ages. >> >> yeah as I said at work I use linux and BSD... >> the linux stuff that just prints out IP really annoys me. >> >> the list stuff and netgraph debug (which should be off in any production >> system) > this is, I guess, where we do not agree. You find it acceptable to run > totally different code in production and during debug. I do not. This > is completely insane, even more nowadays where heavy parallelism > increases the likelihood of races, and subtle change in the code, even > optimization, can cause total behavioral change (ie. Heisenbug). > > For the record, we have been tracking for more than 2 months (first > occurrences happened a year ago) an mbuf corruption in the network > stack, present in all released code since at least FreeBSD 7[0]. Each > time we think it is fixed, we are proven wrong by customers within a > few days when the system crashes again. Even the last attempt which > was believed to be bullet-proof failed and crashes daily. > > All that to say that production code should embed enough facilities to > allow the system to be fully debugged with a runtime cost as low as > possible, and a code-size cost as low as possible[1]. I should be able > to connect on a production machine, turn a knob, an see what is going > wrong, without the customer noticing. > > In the worst case, when you have to enable debug-only code, it must > not be done by making the non-debug case more expensive, but wrap > around. The whole original point of the patches was that LOCK_FILE and > LOCK_LINE are a bad answer to a wrong problem. > > `__FILE__, __LINE__' and the bloat introduced is not the problem, > `const char *file, int line' in way too much prototypes is. > > Now, you make me realize that `const char *file, int line' should just > be removed, not replaced by `unsigned long' or anything else. It's > likely to be done in another iteration. > >> just require you to be able to see the console. and have sources nearby. >> if you need the IP use gdb. >> > "console debugging" is yet another abomination which should be hunted > down. Just try to do any useful work at high-pps on a serial > console... > >> it's just what you are used to. You are obviously from the dark side >> ^H^H^H^H^H^H linux. >> > My obedience is totally irrelevant to the problem. > > However, if you want to know, my heart tends to be with BSDs. > Unfortunately, it's a sad love-story where your Beloved keeps > deceiving you day after day. You want to change small bits at a time, > make several iteration of progress to make things brighter, but your > Beloved refuses any change because of too much inertia. Sad. > >> so you are used to doing it that way.. but don't expect us to change just >> because that's what Linux does. >> > again, mentioning Linux is totally irrelevant. Use of Instruction > Pointer are implementation details for a not so intrusive solution to > the problem I pointed out, and which you are totally missing. > > Now, please answer this: do you find any of the bloat to the non-debug > case (ie. passing a NULL pointer and a 0 integer, when `LOCK_DEBUG == > 0') worth the extra debugability comfort to be acceptable ? > > If you do, then your focus is on making things comfortable for > developers, at the expense 100's of users, rather than making things > comfortable for 100's of users, at the expense of developers. > >> When we have a problem at work on teh Linux driver, my first step is >> always >> to try duplicate it on FreeBSD because: >> > well, you're lucky FreeBSD supports your device! Lately, we got lately > a shiny multi-queue network cards with bypass mechanism... that is not > supported in Free
Re: Using Instruction Pointer address in debug interfaces [Was: Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]]
Hi, On Tue, Nov 8, 2011 at 9:35 PM, Julian Elischer wrote: > On 11/8/11 5:52 PM, Arnaud Lacombe wrote: >> >> Hi, >> >> On Tue, Nov 8, 2011 at 7:08 PM, Julian Elischer >> wrote: >>> >>> On 11/8/11 10:49 AM, Arnaud Lacombe wrote: Hi, To avoid future complaints about the fact that I would be only "talk" without "action", I did implement what I suggested above. As it is quite a large patch-set, I will not post it directly here, however, it is available on github: https://github.com/lacombar/freebsd/tree/master/topic/kern-lock-debug It convert a bunch of debug interface to use the caller instruction pointer, as well as a proof-of-concept teaching printf(9) to convert IP to symbol_name+offset. It translates in a direct saving of about +250kB on i386's GENERIC, just in kernel text size. Even the worst case, ie LOCK_DEBUG == 0, translates to a save of +80kB. Please note that this is still WIP code. >>> >>> A couple of comments. >>> Firstly, the idea of a printf method to print the IP as symbol+offset is >>> an >>> interesting idea >>> that should be followed up in its own right. >>> >> FWIW, I have no credit in this idea. It has been in Linux for ages and >> ages. > > yeah as I said at work I use linux and BSD... > the linux stuff that just prints out IP really annoys me. > > the list stuff and netgraph debug (which should be off in any production > system) this is, I guess, where we do not agree. You find it acceptable to run totally different code in production and during debug. I do not. This is completely insane, even more nowadays where heavy parallelism increases the likelihood of races, and subtle change in the code, even optimization, can cause total behavioral change (ie. Heisenbug). For the record, we have been tracking for more than 2 months (first occurrences happened a year ago) an mbuf corruption in the network stack, present in all released code since at least FreeBSD 7[0]. Each time we think it is fixed, we are proven wrong by customers within a few days when the system crashes again. Even the last attempt which was believed to be bullet-proof failed and crashes daily. All that to say that production code should embed enough facilities to allow the system to be fully debugged with a runtime cost as low as possible, and a code-size cost as low as possible[1]. I should be able to connect on a production machine, turn a knob, an see what is going wrong, without the customer noticing. In the worst case, when you have to enable debug-only code, it must not be done by making the non-debug case more expensive, but wrap around. The whole original point of the patches was that LOCK_FILE and LOCK_LINE are a bad answer to a wrong problem. `__FILE__, __LINE__' and the bloat introduced is not the problem, `const char *file, int line' in way too much prototypes is. Now, you make me realize that `const char *file, int line' should just be removed, not replaced by `unsigned long' or anything else. It's likely to be done in another iteration. > just require you to be able to see the console. and have sources nearby. > if you need the IP use gdb. > "console debugging" is yet another abomination which should be hunted down. Just try to do any useful work at high-pps on a serial console... > it's just what you are used to. You are obviously from the dark side > ^H^H^H^H^H^H linux. > My obedience is totally irrelevant to the problem. However, if you want to know, my heart tends to be with BSDs. Unfortunately, it's a sad love-story where your Beloved keeps deceiving you day after day. You want to change small bits at a time, make several iteration of progress to make things brighter, but your Beloved refuses any change because of too much inertia. Sad. > so you are used to doing it that way.. but don't expect us to change just > because that's what Linux does. > again, mentioning Linux is totally irrelevant. Use of Instruction Pointer are implementation details for a not so intrusive solution to the problem I pointed out, and which you are totally missing. Now, please answer this: do you find any of the bloat to the non-debug case (ie. passing a NULL pointer and a 0 integer, when `LOCK_DEBUG == 0') worth the extra debugability comfort to be acceptable ? If you do, then your focus is on making things comfortable for developers, at the expense 100's of users, rather than making things comfortable for 100's of users, at the expense of developers. > When we have a problem at work on teh Linux driver, my first step is always > to try duplicate it on FreeBSD because: > well, you're lucky FreeBSD supports your device! Lately, we got lately a shiny multi-queue network cards with bypass mechanism... that is not supported in FreeBSD. So currently, we got an expensive paper-weight. > 1/ half the time freebsd will just immediatly assert on something and > present you with the bug.. done. > well, certainly not from a release bu
Re: Using Instruction Pointer address in debug interfaces [Was: Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]]
On 11/8/11 5:52 PM, Arnaud Lacombe wrote: Hi, On Tue, Nov 8, 2011 at 7:08 PM, Julian Elischer wrote: On 11/8/11 10:49 AM, Arnaud Lacombe wrote: Hi, To avoid future complaints about the fact that I would be only "talk" without "action", I did implement what I suggested above. As it is quite a large patch-set, I will not post it directly here, however, it is available on github: https://github.com/lacombar/freebsd/tree/master/topic/kern-lock-debug It convert a bunch of debug interface to use the caller instruction pointer, as well as a proof-of-concept teaching printf(9) to convert IP to symbol_name+offset. It translates in a direct saving of about +250kB on i386's GENERIC, just in kernel text size. Even the worst case, ie LOCK_DEBUG == 0, translates to a save of +80kB. Please note that this is still WIP code. A couple of comments. Firstly, the idea of a printf method to print the IP as symbol+offset is an interesting idea that should be followed up in its own right. FWIW, I have no credit in this idea. It has been in Linux for ages and ages. yeah as I said at work I use linux and BSD... the linux stuff that just prints out IP really annoys me. the list stuff and netgraph debug (which should be off in any production system) just require you to be able to see the console. and have sources nearby. if you need the IP use gdb. it's just what you are used to. You are obviously from the dark side ^H^H^H^H^H^H linux. so you are used to doing it that way.. but don't expect us to change just because that's what Linux does. When we have a problem at work on teh Linux driver, my first step is always to try duplicate it on FreeBSD because: 1/ half the time freebsd will just immediatly assert on something and present you with the bug.. done. 2/ I can run gdb through firewire on it on ANY standard unmodified kernel and find it, where on Linux I need to get a whole universe of stupid patches all aligned and MAYBE I might be able to see what is going on. if it's on redhat I need to do this, on ubuntu that, on suse something else ,and on different revisions of the kernel it all changes anyhow.. That said, IP address are barely used in FreeBSD, there is no legacy. As such, the API should not use `unsigned long' but `void *'[0]; this is the natural type returned by `__builtin_return_address()' and the `&&' operator. This would allow to introduce a modifier to `%p' to do the translation. possibly intptr_t is what should be used. but I'd expect Bruce to drop in here and let us us know. - Arnaud ps: netgraph is on my target list, as well as the list code, to some extend :) well let me know what you want to do because while it can do with love it is also used by a lot of people. if you nean to remove file/line.. don't. [0]: as I really hate `caddr_t' it's probably older than you are.. times change. void* wasn't used much back then. ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Using Instruction Pointer address in debug interfaces [Was: Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]]
On 11/8/11 5:22 PM, Arnaud Lacombe wrote: Hi, On Tue, Nov 8, 2011 at 3:34 PM, Attilio Rao wrote: 2011/11/8 Arnaud Lacombe: Hi, On Mon, Nov 7, 2011 at 2:03 PM, Arnaud Lacombe wrote: On Mon, Nov 7, 2011 at 4:36 AM, Attilio Rao wrote: 2011/11/7 Arnaud Lacombe: Hi, On Sat, Nov 5, 2011 at 10:13 AM, Kostik Belousov 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. 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) * can only occur at the file EOF. */ VM_OBJECT_LOCK(object); - if (pages[ap->a_reqpage]->valid != 0) { + if (vm_page_read_valid(pages[ap->a_reqpage]) != 0) { for (i = 0; i< npages; ++i) { if (i != ap->a_reqpage) { vm_page_lock(pages[i]); @@ -198,16 +198,16 @@ nfs_getpages(struct vop_getpages_args *ap) /* * Read operation filled an entire page */ - m->valid = VM_PAGE_BITS_ALL; - KASSERT(m->dirty == 0, + vm_page_write_valid(m, VM_PAGE_BITS_ALL); + KASSERT(vm_page_read_dirty(m) == 0, ("nfs_getpages: page %p is dirty", m)); } else if (size> toff) { /* * Read operation filled a partial page. */ - m->valid = 0; + vm_page_write_valid(m, 0); vm_page_set_valid(m, 0, size - toff); - KASSERT(m->dirty == 0, + KASSERT(vm_page_read_dirty(m) == 0, ("nfs_getpages: page %p is dirty", m)); } else { /* 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) vm_page_dirty(m); } +void +vm_page_lock_func(vm_page_t m, const char *file, int line) +{ + +#if LOCK_DEBUG> 0 || defined(MUTEX_NOINLINE) + _mtx_lock_flags(vm_page_lockptr(m), 0, file, line); +#else + __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. Well, unfortunately, I wasn't around to comment 10 years ago when this got in. 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). Yes, and that's not an excuse to use the _internal_ implementation details of mutex(9), and not the exposed API. This is especially true when applied to someone so eager to follow "standards". 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. You comment just as if __FILE__ and __LINE__ were mandatory in a debug interface. This is _not_ true. __FILE__ and __LINE__ are just hideous for debugging of anything but early alpha code. LOCK_FILE and LOCK_LINE are a bad answer to a bad interface. Even if you just pass NULL and 0 as argument, you've got the bloat of passing unused argument. You might as well just pass a single argument that would do the exact same job and be _always_ available, eg. the IP of the caller, or the first return address. KDB magic still let you translate to something humanly understandable. If the toolchain does not support the feature on all supported platform, well, fix the toolchain. To avoid future complaints about the fact that I would be only "talk" without "action", I did implement what I suggested above. As it is quite a large patch-set, I will not post it directly here, however, it is available
Re: Using Instruction Pointer address in debug interfaces [Was: Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]]
Hi, On Tue, Nov 8, 2011 at 7:08 PM, Julian Elischer wrote: > On 11/8/11 10:49 AM, Arnaud Lacombe wrote: >> >> Hi, >> To avoid future complaints about the fact that I would be only "talk" >> without "action", I did implement what I suggested above. As it is >> quite a large patch-set, I will not post it directly here, however, it >> is available on github: >> >> https://github.com/lacombar/freebsd/tree/master/topic/kern-lock-debug >> >> It convert a bunch of debug interface to use the caller instruction >> pointer, as well as a proof-of-concept teaching printf(9) to convert >> IP to symbol_name+offset. >> >> It translates in a direct saving of about +250kB on i386's GENERIC, >> just in kernel text size. Even the worst case, ie LOCK_DEBUG == 0, >> translates to a save of +80kB. >> >> Please note that this is still WIP code. > > A couple of comments. > Firstly, the idea of a printf method to print the IP as symbol+offset is an > interesting idea > that should be followed up in its own right. > FWIW, I have no credit in this idea. It has been in Linux for ages and ages. That said, IP address are barely used in FreeBSD, there is no legacy. As such, the API should not use `unsigned long' but `void *'[0]; this is the natural type returned by `__builtin_return_address()' and the `&&' operator. This would allow to introduce a modifier to `%p' to do the translation. - Arnaud ps: netgraph is on my target list, as well as the list code, to some extend :) [0]: as I really hate `caddr_t' ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Using Instruction Pointer address in debug interfaces [Was: Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]]
Hi, On Tue, Nov 8, 2011 at 8:09 PM, Arnaud Lacombe wrote: > Hi, > > On Tue, Nov 8, 2011 at 3:55 PM, Andriy Gapon wrote: >> >> [cc list trimmed] >> >> on 08/11/2011 22:34 Attilio Rao said the following: >>> 2011/11/8 Arnaud Lacombe : To avoid future complaints about the fact that I would be only "talk" without "action", I did implement what I suggested above. As it is quite a large patch-set, I will not post it directly here, however, it is available on github: >>> >>> I really think that this is way too dependent by the good health of >>> your tool, thus that is highly fragile. >>> >>> However, you may have more luck by just the core of your kernel >>> changes here, for comment and leave alone all the (ptr -> >>> LOCK_FILE/LOCK_LINE conversion). >>> >>> Said that, I think this logic is too fragile and likely won't be as >>> effective as __FILE__/__LINE__ in many cases. >> >> I agree. >> If we were able to somehow automatically, magically, easily and correctly >> determine an instruction pointer of a caller, then it would make sense to >> ditch >> explicit passing of __FILE__/__LINE__ arguments in favor of doing instruction >> pointer decoding. >> > again, no need for magic, this already exists, as the form of gcc[0]'s > __builtin_return_address(0). > actually, this should be __builtin_return_address(1). - Arnaud ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Using Instruction Pointer address in debug interfaces [Was: Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]]
Hi, On Tue, Nov 8, 2011 at 3:34 PM, Attilio Rao wrote: > 2011/11/8 Arnaud Lacombe : >> Hi, >> >> On Mon, Nov 7, 2011 at 2:03 PM, Arnaud Lacombe wrote: >>> On Mon, Nov 7, 2011 at 4:36 AM, Attilio Rao wrote: 2011/11/7 Arnaud Lacombe : > Hi, > > On Sat, Nov 5, 2011 at 10:13 AM, Kostik Belousov > 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. >> >> 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) >> * can only occur at the file EOF. >> */ >> VM_OBJECT_LOCK(object); >> - if (pages[ap->a_reqpage]->valid != 0) { >> + if (vm_page_read_valid(pages[ap->a_reqpage]) != 0) { >> for (i = 0; i < npages; ++i) { >> if (i != ap->a_reqpage) { >> vm_page_lock(pages[i]); >> @@ -198,16 +198,16 @@ nfs_getpages(struct vop_getpages_args *ap) >> /* >> * Read operation filled an entire page >> */ >> - m->valid = VM_PAGE_BITS_ALL; >> - KASSERT(m->dirty == 0, >> + vm_page_write_valid(m, VM_PAGE_BITS_ALL); >> + KASSERT(vm_page_read_dirty(m) == 0, >> ("nfs_getpages: page %p is dirty", m)); >> } else if (size > toff) { >> /* >> * Read operation filled a partial page. >> */ >> - m->valid = 0; >> + vm_page_write_valid(m, 0); >> vm_page_set_valid(m, 0, size - toff); >> - KASSERT(m->dirty == 0, >> + KASSERT(vm_page_read_dirty(m) == 0, >> ("nfs_getpages: page %p is dirty", m)); >> } else { >> /* >> 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) >> vm_page_dirty(m); >> } >> >> +void >> +vm_page_lock_func(vm_page_t m, const char *file, int line) >> +{ >> + >> +#if LOCK_DEBUG > 0 || defined(MUTEX_NOINLINE) >> + _mtx_lock_flags(vm_page_lockptr(m), 0, file, line); >> +#else >> + __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. >>> Well, unfortunately, I wasn't around to comment 10 years ago when this got >>> in. >>> 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). >>> Yes, and that's not an excuse to use the _internal_ implementation >>> details of mutex(9), and not the exposed API. This is especially true >>> when applied to someone so eager to follow "standards". >>> 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. >>> You comment just as if __FILE__ and __LINE__ were mandatory in a debug >>> interface. This is _not_ true. __FILE__ and __LINE__ are just hideous >>> for debugging of anything but early alpha code. LOCK_FILE and >>> LOCK_LINE are a bad answer to a bad interface. Even if you just pass >>> NULL and 0 as argument, you've got the bloat of passing u
Re: Using Instruction Pointer address in debug interfaces [Was: Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]]
Hi, On Tue, Nov 8, 2011 at 3:55 PM, Andriy Gapon wrote: > > [cc list trimmed] > > on 08/11/2011 22:34 Attilio Rao said the following: >> 2011/11/8 Arnaud Lacombe : >>> To avoid future complaints about the fact that I would be only "talk" >>> without "action", I did implement what I suggested above. As it is >>> quite a large patch-set, I will not post it directly here, however, it >>> is available on github: >> >> I really think that this is way too dependent by the good health of >> your tool, thus that is highly fragile. >> >> However, you may have more luck by just the core of your kernel >> changes here, for comment and leave alone all the (ptr -> >> LOCK_FILE/LOCK_LINE conversion). >> >> Said that, I think this logic is too fragile and likely won't be as >> effective as __FILE__/__LINE__ in many cases. > > I agree. > If we were able to somehow automatically, magically, easily and correctly > determine an instruction pointer of a caller, then it would make sense to > ditch > explicit passing of __FILE__/__LINE__ arguments in favor of doing instruction > pointer decoding. > again, no need for magic, this already exists, as the form of gcc[0]'s __builtin_return_address(0). > But if we are just replacing explicit passing of (well-known) macros > __FILE__/__LINE__ with explicit passing of THIS_IP, then I don't see a point. > make sense too, but you need to be sure stuff between the caller and callee is fully inlined. > Apologies if I missed the rationale for this change. > mostly getting rid of all the __FILE__ and __LINE__ bloat. - Arnaud [0]: check about LLVM support is left to the reader. ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Using Instruction Pointer address in debug interfaces [Was: Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]]
Hi, On Tue, Nov 8, 2011 at 7:08 PM, Julian Elischer wrote: > On 11/8/11 10:49 AM, Arnaud Lacombe wrote: >> >> Hi, >> To avoid future complaints about the fact that I would be only "talk" >> without "action", I did implement what I suggested above. As it is >> quite a large patch-set, I will not post it directly here, however, it >> is available on github: >> >> https://github.com/lacombar/freebsd/tree/master/topic/kern-lock-debug >> >> It convert a bunch of debug interface to use the caller instruction >> pointer, as well as a proof-of-concept teaching printf(9) to convert >> IP to symbol_name+offset. >> >> It translates in a direct saving of about +250kB on i386's GENERIC, >> just in kernel text size. Even the worst case, ie LOCK_DEBUG == 0, >> translates to a save of +80kB. >> >> Please note that this is still WIP code. > > A couple of comments. > Firstly, the idea of a printf method to print the IP as symbol+offset is an > interesting idea > that should be followed up in its own right. > > However, (comment 2) I would much rather file+line in this case. > I don't want to have the tools to decode the offset into a location in > sources. > this already exists and is called "debug symbols" - Arnaud > We have both systems in operation art work and I far prefer the latter. > > ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Using Instruction Pointer address in debug interfaces [Was: Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]]
On 11/8/11 10:49 AM, Arnaud Lacombe wrote: Hi, To avoid future complaints about the fact that I would be only "talk" without "action", I did implement what I suggested above. As it is quite a large patch-set, I will not post it directly here, however, it is available on github: https://github.com/lacombar/freebsd/tree/master/topic/kern-lock-debug It convert a bunch of debug interface to use the caller instruction pointer, as well as a proof-of-concept teaching printf(9) to convert IP to symbol_name+offset. It translates in a direct saving of about +250kB on i386's GENERIC, just in kernel text size. Even the worst case, ie LOCK_DEBUG == 0, translates to a save of +80kB. Please note that this is still WIP code. A couple of comments. Firstly, the idea of a printf method to print the IP as symbol+offset is an interesting idea that should be followed up in its own right. However, (comment 2) I would much rather file+line in this case. I don't want to have the tools to decode the offset into a location in sources. We have both systems in operation art work and I far prefer the latter. ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Using Instruction Pointer address in debug interfaces [Was: Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]]
[cc list trimmed] on 08/11/2011 22:34 Attilio Rao said the following: > 2011/11/8 Arnaud Lacombe : >> To avoid future complaints about the fact that I would be only "talk" >> without "action", I did implement what I suggested above. As it is >> quite a large patch-set, I will not post it directly here, however, it >> is available on github: > > I really think that this is way too dependent by the good health of > your tool, thus that is highly fragile. > > However, you may have more luck by just the core of your kernel > changes here, for comment and leave alone all the (ptr -> > LOCK_FILE/LOCK_LINE conversion). > > Said that, I think this logic is too fragile and likely won't be as > effective as __FILE__/__LINE__ in many cases. I agree. If we were able to somehow automatically, magically, easily and correctly determine an instruction pointer of a caller, then it would make sense to ditch explicit passing of __FILE__/__LINE__ arguments in favor of doing instruction pointer decoding. But if we are just replacing explicit passing of (well-known) macros __FILE__/__LINE__ with explicit passing of THIS_IP, then I don't see a point. Apologies if I missed the rationale for this change. -- Andriy Gapon ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Using Instruction Pointer address in debug interfaces [Was: Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]]
2011/11/8 Arnaud Lacombe : > Hi, > > On Mon, Nov 7, 2011 at 2:03 PM, Arnaud Lacombe wrote: >> On Mon, Nov 7, 2011 at 4:36 AM, Attilio Rao wrote: >>> 2011/11/7 Arnaud Lacombe : Hi, On Sat, Nov 5, 2011 at 10:13 AM, Kostik Belousov 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. > > 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) > * can only occur at the file EOF. > */ > VM_OBJECT_LOCK(object); > - if (pages[ap->a_reqpage]->valid != 0) { > + if (vm_page_read_valid(pages[ap->a_reqpage]) != 0) { > for (i = 0; i < npages; ++i) { > if (i != ap->a_reqpage) { > vm_page_lock(pages[i]); > @@ -198,16 +198,16 @@ nfs_getpages(struct vop_getpages_args *ap) > /* > * Read operation filled an entire page > */ > - m->valid = VM_PAGE_BITS_ALL; > - KASSERT(m->dirty == 0, > + vm_page_write_valid(m, VM_PAGE_BITS_ALL); > + KASSERT(vm_page_read_dirty(m) == 0, > ("nfs_getpages: page %p is dirty", m)); > } else if (size > toff) { > /* > * Read operation filled a partial page. > */ > - m->valid = 0; > + vm_page_write_valid(m, 0); > vm_page_set_valid(m, 0, size - toff); > - KASSERT(m->dirty == 0, > + KASSERT(vm_page_read_dirty(m) == 0, > ("nfs_getpages: page %p is dirty", m)); > } else { > /* > 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) > vm_page_dirty(m); > } > > +void > +vm_page_lock_func(vm_page_t m, const char *file, int line) > +{ > + > +#if LOCK_DEBUG > 0 || defined(MUTEX_NOINLINE) > + _mtx_lock_flags(vm_page_lockptr(m), 0, file, line); > +#else > + __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. >>> >> Well, unfortunately, I wasn't around to comment 10 years ago when this got >> in. >> >>> 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). >>> >> Yes, and that's not an excuse to use the _internal_ implementation >> details of mutex(9), and not the exposed API. This is especially true >> when applied to someone so eager to follow "standards". >> >>> 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. >>> >> You comment just as if __FILE__ and __LINE__ were mandatory in a debug >> interface. This is _not_ true. __FILE__ and __LINE__ are just hideous >> for debugging of anything but early alpha code. LOCK_FILE and >> LOCK_LINE are a bad answer to a bad interface. Even if you just pass >> NULL and 0 as argument, you've got the bloat of passing unused >> argument. You might as well just pass a single argument that would do >> the exact same job and be _always_ available, eg. the IP of the >> caller, or the fi
Using Instruction Pointer address in debug interfaces [Was: Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]]
Hi, On Mon, Nov 7, 2011 at 2:03 PM, Arnaud Lacombe wrote: > On Mon, Nov 7, 2011 at 4:36 AM, Attilio Rao wrote: >> 2011/11/7 Arnaud Lacombe : >>> Hi, >>> >>> On Sat, Nov 5, 2011 at 10:13 AM, Kostik Belousov >>> 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. 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) * can only occur at the file EOF. */ VM_OBJECT_LOCK(object); - if (pages[ap->a_reqpage]->valid != 0) { + if (vm_page_read_valid(pages[ap->a_reqpage]) != 0) { for (i = 0; i < npages; ++i) { if (i != ap->a_reqpage) { vm_page_lock(pages[i]); @@ -198,16 +198,16 @@ nfs_getpages(struct vop_getpages_args *ap) /* * Read operation filled an entire page */ - m->valid = VM_PAGE_BITS_ALL; - KASSERT(m->dirty == 0, + vm_page_write_valid(m, VM_PAGE_BITS_ALL); + KASSERT(vm_page_read_dirty(m) == 0, ("nfs_getpages: page %p is dirty", m)); } else if (size > toff) { /* * Read operation filled a partial page. */ - m->valid = 0; + vm_page_write_valid(m, 0); vm_page_set_valid(m, 0, size - toff); - KASSERT(m->dirty == 0, + KASSERT(vm_page_read_dirty(m) == 0, ("nfs_getpages: page %p is dirty", m)); } else { /* 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) vm_page_dirty(m); } +void +vm_page_lock_func(vm_page_t m, const char *file, int line) +{ + +#if LOCK_DEBUG > 0 || defined(MUTEX_NOINLINE) + _mtx_lock_flags(vm_page_lockptr(m), 0, file, line); +#else + __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. >> > Well, unfortunately, I wasn't around to comment 10 years ago when this got in. > >> 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). >> > Yes, and that's not an excuse to use the _internal_ implementation > details of mutex(9), and not the exposed API. This is especially true > when applied to someone so eager to follow "standards". > >> 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. >> > You comment just as if __FILE__ and __LINE__ were mandatory in a debug > interface. This is _not_ true. __FILE__ and __LINE__ are just hideous > for debugging of anything but early alpha code. LOCK_FILE and > LOCK_LINE are a bad answer to a bad interface. Even if you just pass > NULL and 0 as argument, you've got the bloat of passing unused > argument. You might as well just pass a single argument that would do > the exact same job and be _always_ available, eg. the IP of the > caller, or the first return address. KDB magic still let you translate > to something humanly understandable. If the toolchain does not support > the feature o
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
Hi, On Mon, Nov 7, 2011 at 2:35 PM, Kostik Belousov wrote: > On Mon, Nov 07, 2011 at 11:45:38AM -0600, Alan Cox wrote: >> 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. > > Sure. Below is the extraction of only vm_page_lock() bits, together > with the suggested rename. When Attilio provides the promised simplification > of the mutex KPI, this can be reduced. > If you want to be pedantic, you also must hide the definition of `struct vm_page' when KLD_MODULE is defined. You want to be sure no one is gonna mess with the internal of the structure (ie. either directly dereference fields, compute size or any offset...) and will have no other choice but to use assessors. Maybe you are addressing this in another patch. - Arnaud > diff --git a/sys/vm/vm_page.c b/sys/vm/vm_page.c > index 389aea5..ea4ea34 100644 > --- a/sys/vm/vm_page.c > +++ b/sys/vm/vm_page.c > @@ -2677,6 +2677,44 @@ vm_page_test_dirty(vm_page_t m) > vm_page_dirty(m); > } > > +void > +vm_page_lock_KBI(vm_page_t m, const char *file, int line) > +{ > + > +#if LOCK_DEBUG > 0 || defined(MUTEX_NOINLINE) > + _mtx_lock_flags(vm_page_lockptr(m), 0, file, line); > +#else > + __mtx_lock(vm_page_lockptr(m), 0, file, line); > +#endif > +} > + > +void > +vm_page_unlock_KBI(vm_page_t m, const char *file, int line) > +{ > + > +#if LOCK_DEBUG > 0 || defined(MUTEX_NOINLINE) > + _mtx_unlock_flags(vm_page_lockptr(m), 0, file, line); > +#else > + __mtx_unlock(vm_page_lockptr(m), curthread, 0, file, line); > +#endif > +} > + > +int > +vm_page_trylock_KBI(vm_page_t m, const char *file, int line) > +{ > + > + return (_mtx_trylock(vm_page_lockptr(m), 0, file, line)); > +} > + > +void > +vm_page_lock_assert_KBI(vm_page_t m, int a, const char *file, int line) > +{ > + > +#ifdef INVARIANTS > + _mtx_assert(vm_page_lockptr(m), a, file, line); > +#endif > +} > + > int so_zerocp_fullpage = 0; > > /* > diff --git a/sys/vm/vm_page.h b/sys/vm/vm_page.h > index 7099b70..a5604b7 100644 > --- a/sys/vm/vm_page.h > +++ b/sys/vm/vm_page.h > @@ -218,11 +218,23 @@ extern struct vpglocks pa_lock[]; > > #define PA_LOCK_ASSERT(pa, a) mtx_assert(PA_LOCKPTR(pa), (a)) > > +#ifdef KLD_MODULE > +#define vm_page_lock(m) vm_page_lock_KBI((m), LOCK_FILE, > LOCK_LINE) > +#define vm_page_unlock(m) vm_page_unlock_KBI((m), LOCK_FILE, > LOCK_LINE) > +#define vm_page_trylock(m) vm_page_trylock_KBI((m), LOCK_FILE, > LOCK_LINE) > +#ifdef INVARIANTS > +#define vm_page_lock_assert(m, a) \ > + vm_page_lock_assert_KBI((m), (a), LOCK_FILE, LOCK_LINE) > +#else > +#define vm_page_lock_assert(m, a) > +#endif > +#else /* KLD_MODULE */ > #define vm_page_lockptr(m) (PA_LOCKPTR(VM_PAGE_TO_PHYS((m > #define vm_page_lock(m) mtx_lock(vm_page_lockptr((m))) > #define vm_page_unlock(m) mtx_unlock(vm_page_lockptr((m))) > #define vm_page_trylock(m) mtx_trylock(vm_page_lockptr((m))) > #define vm_page_lock_assert(m, a) > mtx_assert(vm_page_lockptr((m)), (a)) > +#endif > > #define vm_page_queue_free_mtx vm_page_queue_free_lock.data > /* > @@ -403,6 +415,11 @@ void vm_page_cowfault (vm_page_t); > int vm_page_cowsetup(vm_page_t); > void vm_page_cowclear (vm_page_t); > > +void vm_page_lock_KBI(vm_page_t m, const char *file, int line); > +void vm_page_unlock_KBI(vm_page_t m, const char *file, int line); > +int vm_page_trylock_KBI(vm_page_t m, const char *file, int line); > +void vm_page_lock_assert_KBI(vm_page_t m, int a, const char *file, int line); > + > #ifdef INVARIANTS > void vm_page_object_lock_assert(vm_page_t m); > #define VM_PAGE_OBJECT_LOCK_ASSERT(m) vm_page_object_lock_assert(m) > ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
On Mon, Nov 07, 2011 at 11:47:59AM -0800, m...@freebsd.org wrote: > On Mon, Nov 7, 2011 at 11:35 AM, Kostik Belousov wrote: > > On Mon, Nov 07, 2011 at 11:45:38AM -0600, Alan Cox wrote: > >> 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. > > > > Sure. Below is the extraction of only vm_page_lock() bits, together > > with the suggested rename. When Attilio provides the promised simplification > > of the mutex KPI, this can be reduced. > > > > diff --git a/sys/vm/vm_page.c b/sys/vm/vm_page.c > > index 389aea5..ea4ea34 100644 > > --- a/sys/vm/vm_page.c > > +++ b/sys/vm/vm_page.c > > @@ -2677,6 +2677,44 @@ vm_page_test_dirty(vm_page_t m) > > vm_page_dirty(m); > > } > > > > +void > > +vm_page_lock_KBI(vm_page_t m, const char *file, int line) > > +{ > > + > > +#if LOCK_DEBUG > 0 || defined(MUTEX_NOINLINE) > > + _mtx_lock_flags(vm_page_lockptr(m), 0, file, line); > > +#else > > + __mtx_lock(vm_page_lockptr(m), 0, file, line); > > +#endif > > +} > > + > > +void > > +vm_page_unlock_KBI(vm_page_t m, const char *file, int line) > > +{ > > + > > +#if LOCK_DEBUG > 0 || defined(MUTEX_NOINLINE) > > + _mtx_unlock_flags(vm_page_lockptr(m), 0, file, line); > > +#else > > + __mtx_unlock(vm_page_lockptr(m), curthread, 0, file, line); > > +#endif > > +} > > + > > +int > > +vm_page_trylock_KBI(vm_page_t m, const char *file, int line) > > +{ > > + > > + return (_mtx_trylock(vm_page_lockptr(m), 0, file, line)); > > +} > > + > > +void > > +vm_page_lock_assert_KBI(vm_page_t m, int a, const char *file, int line) > > +{ > > + > > +#ifdef INVARIANTS > > + _mtx_assert(vm_page_lockptr(m), a, file, line); > > +#endif > > +} > > + > > int so_zerocp_fullpage = 0; > > > > /* > > diff --git a/sys/vm/vm_page.h b/sys/vm/vm_page.h > > index 7099b70..a5604b7 100644 > > --- a/sys/vm/vm_page.h > > +++ b/sys/vm/vm_page.h > > @@ -218,11 +218,23 @@ extern struct vpglocks pa_lock[]; > > > > #define PA_LOCK_ASSERT(pa, a) mtx_assert(PA_LOCKPTR(pa), (a)) > > > > +#ifdef KLD_MODULE > > +#define vm_page_lock(m) vm_page_lock_KBI((m), LOCK_FILE, > > LOCK_LINE) > > +#define vm_page_unlock(m) vm_page_unlock_KBI((m), LOCK_FILE, > > LOCK_LINE) > > +#define vm_page_trylock(m) vm_page_trylock_KBI((m), LOCK_FILE, > > LOCK_LINE) > > +#ifdef INVARIANTS > > +#define vm_page_lock_assert(m, a) \ > > + vm_page_lock_assert_KBI((m), (a), LOCK_FILE, LOCK_LINE) > > +#else > > +#define vm_page_lock_assert(m, a) > > +#endif > > +#else /* KLD_MODULE */ > > #define vm_page_lockptr(m) (PA_LOCKPTR(VM_PAGE_TO_PHYS((m > > Is it not possible to have vm_page_lockptr() be a function for the > KLD_MODULE case? Because then the vm_page_lock() functions and > friends would all just use mtx_lock, etc., in both the KLD and non-KLD > case. > > Or am I missing something? It is possible, but I tried to avoid exposing lockptr. IMHO vm_page_lockptr() is an implementation detail. Please also see my other response to Alan regarding the relocking macros. pgpYgFHehhwCS.pgp Description: PGP signature
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
On Mon, Nov 7, 2011 at 11:35 AM, Kostik Belousov wrote: > On Mon, Nov 07, 2011 at 11:45:38AM -0600, Alan Cox wrote: >> 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. > > Sure. Below is the extraction of only vm_page_lock() bits, together > with the suggested rename. When Attilio provides the promised simplification > of the mutex KPI, this can be reduced. > > diff --git a/sys/vm/vm_page.c b/sys/vm/vm_page.c > index 389aea5..ea4ea34 100644 > --- a/sys/vm/vm_page.c > +++ b/sys/vm/vm_page.c > @@ -2677,6 +2677,44 @@ vm_page_test_dirty(vm_page_t m) > vm_page_dirty(m); > } > > +void > +vm_page_lock_KBI(vm_page_t m, const char *file, int line) > +{ > + > +#if LOCK_DEBUG > 0 || defined(MUTEX_NOINLINE) > + _mtx_lock_flags(vm_page_lockptr(m), 0, file, line); > +#else > + __mtx_lock(vm_page_lockptr(m), 0, file, line); > +#endif > +} > + > +void > +vm_page_unlock_KBI(vm_page_t m, const char *file, int line) > +{ > + > +#if LOCK_DEBUG > 0 || defined(MUTEX_NOINLINE) > + _mtx_unlock_flags(vm_page_lockptr(m), 0, file, line); > +#else > + __mtx_unlock(vm_page_lockptr(m), curthread, 0, file, line); > +#endif > +} > + > +int > +vm_page_trylock_KBI(vm_page_t m, const char *file, int line) > +{ > + > + return (_mtx_trylock(vm_page_lockptr(m), 0, file, line)); > +} > + > +void > +vm_page_lock_assert_KBI(vm_page_t m, int a, const char *file, int line) > +{ > + > +#ifdef INVARIANTS > + _mtx_assert(vm_page_lockptr(m), a, file, line); > +#endif > +} > + > int so_zerocp_fullpage = 0; > > /* > diff --git a/sys/vm/vm_page.h b/sys/vm/vm_page.h > index 7099b70..a5604b7 100644 > --- a/sys/vm/vm_page.h > +++ b/sys/vm/vm_page.h > @@ -218,11 +218,23 @@ extern struct vpglocks pa_lock[]; > > #define PA_LOCK_ASSERT(pa, a) mtx_assert(PA_LOCKPTR(pa), (a)) > > +#ifdef KLD_MODULE > +#define vm_page_lock(m) vm_page_lock_KBI((m), LOCK_FILE, > LOCK_LINE) > +#define vm_page_unlock(m) vm_page_unlock_KBI((m), LOCK_FILE, > LOCK_LINE) > +#define vm_page_trylock(m) vm_page_trylock_KBI((m), LOCK_FILE, > LOCK_LINE) > +#ifdef INVARIANTS > +#define vm_page_lock_assert(m, a) \ > + vm_page_lock_assert_KBI((m), (a), LOCK_FILE, LOCK_LINE) > +#else > +#define vm_page_lock_assert(m, a) > +#endif > +#else /* KLD_MODULE */ > #define vm_page_lockptr(m) (PA_LOCKPTR(VM_PAGE_TO_PHYS((m Is it not possible to have vm_page_lockptr() be a function for the KLD_MODULE case? Because then the vm_page_lock() functions and friends would all just use mtx_lock, etc., in both the KLD and non-KLD case. Or am I missing something? Thanks, matthew > #define vm_page_lock(m) mtx_lock(vm_page_lockptr((m))) > #define vm_page_unlock(m) mtx_unlock(vm_page_lockptr((m))) > #define vm_page_trylock(m) mtx_trylock(vm_page_lockptr((m))) > #define vm_page_lock_assert(m, a) > mtx_assert(vm_page_lockptr((m)), (a)) > +#endif > > #define vm_page_queue_free_mtx vm_page_queue_free_lock.data > /* > @@ -403,6 +415,11 @@ void vm_page_cowfault (vm_page_t); > int vm_page_cowsetup(vm_page_t); > void vm_page_cowclear (vm_page_t); > > +void vm_page_lock_KBI(vm_page_t m, const char *file, int line); > +void vm_page_unlock_KBI(vm_page_t m, const char *file, int line); > +int vm_page_trylock_KBI(vm_page_t m, const char *file, int line); > +void vm_page_lock_assert_KBI(vm_page_t m, int a, const char *file, int line); > + > #ifdef INVARIANTS > void vm_page_object_lock_assert(vm_page_t m); > #define VM_PAGE_OBJECT_LOCK_ASSERT(m) vm_page_object_lock_assert(m) > ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
Hi, On Mon, Nov 7, 2011 at 2:03 PM, Arnaud Lacombe wrote: > On Mon, Nov 7, 2011 at 4:36 AM, Attilio Rao wrote: >> I'm unsure if this replies to your concerns because you just criticize >> without making a real technical question in this post. >> > I made comments on 3 points: > - using internal implementation details of mutex(9) is broken > - LOCK_FILE/LOCK_LINE are broken (a bit of a divergence on the > original subject :/) > - there is _no_ reason not to use inlines function for such trivial oneliners > ok, I read the original thread, now that I understand the purpose of the patch. It would make the third comment irrelevant, but I still do not agree about the reason of the patch. - ARnaud ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
On Mon, Nov 07, 2011 at 11:45:38AM -0600, Alan Cox wrote: > 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. Sure. Below is the extraction of only vm_page_lock() bits, together with the suggested rename. When Attilio provides the promised simplification of the mutex KPI, this can be reduced. diff --git a/sys/vm/vm_page.c b/sys/vm/vm_page.c index 389aea5..ea4ea34 100644 --- a/sys/vm/vm_page.c +++ b/sys/vm/vm_page.c @@ -2677,6 +2677,44 @@ vm_page_test_dirty(vm_page_t m) vm_page_dirty(m); } +void +vm_page_lock_KBI(vm_page_t m, const char *file, int line) +{ + +#if LOCK_DEBUG > 0 || defined(MUTEX_NOINLINE) + _mtx_lock_flags(vm_page_lockptr(m), 0, file, line); +#else + __mtx_lock(vm_page_lockptr(m), 0, file, line); +#endif +} + +void +vm_page_unlock_KBI(vm_page_t m, const char *file, int line) +{ + +#if LOCK_DEBUG > 0 || defined(MUTEX_NOINLINE) + _mtx_unlock_flags(vm_page_lockptr(m), 0, file, line); +#else + __mtx_unlock(vm_page_lockptr(m), curthread, 0, file, line); +#endif +} + +int +vm_page_trylock_KBI(vm_page_t m, const char *file, int line) +{ + + return (_mtx_trylock(vm_page_lockptr(m), 0, file, line)); +} + +void +vm_page_lock_assert_KBI(vm_page_t m, int a, const char *file, int line) +{ + +#ifdef INVARIANTS + _mtx_assert(vm_page_lockptr(m), a, file, line); +#endif +} + int so_zerocp_fullpage = 0; /* diff --git a/sys/vm/vm_page.h b/sys/vm/vm_page.h index 7099b70..a5604b7 100644 --- a/sys/vm/vm_page.h +++ b/sys/vm/vm_page.h @@ -218,11 +218,23 @@ extern struct vpglocks pa_lock[]; #definePA_LOCK_ASSERT(pa, a) mtx_assert(PA_LOCKPTR(pa), (a)) +#ifdef KLD_MODULE +#definevm_page_lock(m) vm_page_lock_KBI((m), LOCK_FILE, LOCK_LINE) +#definevm_page_unlock(m) vm_page_unlock_KBI((m), LOCK_FILE, LOCK_LINE) +#definevm_page_trylock(m) vm_page_trylock_KBI((m), LOCK_FILE, LOCK_LINE) +#ifdef INVARIANTS +#definevm_page_lock_assert(m, a) \ +vm_page_lock_assert_KBI((m), (a), LOCK_FILE, LOCK_LINE) +#else +#definevm_page_lock_assert(m, a) +#endif +#else /* KLD_MODULE */ #definevm_page_lockptr(m) (PA_LOCKPTR(VM_PAGE_TO_PHYS((m #definevm_page_lock(m) mtx_lock(vm_page_lockptr((m))) #definevm_page_unlock(m) mtx_unlock(vm_page_lockptr((m))) #definevm_page_trylock(m) mtx_trylock(vm_page_lockptr((m))) #definevm_page_lock_assert(m, a) mtx_assert(vm_page_lockptr((m)), (a)) +#endif #definevm_page_queue_free_mtx vm_page_queue_free_lock.data /* @@ -403,6 +415,11 @@ void vm_page_cowfault (vm_page_t); int vm_page_cowsetup(vm_page_t); void vm_page_cowclear (vm_page_t); +void vm_page_lock_KBI(vm_page_t m, const char *file, int line); +void vm_page_unlock_KBI(vm_page_t m, const char *file, int line); +int vm_page_trylock_KBI(vm_page_t m, const char *file, int line); +void vm_page_lock_assert_KBI(vm_page_t m, int a, const char *file, int line); + #ifdef INVARIANTS void vm_page_object_lock_assert(vm_page_t m); #defineVM_PAGE_OBJECT_LOCK_ASSERT(m) vm_page_object_lock_assert(m) pgpxFZVFLBvYF.pgp Description: PGP signature
Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3
Hi, On Wed, Nov 2, 2011 at 6:32 AM, Andriy Gapon wrote: > > [restored cc: to the original poster] > > on 02/11/2011 08:10 Benjamin Kaduk said the following: >> I am perhaps confused. Last I checked, bsd.kmod.mk caused '-include >> opt_global.h' to be passed on the command line. Is the issue just that the >> opt_global.h used for the kmod could be different from the actual kernel's >> opt_global.h, because KERNCONF was not specified and the header is generated >> at >> module-build time? In this case, clearly the onus is on the user to pass >> KERNCONF at module build time. > > To be precise, this is what is actually passed to a compiler: > sys/conf/kmod.mk: > .if defined(KERNBUILDDIR) > CFLAGS+= -DHAVE_KERNEL_OPTION_HEADERS -include > ${KERNBUILDDIR}/opt_global.h > .endif > > where KERNBUILDDIR can be passed via environment from a kernel build: > sys/conf/kern.post.mk: > MKMODULESENV+= KERNBUILDDIR="${.CURDIR}" SYSDIR="${SYSDIR}" > > KERNCONF does not have any meaning in a module build. > > To make sure that a module build sees exactly the same kernel options as a > kernel with which the module should work, one has to either build the module > together with the kernel (within the kernel build; search for MODULES in > make.conf(5)) or to manually specify KERNBUILDDIR to point to a correct kernel > build directory. (Which to a certain degree implies impossibility to build a > "perfect" module for a pre-built binary kernel or to provide a "perfect" > universal pre-built module for any custom kernel) > > Of course, the real problem is that modules should not care about any (or at > least some) kernel options, they should be isolated from the options via a > proper KPI/KBI (perhaps DDI or "module-to-kernel interface" or whatever). A > module should be able to work correctly with kernels built with different > options. > You cannot be make a point in shade of gray, it either "must care" or "must not care" about kernel option, not "care about some, but not other". Moreover, you cannot advocate stable internal KBI/KPI when you are not even able to provide a stable userland ABI... > P.P.S. [and tangential] I see that many module makefiles fake up various > kernel > options in a fashion similar to the following: > .if !defined(KERNBUILDDIR) > opt_compat.h: > echo "#define COMPAT_FREEBSD6 1" > ${.TARGET} > > opt_kbd.h: > echo "#define KBD_INSTALL_CDEV 1" > ${.TARGET} > .endif > > And handful of modules fake up opt_global.h, e.g.: > opt_global.h: > echo "#define ALTQ 1" > ${.TARGET} This mess is utterly broken. FWIW, I advocate to make KERNBUILDDIR (ie. kernel option's configuration) mandatory for building any modules. - Arnaud > -- > Andriy Gapon > ___ > freebsd-current@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-current > To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org" > ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
On Mon, Nov 7, 2011 at 4:36 AM, Attilio Rao wrote: > 2011/11/7 Arnaud Lacombe : >> Hi, >> >> On Sat, Nov 5, 2011 at 10:13 AM, Kostik Belousov 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. >>> >>> 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) >>> * can only occur at the file EOF. >>> */ >>> VM_OBJECT_LOCK(object); >>> - if (pages[ap->a_reqpage]->valid != 0) { >>> + if (vm_page_read_valid(pages[ap->a_reqpage]) != 0) { >>> for (i = 0; i < npages; ++i) { >>> if (i != ap->a_reqpage) { >>> vm_page_lock(pages[i]); >>> @@ -198,16 +198,16 @@ nfs_getpages(struct vop_getpages_args *ap) >>> /* >>> * Read operation filled an entire page >>> */ >>> - m->valid = VM_PAGE_BITS_ALL; >>> - KASSERT(m->dirty == 0, >>> + vm_page_write_valid(m, VM_PAGE_BITS_ALL); >>> + KASSERT(vm_page_read_dirty(m) == 0, >>> ("nfs_getpages: page %p is dirty", m)); >>> } else if (size > toff) { >>> /* >>> * Read operation filled a partial page. >>> */ >>> - m->valid = 0; >>> + vm_page_write_valid(m, 0); >>> vm_page_set_valid(m, 0, size - toff); >>> - KASSERT(m->dirty == 0, >>> + KASSERT(vm_page_read_dirty(m) == 0, >>> ("nfs_getpages: page %p is dirty", m)); >>> } else { >>> /* >>> 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) >>> vm_page_dirty(m); >>> } >>> >>> +void >>> +vm_page_lock_func(vm_page_t m, const char *file, int line) >>> +{ >>> + >>> +#if LOCK_DEBUG > 0 || defined(MUTEX_NOINLINE) >>> + _mtx_lock_flags(vm_page_lockptr(m), 0, file, line); >>> +#else >>> + __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. > Well, unfortunately, I wasn't around to comment 10 years ago when this got in. > 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). > Yes, and that's not an excuse to use the _internal_ implementation details of mutex(9), and not the exposed API. This is especially true when applied to someone so eager to follow "standards". > 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. > You comment just as if __FILE__ and __LINE__ were mandatory in a debug interface. This is _not_ true. __FILE__ and __LINE__ are just hideous for debugging of anything but early alpha code. LOCK_FILE and LOCK_LINE are a bad answer to a bad interface. Even if you just pass NULL and 0 as argument, you've got the bloat of passing unused argument. You might as well just pass a single argument that would do the exact same job and be _always_ available, eg. the IP of the caller, or the first return address. KDB magic still let you translate to something humanly understandable. If the toolchain does not support the feature on all supported platform, well, fix the toolchain. > I'm unsure if this replies to your concerns because you just criticize > without making a real technical question in this post. > I m
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
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, m...@freebsd.org wrote: On Sat, Nov 5, 2011 at 7:13 AM, Kostik Belousov 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 ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
2011/11/7 Attilio Rao : > 2011/11/7 Arnaud Lacombe : >> Hi, >> >> On Sat, Nov 5, 2011 at 10:13 AM, Kostik Belousov 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. >>> >>> 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) >>> * can only occur at the file EOF. >>> */ >>> VM_OBJECT_LOCK(object); >>> - if (pages[ap->a_reqpage]->valid != 0) { >>> + if (vm_page_read_valid(pages[ap->a_reqpage]) != 0) { >>> for (i = 0; i < npages; ++i) { >>> if (i != ap->a_reqpage) { >>> vm_page_lock(pages[i]); >>> @@ -198,16 +198,16 @@ nfs_getpages(struct vop_getpages_args *ap) >>> /* >>> * Read operation filled an entire page >>> */ >>> - m->valid = VM_PAGE_BITS_ALL; >>> - KASSERT(m->dirty == 0, >>> + vm_page_write_valid(m, VM_PAGE_BITS_ALL); >>> + KASSERT(vm_page_read_dirty(m) == 0, >>> ("nfs_getpages: page %p is dirty", m)); >>> } else if (size > toff) { >>> /* >>> * Read operation filled a partial page. >>> */ >>> - m->valid = 0; >>> + vm_page_write_valid(m, 0); >>> vm_page_set_valid(m, 0, size - toff); >>> - KASSERT(m->dirty == 0, >>> + KASSERT(vm_page_read_dirty(m) == 0, >>> ("nfs_getpages: page %p is dirty", m)); >>> } else { >>> /* >>> 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) >>> vm_page_dirty(m); >>> } >>> >>> +void >>> +vm_page_lock_func(vm_page_t m, const char *file, int line) >>> +{ >>> + >>> +#if LOCK_DEBUG > 0 || defined(MUTEX_NOINLINE) >>> + _mtx_lock_flags(vm_page_lockptr(m), 0, file, line); >>> +#else >>> + __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) >>> + _mtx_unlock_flags(vm_page_lockptr(m), 0, file, line); >>> +#else >>> + __mtx_unlock(vm_page_lockptr(m), curthread, 0, file, 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. After thinking a bit about it, my guess is that the best approach would be patching mutex.h in order to offer a simple way to do what Kostik needs (i.e. a general interface to do that). I wouldn't encourage, infact, neither putting more things in mutex.h or growing checks in other files dependi
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
2011/11/7 Arnaud Lacombe : > Hi, > > On Sat, Nov 5, 2011 at 10:13 AM, Kostik Belousov 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. >> >> 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) >> * can only occur at the file EOF. >> */ >> VM_OBJECT_LOCK(object); >> - if (pages[ap->a_reqpage]->valid != 0) { >> + if (vm_page_read_valid(pages[ap->a_reqpage]) != 0) { >> for (i = 0; i < npages; ++i) { >> if (i != ap->a_reqpage) { >> vm_page_lock(pages[i]); >> @@ -198,16 +198,16 @@ nfs_getpages(struct vop_getpages_args *ap) >> /* >> * Read operation filled an entire page >> */ >> - m->valid = VM_PAGE_BITS_ALL; >> - KASSERT(m->dirty == 0, >> + vm_page_write_valid(m, VM_PAGE_BITS_ALL); >> + KASSERT(vm_page_read_dirty(m) == 0, >> ("nfs_getpages: page %p is dirty", m)); >> } else if (size > toff) { >> /* >> * Read operation filled a partial page. >> */ >> - m->valid = 0; >> + vm_page_write_valid(m, 0); >> vm_page_set_valid(m, 0, size - toff); >> - KASSERT(m->dirty == 0, >> + KASSERT(vm_page_read_dirty(m) == 0, >> ("nfs_getpages: page %p is dirty", m)); >> } else { >> /* >> 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) >> vm_page_dirty(m); >> } >> >> +void >> +vm_page_lock_func(vm_page_t m, const char *file, int line) >> +{ >> + >> +#if LOCK_DEBUG > 0 || defined(MUTEX_NOINLINE) >> + _mtx_lock_flags(vm_page_lockptr(m), 0, file, line); >> +#else >> + __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) >> + _mtx_unlock_flags(vm_page_lockptr(m), 0, file, line); >> +#else >> + __mtx_unlock(vm_page_lockptr(m), curthread, 0, file, 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 -- Peace can only be achieved by understanding - A. Einstein ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
Hi, On Sun, Nov 6, 2011 at 11:42 AM, Kostik Belousov wrote: > On Sun, Nov 06, 2011 at 07:22:51AM -0800, m...@freebsd.org wrote: >> On Sun, Nov 6, 2011 at 4:43 AM, Kostik Belousov wrote: >> > 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. >> >> I'm pretty sure that when the C standard says, "the implementation", >> they're referring to the compiler and OS it runs on. Which makes the >> FreeBSD kernel part of "the implementation", which is precisely why so >> many headers have defines that start with __ and then, if certain >> posix defines are set, also uses non-__ versions of the name. > > For libc providing parts, required by standard, you are right. > But our kernel is a freestanding program using a compiler, so in-kernel > uses of the reserved namespace is a violation. > So you prefer to introduce a new notation which will confuses everybody for the sake of following an interpretation of the standard[0] ? Btw, which point of the standard are you quoting ? Section "7.1.3 Reserved identifiers" of ISO/IEC 9899 ? Thanks, - Arnaud ps: my vote is for a deep-sky-blue bikeshed. [0]: I'd be tempted to interpret "the implementation" as the non-visible part of an API, ie vm_page_lock() is public and rely on __vm_page_lock() for its implementation. As such, I would not consider "the kernel" as a single whole unit, but as a sum of public API and implementation. ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
Hi, On Sat, Nov 5, 2011 at 10:13 AM, Kostik Belousov 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. > > 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) > * can only occur at the file EOF. > */ > VM_OBJECT_LOCK(object); > - if (pages[ap->a_reqpage]->valid != 0) { > + if (vm_page_read_valid(pages[ap->a_reqpage]) != 0) { > for (i = 0; i < npages; ++i) { > if (i != ap->a_reqpage) { > vm_page_lock(pages[i]); > @@ -198,16 +198,16 @@ nfs_getpages(struct vop_getpages_args *ap) > /* > * Read operation filled an entire page > */ > - m->valid = VM_PAGE_BITS_ALL; > - KASSERT(m->dirty == 0, > + vm_page_write_valid(m, VM_PAGE_BITS_ALL); > + KASSERT(vm_page_read_dirty(m) == 0, > ("nfs_getpages: page %p is dirty", m)); > } else if (size > toff) { > /* > * Read operation filled a partial page. > */ > - m->valid = 0; > + vm_page_write_valid(m, 0); > vm_page_set_valid(m, 0, size - toff); > - KASSERT(m->dirty == 0, > + KASSERT(vm_page_read_dirty(m) == 0, > ("nfs_getpages: page %p is dirty", m)); > } else { > /* > 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) > vm_page_dirty(m); > } > > +void > +vm_page_lock_func(vm_page_t m, const char *file, int line) > +{ > + > +#if LOCK_DEBUG > 0 || defined(MUTEX_NOINLINE) > + _mtx_lock_flags(vm_page_lockptr(m), 0, file, line); > +#else > + __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. > +void > +vm_page_unlock_func(vm_page_t m, const char *file, int line) > +{ > + > +#if LOCK_DEBUG > 0 || defined(MUTEX_NOINLINE) > + _mtx_unlock_flags(vm_page_lockptr(m), 0, file, line); > +#else > + __mtx_unlock(vm_page_lockptr(m), curthread, 0, file, line); > +#endif > +} > + > +int > +vm_page_trylock_func(vm_page_t m, const char *file, int line) > +{ > + > + return (_mtx_trylock(vm_page_lockptr(m), 0, file, line)); > +} > + > +void > +vm_page_lock_assert_func(vm_page_t m, int a, const char *file, int line) > +{ > + > +#ifdef INVARIANTS > + _mtx_assert(vm_page_lockptr(m), a, file, line); > +#endif > +} > + same remark on all the above. > +vm_page_bits_t > +vm_page_read_dirty_func(vm_page_t m) > +{ > + > + return (m->dirty); > +} > + > +vm_page_bits_t > +vm_page_read_valid_func(vm_page_t m) > +{ > + > + return (m->valid); > +} > + > +void > +vm_page_write_valid_func(vm_page_t m, vm_page_bits_t v) > +{ > + > + m->valid = v; > +} > + > + > int so_zerocp_fullpage = 0; > > /* > diff --git a/sys/vm/vm_page.h b/sys/vm/vm_page.h > index 7099b70..4f8f71e 100644 > --- a/sys/vm/vm_page.h > +++ b/sys/vm/vm_page.h > @@ -218,12 +218,50 @@ extern struct vpglocks pa_lock[]; > > #define PA_LOCK_ASSERT(pa, a) mtx_assert(PA_LOCKPTR(pa), (a)) > > +#ifdef KLD_MODULE > +#define vm_page_lock(m) vm_page_lock_func((m), LOCK_FILE, > LOCK_LINE) > +#define vm_page_unlock(m) vm_page_unlock_func((m), LOCK_FILE, > LOCK_LINE) > +#define vm_page_trylock(m) vm_page_trylock_func((m), LOCK_FILE, > LOCK_LINE) > +#ifdef INVARIANTS > +#define vm_page_lock_assert(m, a) \ > + vm_page_lock_assert_func((m), (a), LOCK_FILE, LOCK_LINE) > +#else > +#define vm_page_lock_assert(m, a) > +#endif > + > +#define vm_page_read_dirty(m) vm_page_read_dirty_func((m)) > +#define vm_page_read_
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
On Sun, Nov 06, 2011 at 07:22:51AM -0800, m...@freebsd.org wrote: > On Sun, Nov 6, 2011 at 4:43 AM, Kostik Belousov wrote: > > 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. > > I'm pretty sure that when the C standard says, "the implementation", > they're referring to the compiler and OS it runs on. Which makes the > FreeBSD kernel part of "the implementation", which is precisely why so > many headers have defines that start with __ and then, if certain > posix defines are set, also uses non-__ versions of the name. For libc providing parts, required by standard, you are right. But our kernel is a freestanding program using a compiler, so in-kernel uses of the reserved namespace is a violation. pgp4CGtNzn8lW.pgp Description: PGP signature
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
On Sun, Nov 6, 2011 at 4:43 AM, Kostik Belousov wrote: > 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. I'm pretty sure that when the C standard says, "the implementation", they're referring to the compiler and OS it runs on. Which makes the FreeBSD kernel part of "the implementation", which is precisely why so many headers have defines that start with __ and then, if certain posix defines are set, also uses non-__ versions of the name. Cheers, matthew ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
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, m...@freebsd.org wrote: > >>On Sat, Nov 5, 2011 at 7:13 AM, Kostik Belousov > >>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. 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. pgpr5pwKbvDyo.pgp Description: PGP signature
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
On 11/05/2011 10:15, Kostik Belousov wrote: On Sat, Nov 05, 2011 at 07:37:48AM -0700, m...@freebsd.org wrote: On Sat, Nov 5, 2011 at 7:13 AM, Kostik Belousov 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. Alan ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
On Sat, Nov 05, 2011 at 07:37:48AM -0700, m...@freebsd.org wrote: > On Sat, Nov 5, 2011 at 7:13 AM, Kostik Belousov 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. pgpw68GTgJGiJ.pgp Description: PGP signature
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
On Sat, Nov 5, 2011 at 7:13 AM, Kostik Belousov 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). Thanks, matthew > > 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) > * can only occur at the file EOF. > */ > VM_OBJECT_LOCK(object); > - if (pages[ap->a_reqpage]->valid != 0) { > + if (vm_page_read_valid(pages[ap->a_reqpage]) != 0) { > for (i = 0; i < npages; ++i) { > if (i != ap->a_reqpage) { > vm_page_lock(pages[i]); > @@ -198,16 +198,16 @@ nfs_getpages(struct vop_getpages_args *ap) > /* > * Read operation filled an entire page > */ > - m->valid = VM_PAGE_BITS_ALL; > - KASSERT(m->dirty == 0, > + vm_page_write_valid(m, VM_PAGE_BITS_ALL); > + KASSERT(vm_page_read_dirty(m) == 0, > ("nfs_getpages: page %p is dirty", m)); > } else if (size > toff) { > /* > * Read operation filled a partial page. > */ > - m->valid = 0; > + vm_page_write_valid(m, 0); > vm_page_set_valid(m, 0, size - toff); > - KASSERT(m->dirty == 0, > + KASSERT(vm_page_read_dirty(m) == 0, > ("nfs_getpages: page %p is dirty", m)); > } else { > /* > 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) > vm_page_dirty(m); > } > > +void > +vm_page_lock_func(vm_page_t m, const char *file, int line) > +{ > + > +#if LOCK_DEBUG > 0 || defined(MUTEX_NOINLINE) > + _mtx_lock_flags(vm_page_lockptr(m), 0, file, line); > +#else > + __mtx_lock(vm_page_lockptr(m), 0, file, line); > +#endif > +} > + > +void > +vm_page_unlock_func(vm_page_t m, const char *file, int line) > +{ > + > +#if LOCK_DEBUG > 0 || defined(MUTEX_NOINLINE) > + _mtx_unlock_flags(vm_page_lockptr(m), 0, file, line); > +#else > + __mtx_unlock(vm_page_lockptr(m), curthread, 0, file, line); > +#endif > +} > + > +int > +vm_page_trylock_func(vm_page_t m, const char *file, int line) > +{ > + > + return (_mtx_trylock(vm_page_lockptr(m), 0, file, line)); > +} > + > +void > +vm_page_lock_assert_func(vm_page_t m, int a, const char *file, int line) > +{ > + > +#ifdef INVARIANTS > + _mtx_assert(vm_page_lockptr(m), a, file, line); > +#endif > +} > + > +vm_page_bits_t > +vm_page_read_dirty_func(vm_page_t m) > +{ > + > + return (m->dirty); > +} > + > +vm_page_bits_t > +vm_page_read_valid_func(vm_page_t m) > +{ > + > + return (m->valid); > +} > + > +void > +vm_page_write_valid_func(vm_page_t m, vm_page_bits_t v) > +{ > + > + m->valid = v; > +} > + > + > int so_zerocp_fullpage = 0; > > /* > diff --git a/sys/vm/vm_page.h b/sys/vm/vm_page.h > index 7099b70..4f8f71e 100644 > --- a/sys/vm/vm_page.h > +++ b/sys/vm/vm_page.h > @@ -218,12 +218,50 @@ extern struct vpglocks pa_lock[]; > > #define PA_LOCK_ASSERT(pa, a) mtx_assert(PA_LOCKPTR(pa), (a)) > > +#ifdef KLD_MODULE > +#define vm_page_lock(m) vm_page_lock_func((m), LOCK_FILE, > LOCK_LINE) > +#define vm_page_unlock(m) vm_page_unlock_func((m), LOCK_FILE, > LOCK_LINE) > +#define vm_page_trylock(m) vm_page_trylock_func((m), LOCK_FILE, > LOCK_LINE) > +#ifdef INVARIANTS > +#define vm_page_lock_assert(m, a) \ > + vm_page_lock_assert_func((m), (a), LOCK_FILE, LOCK_LINE) > +#else > +#define vm_page_lock_assert(m, a) > +#endif > + > +#define vm_page_read_dirty(m) vm_page_read_dirty_func((m)) > +#define vm_page_read_valid(m) vm_page_read_valid_func((m)) > +#define vm_page_write_valid(m, v) vm_page_write_valid_func((m), > (v)) > + > +#else /* KLD_MODULE */ > #define vm_page_lockptr(m) (PA_LOCKPTR(VM_PAGE_TO_PHYS((m > #define vm_page_lock(m) mtx_lock(vm_page_lockptr((m))) > #define vm_page_unlock(m) mtx_unlock(vm_page_lockptr
vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
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) * can only occur at the file EOF. */ VM_OBJECT_LOCK(object); - if (pages[ap->a_reqpage]->valid != 0) { + if (vm_page_read_valid(pages[ap->a_reqpage]) != 0) { for (i = 0; i < npages; ++i) { if (i != ap->a_reqpage) { vm_page_lock(pages[i]); @@ -198,16 +198,16 @@ nfs_getpages(struct vop_getpages_args *ap) /* * Read operation filled an entire page */ - m->valid = VM_PAGE_BITS_ALL; - KASSERT(m->dirty == 0, + vm_page_write_valid(m, VM_PAGE_BITS_ALL); + KASSERT(vm_page_read_dirty(m) == 0, ("nfs_getpages: page %p is dirty", m)); } else if (size > toff) { /* * Read operation filled a partial page. */ - m->valid = 0; + vm_page_write_valid(m, 0); vm_page_set_valid(m, 0, size - toff); - KASSERT(m->dirty == 0, + KASSERT(vm_page_read_dirty(m) == 0, ("nfs_getpages: page %p is dirty", m)); } else { /* 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) vm_page_dirty(m); } +void +vm_page_lock_func(vm_page_t m, const char *file, int line) +{ + +#if LOCK_DEBUG > 0 || defined(MUTEX_NOINLINE) + _mtx_lock_flags(vm_page_lockptr(m), 0, file, line); +#else + __mtx_lock(vm_page_lockptr(m), 0, file, line); +#endif +} + +void +vm_page_unlock_func(vm_page_t m, const char *file, int line) +{ + +#if LOCK_DEBUG > 0 || defined(MUTEX_NOINLINE) + _mtx_unlock_flags(vm_page_lockptr(m), 0, file, line); +#else + __mtx_unlock(vm_page_lockptr(m), curthread, 0, file, line); +#endif +} + +int +vm_page_trylock_func(vm_page_t m, const char *file, int line) +{ + + return (_mtx_trylock(vm_page_lockptr(m), 0, file, line)); +} + +void +vm_page_lock_assert_func(vm_page_t m, int a, const char *file, int line) +{ + +#ifdef INVARIANTS + _mtx_assert(vm_page_lockptr(m), a, file, line); +#endif +} + +vm_page_bits_t +vm_page_read_dirty_func(vm_page_t m) +{ + + return (m->dirty); +} + +vm_page_bits_t +vm_page_read_valid_func(vm_page_t m) +{ + + return (m->valid); +} + +void +vm_page_write_valid_func(vm_page_t m, vm_page_bits_t v) +{ + + m->valid = v; +} + + int so_zerocp_fullpage = 0; /* diff --git a/sys/vm/vm_page.h b/sys/vm/vm_page.h index 7099b70..4f8f71e 100644 --- a/sys/vm/vm_page.h +++ b/sys/vm/vm_page.h @@ -218,12 +218,50 @@ extern struct vpglocks pa_lock[]; #definePA_LOCK_ASSERT(pa, a) mtx_assert(PA_LOCKPTR(pa), (a)) +#ifdef KLD_MODULE +#definevm_page_lock(m) vm_page_lock_func((m), LOCK_FILE, LOCK_LINE) +#definevm_page_unlock(m) vm_page_unlock_func((m), LOCK_FILE, LOCK_LINE) +#definevm_page_trylock(m) vm_page_trylock_func((m), LOCK_FILE, LOCK_LINE) +#ifdef INVARIANTS +#definevm_page_lock_assert(m, a) \ +vm_page_lock_assert_func((m), (a), LOCK_FILE, LOCK_LINE) +#else +#definevm_page_lock_assert(m, a) +#endif + +#definevm_page_read_dirty(m) vm_page_read_dirty_func((m)) +#definevm_page_read_valid(m) vm_page_read_valid_func((m)) +#definevm_page_write_valid(m, v) vm_page_write_valid_func((m), (v)) + +#else /* KLD_MODULE */ #definevm_page_lockptr(m) (PA_LOCKPTR(VM_PAGE_TO_PHYS((m #definevm_page_lock(m) mtx_lock(vm_page_lockptr((m))) #definevm_page_unlock(m) mtx_unlock(vm_page_lockptr((m))) #definevm_page_trylock(m) mtx_trylock(vm_page_lockptr((m))) #definevm_page_lock_assert(m, a) mtx_assert(vm_page_lockptr((m)), (a)) +static inline vm_page_bits_t +vm_page_read_dirty(vm_page_t m) +{ + + return (m->dirty); +} + +static inline vm_page_bits_t +vm_page_read_valid(vm_page_t m) +{ + + return (m->valid); +} + +static inline void +vm_page_write_valid(vm_page_t m, vm_page_bits_t v) +{ + + m->valid = v; +} +#endif + #definevm_page_queue_free_mtx vm_page_queue_free_lock.data /* * These are the flags defined
Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3
On Fri, Nov 04, 2011 at 10:48:45AM -0500, Alan Cox wrote: > On 11/04/2011 10:30, Kostik Belousov wrote: > > for (b = i = 0; i<= PAGE_SIZE / DEV_BSIZE; ++i) { > > if (i == (PAGE_SIZE / DEV_BSIZE) || > >-(m->valid& (1<< i)) > >+(m->valid& ((vm_page_bits_t)1<< i)) > > ) { > > if (i> b) { > > pmap_zero_page_area(m, > > While we're here, we might as well fix the old style bug above by moving > the ") {" to the proper place. Fixed, this does not warrant the universe restart. pgpxPx0PIpYaC.pgp Description: PGP signature
Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3
On 11/04/2011 10:30, Kostik Belousov wrote: On Fri, Nov 04, 2011 at 10:09:09AM -0500, Alan Cox wrote: On 11/04/2011 05:08, Kostik Belousov wrote: On Thu, Nov 03, 2011 at 12:51:10PM -0500, Alan Cox wrote: I would suggest introducing the vm_page_bits_t change first. If, at the same time, you change the return type from the function vm_page_bits() to use vm_page_bits_t, then I believe it is straightforward to fix all of the places in vm_page.c that don't properly handle a 32 KB page size. Ok, I think this is orhtohonal to the ABI issue. The vm_page_bits_t applied. Agreed, which is why I wanted to separate the two things. I've made a few comments below. ... Looks good. I will make universe the patch below. Any further notes ? Only one. See below. diff --git a/sys/vm/vm_page.c b/sys/vm/vm_page.c index f14da4a..f398453 100644 --- a/sys/vm/vm_page.c +++ b/sys/vm/vm_page.c @@ -137,7 +137,7 @@ SYSCTL_INT(_vm, OID_AUTO, tryrelock_restart, CTLFLAG_RD, static uma_zone_t fakepg_zone; -static void vm_page_clear_dirty_mask(vm_page_t m, int pagebits); +static void vm_page_clear_dirty_mask(vm_page_t m, vm_page_bits_t pagebits); static void vm_page_queue_remove(int queue, vm_page_t m); static void vm_page_enqueue(int queue, vm_page_t m); static void vm_page_init_fakepg(void *dummy); @@ -2350,7 +2350,7 @@ retrylookup: * * Inputs are required to range within a page. */ -int +vm_page_bits_t vm_page_bits(int base, int size) { int first_bit; @@ -2367,7 +2367,8 @@ vm_page_bits(int base, int size) first_bit = base>> DEV_BSHIFT; last_bit = (base + size - 1)>> DEV_BSHIFT; - return ((2<< last_bit) - (1<< first_bit)); + return (((vm_page_bits_t)2<< last_bit) - + ((vm_page_bits_t)1<< first_bit)); } /* @@ -2426,7 +2427,7 @@ vm_page_set_valid(vm_page_t m, int base, int size) * Clear the given bits from the specified page's dirty field. */ static __inline void -vm_page_clear_dirty_mask(vm_page_t m, int pagebits) +vm_page_clear_dirty_mask(vm_page_t m, vm_page_bits_t pagebits) { uintptr_t addr; #if PAGE_SIZE< 16384 @@ -2455,7 +2456,6 @@ vm_page_clear_dirty_mask(vm_page_t m, int pagebits) */ addr = (uintptr_t)&m->dirty; #if PAGE_SIZE == 32768 -#error pagebits too short atomic_clear_64((uint64_t *)addr, pagebits); #elif PAGE_SIZE == 16384 atomic_clear_32((uint32_t *)addr, pagebits); @@ -2492,8 +2492,8 @@ vm_page_clear_dirty_mask(vm_page_t m, int pagebits) void vm_page_set_validclean(vm_page_t m, int base, int size) { - u_long oldvalid; - int endoff, frag, pagebits; + vm_page_bits_t oldvalid, pagebits; + int endoff, frag; VM_OBJECT_LOCK_ASSERT(m->object, MA_OWNED); if (size == 0) /* handle degenerate case */ @@ -2505,7 +2505,7 @@ vm_page_set_validclean(vm_page_t m, int base, int size) * first block. */ if ((frag = base& ~(DEV_BSIZE - 1)) != base&& - (m->valid& (1<< (base>> DEV_BSHIFT))) == 0) + (m->valid& ((vm_page_bits_t)1<< (base>> DEV_BSHIFT))) == 0) pmap_zero_page_area(m, frag, base - frag); /* @@ -2515,7 +2515,7 @@ vm_page_set_validclean(vm_page_t m, int base, int size) */ endoff = base + size; if ((frag = endoff& ~(DEV_BSIZE - 1)) != endoff&& - (m->valid& (1<< (endoff>> DEV_BSHIFT))) == 0) + (m->valid& ((vm_page_bits_t)1<< (endoff>> DEV_BSHIFT))) == 0) pmap_zero_page_area(m, endoff, DEV_BSIZE - (endoff& (DEV_BSIZE - 1))); @@ -2585,7 +2585,7 @@ vm_page_clear_dirty(vm_page_t m, int base, int size) void vm_page_set_invalid(vm_page_t m, int base, int size) { - int bits; + vm_page_bits_t bits; VM_OBJECT_LOCK_ASSERT(m->object, MA_OWNED); KASSERT((m->oflags& VPO_BUSY) == 0, @@ -2625,7 +2625,7 @@ vm_page_zero_invalid(vm_page_t m, boolean_t setvalid) */ for (b = i = 0; i<= PAGE_SIZE / DEV_BSIZE; ++i) { if (i == (PAGE_SIZE / DEV_BSIZE) || - (m->valid& (1<< i)) + (m->valid& ((vm_page_bits_t)1<< i)) ) { if (i> b) { pmap_zero_page_area(m, While we're here, we might as well fix the old style bug above by moving the ") {" to the proper place. @@ -2656,9 +2656,10 @@ vm_page_zero_invalid(vm_page_t m, boolean_t setvalid) int vm_page_is_valid(vm_page_t m, int base, int size) { - int bits = vm_page_bits(base, size); + vm_page_bits_t bits; VM_OBJECT_LOCK_ASSERT(m->object, MA_OWNED); + bits = vm_page_bits(base, size); if (m->valid&& ((m->valid& bits) == bits)) return 1; else diff --git a/sys/vm/vm_page.h b/sys/vm/vm_page.h index 23637bb..e3eb08c 100644 --- a/sys/vm/vm_page.h +++ b/sys/vm/vm_page.h @@ -113,6 +11
Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3
On Fri, Nov 04, 2011 at 10:09:09AM -0500, Alan Cox wrote: > On 11/04/2011 05:08, Kostik Belousov wrote: > >On Thu, Nov 03, 2011 at 12:51:10PM -0500, Alan Cox wrote: > >>I would suggest introducing the vm_page_bits_t change first. If, at the > >>same time, you change the return type from the function vm_page_bits() > >>to use vm_page_bits_t, then I believe it is straightforward to fix all > >>of the places in vm_page.c that don't properly handle a 32 KB page size. > >Ok, I think this is orhtohonal to the ABI issue. The vm_page_bits_t > >applied. > > Agreed, which is why I wanted to separate the two things. > > I've made a few comments below. ... > Looks good. I will make universe the patch below. Any further notes ? diff --git a/sys/vm/vm_page.c b/sys/vm/vm_page.c index f14da4a..f398453 100644 --- a/sys/vm/vm_page.c +++ b/sys/vm/vm_page.c @@ -137,7 +137,7 @@ SYSCTL_INT(_vm, OID_AUTO, tryrelock_restart, CTLFLAG_RD, static uma_zone_t fakepg_zone; -static void vm_page_clear_dirty_mask(vm_page_t m, int pagebits); +static void vm_page_clear_dirty_mask(vm_page_t m, vm_page_bits_t pagebits); static void vm_page_queue_remove(int queue, vm_page_t m); static void vm_page_enqueue(int queue, vm_page_t m); static void vm_page_init_fakepg(void *dummy); @@ -2350,7 +2350,7 @@ retrylookup: * * Inputs are required to range within a page. */ -int +vm_page_bits_t vm_page_bits(int base, int size) { int first_bit; @@ -2367,7 +2367,8 @@ vm_page_bits(int base, int size) first_bit = base >> DEV_BSHIFT; last_bit = (base + size - 1) >> DEV_BSHIFT; - return ((2 << last_bit) - (1 << first_bit)); + return (((vm_page_bits_t)2 << last_bit) - + ((vm_page_bits_t)1 << first_bit)); } /* @@ -2426,7 +2427,7 @@ vm_page_set_valid(vm_page_t m, int base, int size) * Clear the given bits from the specified page's dirty field. */ static __inline void -vm_page_clear_dirty_mask(vm_page_t m, int pagebits) +vm_page_clear_dirty_mask(vm_page_t m, vm_page_bits_t pagebits) { uintptr_t addr; #if PAGE_SIZE < 16384 @@ -2455,7 +2456,6 @@ vm_page_clear_dirty_mask(vm_page_t m, int pagebits) */ addr = (uintptr_t)&m->dirty; #if PAGE_SIZE == 32768 -#error pagebits too short atomic_clear_64((uint64_t *)addr, pagebits); #elif PAGE_SIZE == 16384 atomic_clear_32((uint32_t *)addr, pagebits); @@ -2492,8 +2492,8 @@ vm_page_clear_dirty_mask(vm_page_t m, int pagebits) void vm_page_set_validclean(vm_page_t m, int base, int size) { - u_long oldvalid; - int endoff, frag, pagebits; + vm_page_bits_t oldvalid, pagebits; + int endoff, frag; VM_OBJECT_LOCK_ASSERT(m->object, MA_OWNED); if (size == 0) /* handle degenerate case */ @@ -2505,7 +2505,7 @@ vm_page_set_validclean(vm_page_t m, int base, int size) * first block. */ if ((frag = base & ~(DEV_BSIZE - 1)) != base && - (m->valid & (1 << (base >> DEV_BSHIFT))) == 0) + (m->valid & ((vm_page_bits_t)1 << (base >> DEV_BSHIFT))) == 0) pmap_zero_page_area(m, frag, base - frag); /* @@ -2515,7 +2515,7 @@ vm_page_set_validclean(vm_page_t m, int base, int size) */ endoff = base + size; if ((frag = endoff & ~(DEV_BSIZE - 1)) != endoff && - (m->valid & (1 << (endoff >> DEV_BSHIFT))) == 0) + (m->valid & ((vm_page_bits_t)1 << (endoff >> DEV_BSHIFT))) == 0) pmap_zero_page_area(m, endoff, DEV_BSIZE - (endoff & (DEV_BSIZE - 1))); @@ -2585,7 +2585,7 @@ vm_page_clear_dirty(vm_page_t m, int base, int size) void vm_page_set_invalid(vm_page_t m, int base, int size) { - int bits; + vm_page_bits_t bits; VM_OBJECT_LOCK_ASSERT(m->object, MA_OWNED); KASSERT((m->oflags & VPO_BUSY) == 0, @@ -2625,7 +2625,7 @@ vm_page_zero_invalid(vm_page_t m, boolean_t setvalid) */ for (b = i = 0; i <= PAGE_SIZE / DEV_BSIZE; ++i) { if (i == (PAGE_SIZE / DEV_BSIZE) || - (m->valid & (1 << i)) + (m->valid & ((vm_page_bits_t)1 << i)) ) { if (i > b) { pmap_zero_page_area(m, @@ -2656,9 +2656,10 @@ vm_page_zero_invalid(vm_page_t m, boolean_t setvalid) int vm_page_is_valid(vm_page_t m, int base, int size) { - int bits = vm_page_bits(base, size); + vm_page_bits_t bits; VM_OBJECT_LOCK_ASSERT(m->object, MA_OWNED); + bits = vm_page_bits(base, size); if (m->valid && ((m->valid & bits) == bits)) return 1; else diff --git a/sys/vm/vm_page.h b/sys/vm/vm_page.h index 23637bb..e3eb08c 100644 --- a/sys/vm/vm_page.h +++ b/sys/vm/vm_page.h @@ -113,6 +113,20 @@ TAILQ_HEAD(pglist, vm_page); +#if PAGE_SIZE == 4096 +#define VM_PAGE_BITS_ALL 0xffu +typedef uint8_t vm_page_bits_t; +#elif PAGE_SIZE == 8192 +#define
Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3
On 11/04/2011 05:08, Kostik Belousov wrote: On Thu, Nov 03, 2011 at 12:51:10PM -0500, Alan Cox wrote: On 11/03/2011 08:24, Kostik Belousov wrote: On Thu, Nov 03, 2011 at 12:40:08AM -0500, Alan Cox wrote: On 11/02/2011 05:32, Andriy Gapon wrote: [restored cc: to the original poster] As Bruce Evans has pointed to me privately [I am not sure why privately], there is already an example in i386 and amd64 atomic.h, where operations are inlined for a kernel build, but presented as real (external) functions for a module build. You can search e.g. sys/amd64/include/atomic.h for KLD_MODULE. I think that the same treatment could/should be applied to vm_page_*lock* operations defined in sys/vm/vm_page.h. *snip* Yes, it should be. There are without question legitimate reasons for a module to acquire a page lock. I agree. Also, I think that we should use the opportunity to also isolate the modules from the struct vm_page layout changes. As example, I converted nfsclient.ko. I would suggest introducing the vm_page_bits_t change first. If, at the same time, you change the return type from the function vm_page_bits() to use vm_page_bits_t, then I believe it is straightforward to fix all of the places in vm_page.c that don't properly handle a 32 KB page size. Ok, I think this is orhtohonal to the ABI issue. The vm_page_bits_t applied. Agreed, which is why I wanted to separate the two things. I've made a few comments below. diff --git a/sys/vm/vm_page.c b/sys/vm/vm_page.c index f14da4a..f22c34a 100644 --- a/sys/vm/vm_page.c +++ b/sys/vm/vm_page.c @@ -137,7 +137,7 @@ SYSCTL_INT(_vm, OID_AUTO, tryrelock_restart, CTLFLAG_RD, static uma_zone_t fakepg_zone; -static void vm_page_clear_dirty_mask(vm_page_t m, int pagebits); +static void vm_page_clear_dirty_mask(vm_page_t m, vm_page_bits_t pagebits); static void vm_page_queue_remove(int queue, vm_page_t m); static void vm_page_enqueue(int queue, vm_page_t m); static void vm_page_init_fakepg(void *dummy); @@ -2350,7 +2350,7 @@ retrylookup: * * Inputs are required to range within a page. */ -int +vm_page_bits_t vm_page_bits(int base, int size) { int first_bit; @@ -2367,7 +2367,8 @@ vm_page_bits(int base, int size) first_bit = base>> DEV_BSHIFT; last_bit = (base + size - 1)>> DEV_BSHIFT; - return ((2<< last_bit) - (1<< first_bit)); + return (((vm_page_bits_t)2<< last_bit) - + ((vm_page_bits_t)1<< first_bit)); } /* @@ -2426,7 +2427,7 @@ vm_page_set_valid(vm_page_t m, int base, int size) * Clear the given bits from the specified page's dirty field. */ static __inline void -vm_page_clear_dirty_mask(vm_page_t m, int pagebits) +vm_page_clear_dirty_mask(vm_page_t m, vm_page_bits_t pagebits) { uintptr_t addr; #if PAGE_SIZE< 16384 @@ -2455,7 +2456,6 @@ vm_page_clear_dirty_mask(vm_page_t m, int pagebits) */ addr = (uintptr_t)&m->dirty; #if PAGE_SIZE == 32768 -#error pagebits too short atomic_clear_64((uint64_t *)addr, pagebits); #elif PAGE_SIZE == 16384 atomic_clear_32((uint32_t *)addr, pagebits); @@ -2492,8 +2492,8 @@ vm_page_clear_dirty_mask(vm_page_t m, int pagebits) void vm_page_set_validclean(vm_page_t m, int base, int size) { - u_long oldvalid; - int endoff, frag, pagebits; + vm_page_bits_t oldvalid, pagebits; + int endoff, frag; VM_OBJECT_LOCK_ASSERT(m->object, MA_OWNED); if (size == 0) /* handle degenerate case */ @@ -2505,7 +2505,7 @@ vm_page_set_validclean(vm_page_t m, int base, int size) * first block. */ if ((frag = base& ~(DEV_BSIZE - 1)) != base&& - (m->valid& (1<< (base>> DEV_BSHIFT))) == 0) + (m->valid& ((vm_page_bits_t)1<< (base>> DEV_BSHIFT))) == 0) pmap_zero_page_area(m, frag, base - frag); /* @@ -2515,7 +2515,7 @@ vm_page_set_validclean(vm_page_t m, int base, int size) */ endoff = base + size; if ((frag = endoff& ~(DEV_BSIZE - 1)) != endoff&& - (m->valid& (1<< (endoff>> DEV_BSHIFT))) == 0) + (m->valid& ((vm_page_bits_t)1<< (endoff>> DEV_BSHIFT))) == 0) pmap_zero_page_area(m, endoff, DEV_BSIZE - (endoff& (DEV_BSIZE - 1))); @@ -2585,7 +2585,7 @@ vm_page_clear_dirty(vm_page_t m, int base, int size) void vm_page_set_invalid(vm_page_t m, int base, int size) { - int bits; + vm_page_bits_t bits; VM_OBJECT_LOCK_ASSERT(m->object, MA_OWNED); KASSERT((m->oflags& VPO_BUSY) == 0, I believe that a cast is still needed in vm_page_zero_invalid(): (m->valid & (1 << i)) @@ -2656,9 +2656,10 @@ vm_page_zero_invalid(vm_page_t m, boolean_t setvalid) int vm_page_is_valid(vm_page_t m, int base, int size) { - int bits = vm_page_bits(base, size); + vm_page_bits_t bits; VM_OBJECT_LOCK_ASSERT(m->object, MA_OWNED); +
Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3
On Thu, Nov 03, 2011 at 12:51:10PM -0500, Alan Cox wrote: > On 11/03/2011 08:24, Kostik Belousov wrote: > >On Thu, Nov 03, 2011 at 12:40:08AM -0500, Alan Cox wrote: > >>On 11/02/2011 05:32, Andriy Gapon wrote: > >>>[restored cc: to the original poster] > >>>As Bruce Evans has pointed to me privately [I am not sure why privately], > >>>there > >>>is already an example in i386 and amd64 atomic.h, where operations are > >>>inlined > >>>for a kernel build, but presented as real (external) functions for a > >>>module > >>>build. You can search e.g. sys/amd64/include/atomic.h for KLD_MODULE. > >>> > >>>I think that the same treatment could/should be applied to vm_page_*lock* > >>>operations defined in sys/vm/vm_page.h. > >>*snip* > >> > >>Yes, it should be. There are without question legitimate reasons for a > >>module to acquire a page lock. > >I agree. Also, I think that we should use the opportunity to also isolate > >the modules from the struct vm_page layout changes. As example, I converted > >nfsclient.ko. > > > > I would suggest introducing the vm_page_bits_t change first. If, at the > same time, you change the return type from the function vm_page_bits() > to use vm_page_bits_t, then I believe it is straightforward to fix all > of the places in vm_page.c that don't properly handle a 32 KB page size. Ok, I think this is orhtohonal to the ABI issue. The vm_page_bits_t applied. diff --git a/sys/vm/vm_page.c b/sys/vm/vm_page.c index f14da4a..f22c34a 100644 --- a/sys/vm/vm_page.c +++ b/sys/vm/vm_page.c @@ -137,7 +137,7 @@ SYSCTL_INT(_vm, OID_AUTO, tryrelock_restart, CTLFLAG_RD, static uma_zone_t fakepg_zone; -static void vm_page_clear_dirty_mask(vm_page_t m, int pagebits); +static void vm_page_clear_dirty_mask(vm_page_t m, vm_page_bits_t pagebits); static void vm_page_queue_remove(int queue, vm_page_t m); static void vm_page_enqueue(int queue, vm_page_t m); static void vm_page_init_fakepg(void *dummy); @@ -2350,7 +2350,7 @@ retrylookup: * * Inputs are required to range within a page. */ -int +vm_page_bits_t vm_page_bits(int base, int size) { int first_bit; @@ -2367,7 +2367,8 @@ vm_page_bits(int base, int size) first_bit = base >> DEV_BSHIFT; last_bit = (base + size - 1) >> DEV_BSHIFT; - return ((2 << last_bit) - (1 << first_bit)); + return (((vm_page_bits_t)2 << last_bit) - + ((vm_page_bits_t)1 << first_bit)); } /* @@ -2426,7 +2427,7 @@ vm_page_set_valid(vm_page_t m, int base, int size) * Clear the given bits from the specified page's dirty field. */ static __inline void -vm_page_clear_dirty_mask(vm_page_t m, int pagebits) +vm_page_clear_dirty_mask(vm_page_t m, vm_page_bits_t pagebits) { uintptr_t addr; #if PAGE_SIZE < 16384 @@ -2455,7 +2456,6 @@ vm_page_clear_dirty_mask(vm_page_t m, int pagebits) */ addr = (uintptr_t)&m->dirty; #if PAGE_SIZE == 32768 -#error pagebits too short atomic_clear_64((uint64_t *)addr, pagebits); #elif PAGE_SIZE == 16384 atomic_clear_32((uint32_t *)addr, pagebits); @@ -2492,8 +2492,8 @@ vm_page_clear_dirty_mask(vm_page_t m, int pagebits) void vm_page_set_validclean(vm_page_t m, int base, int size) { - u_long oldvalid; - int endoff, frag, pagebits; + vm_page_bits_t oldvalid, pagebits; + int endoff, frag; VM_OBJECT_LOCK_ASSERT(m->object, MA_OWNED); if (size == 0) /* handle degenerate case */ @@ -2505,7 +2505,7 @@ vm_page_set_validclean(vm_page_t m, int base, int size) * first block. */ if ((frag = base & ~(DEV_BSIZE - 1)) != base && - (m->valid & (1 << (base >> DEV_BSHIFT))) == 0) + (m->valid & ((vm_page_bits_t)1 << (base >> DEV_BSHIFT))) == 0) pmap_zero_page_area(m, frag, base - frag); /* @@ -2515,7 +2515,7 @@ vm_page_set_validclean(vm_page_t m, int base, int size) */ endoff = base + size; if ((frag = endoff & ~(DEV_BSIZE - 1)) != endoff && - (m->valid & (1 << (endoff >> DEV_BSHIFT))) == 0) + (m->valid & ((vm_page_bits_t)1 << (endoff >> DEV_BSHIFT))) == 0) pmap_zero_page_area(m, endoff, DEV_BSIZE - (endoff & (DEV_BSIZE - 1))); @@ -2585,7 +2585,7 @@ vm_page_clear_dirty(vm_page_t m, int base, int size) void vm_page_set_invalid(vm_page_t m, int base, int size) { - int bits; + vm_page_bits_t bits; VM_OBJECT_LOCK_ASSERT(m->object, MA_OWNED); KASSERT((m->oflags & VPO_BUSY) == 0, @@ -2656,9 +2656,10 @@ vm_page_zero_invalid(vm_page_t m, boolean_t setvalid) int vm_page_is_valid(vm_page_t m, int base, int size) { - int bits = vm_page_bits(base, size); + vm_page_bits_t bits; VM_OBJECT_LOCK_ASSERT(m->object, MA_OWNED); + bits = vm_page_bits(base, size); if (m->valid && ((m->valid & bits) == bits)) return 1; else diff --git a/sys/vm/vm_page.h b/sys/v
Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3
On 11/03/2011 08:24, Kostik Belousov wrote: On Thu, Nov 03, 2011 at 12:40:08AM -0500, Alan Cox wrote: On 11/02/2011 05:32, Andriy Gapon wrote: [restored cc: to the original poster] As Bruce Evans has pointed to me privately [I am not sure why privately], there is already an example in i386 and amd64 atomic.h, where operations are inlined for a kernel build, but presented as real (external) functions for a module build. You can search e.g. sys/amd64/include/atomic.h for KLD_MODULE. I think that the same treatment could/should be applied to vm_page_*lock* operations defined in sys/vm/vm_page.h. *snip* Yes, it should be. There are without question legitimate reasons for a module to acquire a page lock. I agree. Also, I think that we should use the opportunity to also isolate the modules from the struct vm_page layout changes. As example, I converted nfsclient.ko. I would suggest introducing the vm_page_bits_t change first. If, at the same time, you change the return type from the function vm_page_bits() to use vm_page_bits_t, then I believe it is straightforward to fix all of the places in vm_page.c that don't properly handle a 32 KB page size. Alan Patch is not tested. 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) * can only occur at the file EOF. */ VM_OBJECT_LOCK(object); - if (pages[ap->a_reqpage]->valid != 0) { + if (vm_page_read_valid(pages[ap->a_reqpage]) != 0) { for (i = 0; i< npages; ++i) { if (i != ap->a_reqpage) { vm_page_lock(pages[i]); @@ -198,16 +198,16 @@ nfs_getpages(struct vop_getpages_args *ap) /* * Read operation filled an entire page */ - m->valid = VM_PAGE_BITS_ALL; - KASSERT(m->dirty == 0, + vm_page_write_valid(m, VM_PAGE_BITS_ALL); + KASSERT(vm_page_read_dirty(m) == 0, ("nfs_getpages: page %p is dirty", m)); } else if (size> toff) { /* * Read operation filled a partial page. */ - m->valid = 0; + vm_page_write_valid(m, 0); vm_page_set_valid(m, 0, size - toff); - KASSERT(m->dirty == 0, + KASSERT(vm_page_read_dirty(m) == 0, ("nfs_getpages: page %p is dirty", m)); } else { /* diff --git a/sys/vm/vm_page.c b/sys/vm/vm_page.c index f14da4a..5b8b4e3 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) vm_page_dirty(m); } +void +vm_page_lock_func(vm_page_t m, const char *file, int line) +{ + +#if LOCK_DEBUG> 0 || defined(MUTEX_NOINLINE) + _mtx_lock_flags(vm_page_lockptr(m), 0, file, line); +#else + __mtx_lock(vm_page_lockptr(m), 0, file, line); +#endif +} + +void +vm_page_unlock_func(vm_page_t m, const char *file, int line) +{ + +#if LOCK_DEBUG> 0 || defined(MUTEX_NOINLINE) + _mtx_unlock_flags(vm_page_lockptr(m), 0, file, line); +#else + __mtx_unlock(vm_page_lockptr(m), curthread, 0, file, line); +#endif +} + +int +vm_page_trylock_func(vm_page_t m, const char *file, int line) +{ + + return (_mtx_trylock(vm_page_lockptr(m), 0, file, line)); +} + +void +vm_page_lock_assert_func(vm_page_t m, int a, const char *file, int line) +{ + +#ifdef INVARIANTS + _mtx_assert(vm_page_lockptr(m), a, file, line); +#endif +} + +vm_page_bits_t +vm_page_read_dirty_func(vm_page_t m) +{ + + return (m->dirty); +} + +vm_page_bits_t +vm_page_read_valid_func(vm_page_t m) +{ + + return (m->valid); +} + +void +vm_page_write_valid_func(vm_page_t m, vm_page_bits_t v) +{ + + m->valid = v; +} + + int so_zerocp_fullpage = 0; /* diff --git a/sys/vm/vm_page.h b/sys/vm/vm_page.h index 23637bb..618ba2b 100644 --- a/sys/vm/vm_page.h +++ b/sys/vm/vm_page.h @@ -113,6 +113,21 @@ TAILQ_HEAD(pglist, vm_page); +#if PAGE_SIZE == 4096 +#define VM_PAGE_BITS_ALL 0xffu +typedef uint8_t vm_page_bits_t; +#elif PAGE_SIZE == 8192 +#define VM_PAGE_BITS_ALL 0xu +typedef uint16_t vm_page_bits_t; +#elif PAGE_SIZE == 16384 +#define VM_PAGE_BITS_ALL 0xu +typedef uint32_t vm_page_bits_t; +#elif PAGE_SIZE == 32768 +#define VM_PAGE_BITS_ALL 0xlu +typedef uint64_t vm_page_bits_t; +#endif + + struct vm_page { TAILQ_ENTRY(vm_page) pageq; /* queue info for FIFO queue or free list (Q) */ TAILQ_ENTRY(vm_page) listq; /* pages in same object (O) */ @@ -138,19 +153,8 @@ struct vm_page { /* NOTE that these must sup
Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3
On Thu, Nov 03, 2011 at 12:40:08AM -0500, Alan Cox wrote: > On 11/02/2011 05:32, Andriy Gapon wrote: > >[restored cc: to the original poster] > >As Bruce Evans has pointed to me privately [I am not sure why privately], > >there > >is already an example in i386 and amd64 atomic.h, where operations are > >inlined > >for a kernel build, but presented as real (external) functions for a module > >build. You can search e.g. sys/amd64/include/atomic.h for KLD_MODULE. > > > >I think that the same treatment could/should be applied to vm_page_*lock* > >operations defined in sys/vm/vm_page.h. > *snip* > > Yes, it should be. There are without question legitimate reasons for a > module to acquire a page lock. I agree. Also, I think that we should use the opportunity to also isolate the modules from the struct vm_page layout changes. As example, I converted nfsclient.ko. Patch is not tested. 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) * can only occur at the file EOF. */ VM_OBJECT_LOCK(object); - if (pages[ap->a_reqpage]->valid != 0) { + if (vm_page_read_valid(pages[ap->a_reqpage]) != 0) { for (i = 0; i < npages; ++i) { if (i != ap->a_reqpage) { vm_page_lock(pages[i]); @@ -198,16 +198,16 @@ nfs_getpages(struct vop_getpages_args *ap) /* * Read operation filled an entire page */ - m->valid = VM_PAGE_BITS_ALL; - KASSERT(m->dirty == 0, + vm_page_write_valid(m, VM_PAGE_BITS_ALL); + KASSERT(vm_page_read_dirty(m) == 0, ("nfs_getpages: page %p is dirty", m)); } else if (size > toff) { /* * Read operation filled a partial page. */ - m->valid = 0; + vm_page_write_valid(m, 0); vm_page_set_valid(m, 0, size - toff); - KASSERT(m->dirty == 0, + KASSERT(vm_page_read_dirty(m) == 0, ("nfs_getpages: page %p is dirty", m)); } else { /* diff --git a/sys/vm/vm_page.c b/sys/vm/vm_page.c index f14da4a..5b8b4e3 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) vm_page_dirty(m); } +void +vm_page_lock_func(vm_page_t m, const char *file, int line) +{ + +#if LOCK_DEBUG > 0 || defined(MUTEX_NOINLINE) + _mtx_lock_flags(vm_page_lockptr(m), 0, file, line); +#else + __mtx_lock(vm_page_lockptr(m), 0, file, line); +#endif +} + +void +vm_page_unlock_func(vm_page_t m, const char *file, int line) +{ + +#if LOCK_DEBUG > 0 || defined(MUTEX_NOINLINE) + _mtx_unlock_flags(vm_page_lockptr(m), 0, file, line); +#else + __mtx_unlock(vm_page_lockptr(m), curthread, 0, file, line); +#endif +} + +int +vm_page_trylock_func(vm_page_t m, const char *file, int line) +{ + + return (_mtx_trylock(vm_page_lockptr(m), 0, file, line)); +} + +void +vm_page_lock_assert_func(vm_page_t m, int a, const char *file, int line) +{ + +#ifdef INVARIANTS + _mtx_assert(vm_page_lockptr(m), a, file, line); +#endif +} + +vm_page_bits_t +vm_page_read_dirty_func(vm_page_t m) +{ + + return (m->dirty); +} + +vm_page_bits_t +vm_page_read_valid_func(vm_page_t m) +{ + + return (m->valid); +} + +void +vm_page_write_valid_func(vm_page_t m, vm_page_bits_t v) +{ + + m->valid = v; +} + + int so_zerocp_fullpage = 0; /* diff --git a/sys/vm/vm_page.h b/sys/vm/vm_page.h index 23637bb..618ba2b 100644 --- a/sys/vm/vm_page.h +++ b/sys/vm/vm_page.h @@ -113,6 +113,21 @@ TAILQ_HEAD(pglist, vm_page); +#if PAGE_SIZE == 4096 +#define VM_PAGE_BITS_ALL 0xffu +typedef uint8_t vm_page_bits_t; +#elif PAGE_SIZE == 8192 +#define VM_PAGE_BITS_ALL 0xu +typedef uint16_t vm_page_bits_t; +#elif PAGE_SIZE == 16384 +#define VM_PAGE_BITS_ALL 0xu +typedef uint32_t vm_page_bits_t; +#elif PAGE_SIZE == 32768 +#define VM_PAGE_BITS_ALL 0xlu +typedef uint64_t vm_page_bits_t; +#endif + + struct vm_page { TAILQ_ENTRY(vm_page) pageq; /* queue info for FIFO queue or free list (Q) */ TAILQ_ENTRY(vm_page) listq; /* pages in same object (O) */ @@ -138,19 +153,8 @@ struct vm_page { /* NOTE that these must support one bit per DEV_BSIZE in a page!!! */ /* so, on normal X86 kernels, they must be at least 8 bits wide */ /* In reality, support for 32KB pages is not fully implemented. */ -#if PAGE_SIZE == 4096 - uint8_t valid; /* map of valid DEV_BSIZE chunks (O) */ - uint8
Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3
On 11/02/2011 05:32, Andriy Gapon wrote: [restored cc: to the original poster] on 02/11/2011 08:10 Benjamin Kaduk said the following: I am perhaps confused. Last I checked, bsd.kmod.mk caused '-include opt_global.h' to be passed on the command line. Is the issue just that the opt_global.h used for the kmod could be different from the actual kernel's opt_global.h, because KERNCONF was not specified and the header is generated at module-build time? In this case, clearly the onus is on the user to pass KERNCONF at module build time. To be precise, this is what is actually passed to a compiler: sys/conf/kmod.mk: .if defined(KERNBUILDDIR) CFLAGS+= -DHAVE_KERNEL_OPTION_HEADERS -include ${KERNBUILDDIR}/opt_global.h .endif where KERNBUILDDIR can be passed via environment from a kernel build: sys/conf/kern.post.mk: MKMODULESENV+= KERNBUILDDIR="${.CURDIR}" SYSDIR="${SYSDIR}" KERNCONF does not have any meaning in a module build. To make sure that a module build sees exactly the same kernel options as a kernel with which the module should work, one has to either build the module together with the kernel (within the kernel build; search for MODULES in make.conf(5)) or to manually specify KERNBUILDDIR to point to a correct kernel build directory. (Which to a certain degree implies impossibility to build a "perfect" module for a pre-built binary kernel or to provide a "perfect" universal pre-built module for any custom kernel) Of course, the real problem is that modules should not care about any (or at least some) kernel options, they should be isolated from the options via a proper KPI/KBI (perhaps DDI or "module-to-kernel interface" or whatever). A module should be able to work correctly with kernels built with different options. As Bruce Evans has pointed to me privately [I am not sure why privately], there is already an example in i386 and amd64 atomic.h, where operations are inlined for a kernel build, but presented as real (external) functions for a module build. You can search e.g. sys/amd64/include/atomic.h for KLD_MODULE. I think that the same treatment could/should be applied to vm_page_*lock* operations defined in sys/vm/vm_page.h. *snip* Yes, it should be. There are without question legitimate reasons for a module to acquire a page lock. Alan ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3
[restored cc: to the original poster] on 02/11/2011 08:10 Benjamin Kaduk said the following: > I am perhaps confused. Last I checked, bsd.kmod.mk caused '-include > opt_global.h' to be passed on the command line. Is the issue just that the > opt_global.h used for the kmod could be different from the actual kernel's > opt_global.h, because KERNCONF was not specified and the header is generated > at > module-build time? In this case, clearly the onus is on the user to pass > KERNCONF at module build time. To be precise, this is what is actually passed to a compiler: sys/conf/kmod.mk: .if defined(KERNBUILDDIR) CFLAGS+= -DHAVE_KERNEL_OPTION_HEADERS -include ${KERNBUILDDIR}/opt_global.h .endif where KERNBUILDDIR can be passed via environment from a kernel build: sys/conf/kern.post.mk: MKMODULESENV+= KERNBUILDDIR="${.CURDIR}" SYSDIR="${SYSDIR}" KERNCONF does not have any meaning in a module build. To make sure that a module build sees exactly the same kernel options as a kernel with which the module should work, one has to either build the module together with the kernel (within the kernel build; search for MODULES in make.conf(5)) or to manually specify KERNBUILDDIR to point to a correct kernel build directory. (Which to a certain degree implies impossibility to build a "perfect" module for a pre-built binary kernel or to provide a "perfect" universal pre-built module for any custom kernel) Of course, the real problem is that modules should not care about any (or at least some) kernel options, they should be isolated from the options via a proper KPI/KBI (perhaps DDI or "module-to-kernel interface" or whatever). A module should be able to work correctly with kernels built with different options. As Bruce Evans has pointed to me privately [I am not sure why privately], there is already an example in i386 and amd64 atomic.h, where operations are inlined for a kernel build, but presented as real (external) functions for a module build. You can search e.g. sys/amd64/include/atomic.h for KLD_MODULE. I think that the same treatment could/should be applied to vm_page_*lock* operations defined in sys/vm/vm_page.h. Or, if the above operations are intended to be used only in the kernel proper, then there should be some guidelines on what API should module writers use to perform certain page manipulations like e.g. wiring a page. P.S. Bruce has also pointed out some other potentially dangerous places with respect to the SMP option and modules build. Most prominent is sys/sys/mutex.h P.P.S. [and tangential] I see that many module makefiles fake up various kernel options in a fashion similar to the following: .if !defined(KERNBUILDDIR) opt_compat.h: echo "#define COMPAT_FREEBSD6 1" > ${.TARGET} opt_kbd.h: echo "#define KBD_INSTALL_CDEV 1" > ${.TARGET} .endif And handful of modules fake up opt_global.h, e.g.: opt_global.h: echo "#define ALTQ 1" > ${.TARGET} -- Andriy Gapon ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3
On Tue, 1 Nov 2011, Penta Upa wrote: Yes that seems to be the problem. It will is for out of tree modules. http://www.freebsd.org/cgi/query-pr.cgi?pr=161887 . I have to verify if moving the module to /usr/src/ tree fixes the problem. Thanks, Penta On Tue, Nov 1, 2011 at 2:04 AM, K. Macy wrote: Someone was seeing the same issue with the vmtools kmod. The only thing that might make sense is that the page lock array is defined as being a different size in your kmod as in the kernel itself so the lock corresponding to the page you're locking differs between the two files. Quoting from the PR, Andriy Gapon wrote: A standalone module build doesn't get some important definitions from kernel config (e.g. via opt_global.h) and can be in a serious disagreement with the kernel. In particular, if a kernel is built with SMP option, but a module build doesn't have SMP defined, then they will have different definitions of PA_LOCK_COUNT and thus would work on different actual locks when manipulating the same page. I am perhaps confused. Last I checked, bsd.kmod.mk caused '-include opt_global.h' to be passed on the command line. Is the issue just that the opt_global.h used for the kmod could be different from the actual kernel's opt_global.h, because KERNCONF was not specified and the header is generated at module-build time? In this case, clearly the onus is on the user to pass KERNCONF at module build time. (I have gotten my laptop to panic in vm_page_free() with the page lock not owned, in OpenAFS' vop_getpages implementation, but I had previously attributed this to having an old -current snapshot on my laptop and openafs sources that were using freebsd major version for API decisions (we're not converted to __FreeBSD_version, yet). If there is a real problem here, I will need to care much more.) Thanks, Ben Kaduk ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3
Yes that seems to be the problem. It will is for out of tree modules. http://www.freebsd.org/cgi/query-pr.cgi?pr=161887 . I have to verify if moving the module to /usr/src/ tree fixes the problem. Thanks, Penta On Tue, Nov 1, 2011 at 2:04 AM, K. Macy wrote: > Someone was seeing the same issue with the vmtools kmod. The only > thing that might make sense is that the page lock array is defined as > being a different size in your kmod as in the kernel itself so the > lock corresponding to the page you're locking differs between the two > files. > > Cheers > > On Fri, Oct 21, 2011 at 5:25 PM, Penta Upa wrote: > > Hi, > > > > I'm facing a kernel panic at vm_page_wire(). Page is locked with > > vm_page_lock() yet i get the following panic > > panic: mutex page lock not owned at /usr/src/sys/vm/vm_page:1845 > > > > Code sequence is as below > > vm_page_lock(pp); > > vm_page_lock_assert(pp, MA_OWNED); /* No panic here */ > > vm_page_wire(pp); /* Panic here for the same assertion as above, strange > */ > > vm_page_unlock(pp); > > > > Kernel on the system is unchanged after install. The only thing which > > occurred out the way was that the first time install failed for checksum > > mismatch for src.txz. Also there were some SCSI errors/warnings with the > CD > > drive during install. So the next time, i only installed the base and > > kernel and later unpacked src.txz under /usr/src . Could this lead to any > > issues ? > > > > Attached is a test module (vmtest) and the makefile used. Uname output > from > > the system is > > > > FreeBSD scache 9.0-BETA3 FreeBSD 9.0-BETA3 #0: Sat Sep 24 21:31:28 UTC > > 2011 > > r...@farrell.cse.buffalo.edu:/usr/obj/usr/src/sys/GENERIC amd64 > > > > Is there anything i'm doing wrong here ? Kindly help. > > > > Regards, > > Penta > > > > ___ > > freebsd-current@freebsd.org mailing list > > http://lists.freebsd.org/mailman/listinfo/freebsd-current > > To unsubscribe, send any mail to " > freebsd-current-unsubscr...@freebsd.org" > > > ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3
Someone was seeing the same issue with the vmtools kmod. The only thing that might make sense is that the page lock array is defined as being a different size in your kmod as in the kernel itself so the lock corresponding to the page you're locking differs between the two files. Cheers On Fri, Oct 21, 2011 at 5:25 PM, Penta Upa wrote: > Hi, > > I'm facing a kernel panic at vm_page_wire(). Page is locked with > vm_page_lock() yet i get the following panic > panic: mutex page lock not owned at /usr/src/sys/vm/vm_page:1845 > > Code sequence is as below > vm_page_lock(pp); > vm_page_lock_assert(pp, MA_OWNED); /* No panic here */ > vm_page_wire(pp); /* Panic here for the same assertion as above, strange */ > vm_page_unlock(pp); > > Kernel on the system is unchanged after install. The only thing which > occurred out the way was that the first time install failed for checksum > mismatch for src.txz. Also there were some SCSI errors/warnings with the CD > drive during install. So the next time, i only installed the base and > kernel and later unpacked src.txz under /usr/src . Could this lead to any > issues ? > > Attached is a test module (vmtest) and the makefile used. Uname output from > the system is > > FreeBSD scache 9.0-BETA3 FreeBSD 9.0-BETA3 #0: Sat Sep 24 21:31:28 UTC > 2011 > r...@farrell.cse.buffalo.edu:/usr/obj/usr/src/sys/GENERIC amd64 > > Is there anything i'm doing wrong here ? Kindly help. > > Regards, > Penta > > ___ > freebsd-current@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-current > To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org" > ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3
I created a bug report since there wasn't a response to this email. http://www.freebsd.org/cgi/query-pr.cgi?pr=161887 The test code is attached to the bug report. Regards, Penta On Sat, Oct 29, 2011 at 2:23 AM, Sean Bruno wrote: > On Fri, 2011-10-21 at 08:25 -0700, Penta Upa wrote: > > > Attached is a test module (vmtest) and the makefile used. Uname output > from > > the system is > > I only see a Makefile attached here. Can you attach the code you are > using? > > Sean > > ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3
On Fri, 2011-10-21 at 08:25 -0700, Penta Upa wrote: > Attached is a test module (vmtest) and the makefile used. Uname output from > the system is I only see a Makefile attached here. Can you attach the code you are using? Sean ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
panic at vm_page_wire with FreeBSD 9.0 Beta 3
Hi, I'm facing a kernel panic at vm_page_wire(). Page is locked with vm_page_lock() yet i get the following panic panic: mutex page lock not owned at /usr/src/sys/vm/vm_page:1845 Code sequence is as below vm_page_lock(pp); vm_page_lock_assert(pp, MA_OWNED); /* No panic here */ vm_page_wire(pp); /* Panic here for the same assertion as above, strange */ vm_page_unlock(pp); Kernel on the system is unchanged after install. The only thing which occurred out the way was that the first time install failed for checksum mismatch for src.txz. Also there were some SCSI errors/warnings with the CD drive during install. So the next time, i only installed the base and kernel and later unpacked src.txz under /usr/src . Could this lead to any issues ? Attached is a test module (vmtest) and the makefile used. Uname output from the system is FreeBSD scache 9.0-BETA3 FreeBSD 9.0-BETA3 #0: Sat Sep 24 21:31:28 UTC 2011 r...@farrell.cse.buffalo.edu:/usr/obj/usr/src/sys/GENERIC amd64 Is there anything i'm doing wrong here ? Kindly help. Regards, Penta Makefile Description: Binary data ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"