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 a...@freebsd.org 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]]
On 12/1/11 5:04 PM, Arnaud Lacombe wrote: Hi, On Mon, Nov 14, 2011 at 5:14 AM, Andriy Gapona...@freebsd.org 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]]
Hi, On Mon, Nov 14, 2011 at 5:14 AM, Andriy Gapon a...@freebsd.org 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/18 Attilio Rao atti...@freebsd.org: 2011/11/18 Attilio Rao atti...@freebsd.org: 2011/11/18 Kostik Belousov kostik...@gmail.com: On Fri, Nov 18, 2011 at 11:40:28AM +0100, Attilio Rao wrote: 2011/11/16 Kostik Belousov kostik...@gmail.com: On Tue, Nov 15, 2011 at 07:15:01PM +0100, Attilio Rao wrote: 2011/11/7 Kostik Belousov kostik...@gmail.com: 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]
On Sun, Nov 20, 2011 at 05:37:33PM +0100, Attilio Rao wrote:
2011/11/18 Attilio Rao atti...@freebsd.org:
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/20 Kostik Belousov kostik...@gmail.com:
On Sun, Nov 20, 2011 at 05:37:33PM +0100, Attilio Rao wrote:
2011/11/18 Attilio Rao atti...@freebsd.org:
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 07:02:14PM +0100, Attilio Rao wrote:
2011/11/20 Kostik Belousov kostik...@gmail.com:
+#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]
It looks good to me.
Attilio
2011/11/20 Kostik Belousov kostik...@gmail.com:
On Sun, Nov 20, 2011 at 07:02:14PM +0100, Attilio Rao wrote:
2011/11/20 Kostik Belousov kostik...@gmail.com:
+#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]
2011/11/20 Attilio Rao atti...@freebsd.org: 2011/11/18 Attilio Rao atti...@freebsd.org: 2011/11/18 Attilio Rao atti...@freebsd.org: 2011/11/18 Kostik Belousov kostik...@gmail.com: On Fri, Nov 18, 2011 at 11:40:28AM +0100, Attilio Rao wrote: 2011/11/16 Kostik Belousov kostik...@gmail.com: On Tue, Nov 15, 2011 at 07:15:01PM +0100, Attilio Rao wrote: 2011/11/7 Kostik Belousov kostik...@gmail.com: 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]
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 Kostik Belousov kostik...@gmail.com: 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:22:38PM +0100, Attilio Rao wrote: 2011/11/20 Kostik Belousov kostik...@gmail.com: 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 kostik...@gmail.com: On Sun, Nov 20, 2011 at 08:22:38PM +0100, Attilio Rao wrote: 2011/11/20 Kostik Belousov kostik...@gmail.com: 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]
2011/11/16 Kostik Belousov kostik...@gmail.com: On Tue, Nov 15, 2011 at 07:15:01PM +0100, Attilio Rao wrote: 2011/11/7 Kostik Belousov kostik...@gmail.com: 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 Fri, Nov 18, 2011 at 11:40:28AM +0100, Attilio Rao wrote: 2011/11/16 Kostik Belousov kostik...@gmail.com: On Tue, Nov 15, 2011 at 07:15:01PM +0100, Attilio Rao wrote: 2011/11/7 Kostik Belousov kostik...@gmail.com: 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/18 Kostik Belousov kostik...@gmail.com: On Fri, Nov 18, 2011 at 11:40:28AM +0100, Attilio Rao wrote: 2011/11/16 Kostik Belousov kostik...@gmail.com: On Tue, Nov 15, 2011 at 07:15:01PM +0100, Attilio Rao wrote: 2011/11/7 Kostik Belousov kostik...@gmail.com: 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:56:55AM +0100, Attilio Rao wrote: 2011/11/18 Kostik Belousov kostik...@gmail.com: On Fri, Nov 18, 2011 at 11:40:28AM +0100, Attilio Rao wrote: 2011/11/16 Kostik Belousov kostik...@gmail.com: On Tue, Nov 15, 2011 at 07:15:01PM +0100, Attilio Rao wrote: 2011/11/7 Kostik Belousov kostik...@gmail.com: 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 Attilio Rao atti...@freebsd.org: 2011/11/18 Kostik Belousov kostik...@gmail.com: On Fri, Nov 18, 2011 at 11:40:28AM +0100, Attilio Rao wrote: 2011/11/16 Kostik Belousov kostik...@gmail.com: On Tue, Nov 15, 2011 at 07:15:01PM +0100, Attilio Rao wrote: 2011/11/7 Kostik Belousov kostik...@gmail.com: 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 Tue, Nov 15, 2011 at 07:15:01PM +0100, Attilio Rao wrote: 2011/11/7 Kostik Belousov kostik...@gmail.com: 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]
2011/11/7 Kostik Belousov kostik...@gmail.com: 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: 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 atti...@freebsd.org wrote: 2011/11/7 Kostik Belousov kostik...@gmail.com: 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/15 m...@freebsd.org: On Tue, Nov 15, 2011 at 10:15 AM, Attilio Rao atti...@freebsd.org wrote: 2011/11/7 Kostik Belousov kostik...@gmail.com: 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 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 kostik...@gmail.com 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: 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 jul...@freebsd.org 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 11/9/11, Arnaud Lacombe lacom...@gmail.com wrote: Hi, On Tue, Nov 8, 2011 at 9:35 PM, Julian Elischer jul...@freebsd.org wrote: On 11/8/11 5:52 PM, Arnaud Lacombe wrote: Hi, On Tue, Nov 8, 2011 at 7:08 PM, Julian Elischerjul...@freebsd.org 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
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 Elischerjul...@freebsd.org wrote: On 11/8/11 5:52 PM, Arnaud Lacombe wrote: Hi, On Tue, Nov 8, 2011 at 7:08 PM, Julian Elischerjul...@freebsd.org 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
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
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 lacom...@gmail.com wrote:
On Mon, Nov 7, 2011 at 4:36 AM, Attilio Rao atti...@freebsd.org wrote:
2011/11/7 Arnaud Lacombe lacom...@gmail.com:
Hi,
On Sat, Nov 5, 2011 at 10:13 AM, Kostik Belousov kostik...@gmail.com
wrote:
On Fri, Nov 04, 2011 at 06:03:39PM +0200, Kostik Belousov wrote:
Below is the KBI patch after vm_page_bits_t merge is done.
Again, I did not spent time converting all in-tree consumers
from the (potentially) loadable modules to the new KPI until it
is agreed upon.
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 on github:
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 lacom...@gmail.com:
Hi,
On Mon, Nov 7, 2011 at 2:03 PM, Arnaud Lacombe lacom...@gmail.com wrote:
On Mon, Nov 7, 2011 at 4:36 AM, Attilio Rao atti...@freebsd.org wrote:
2011/11/7 Arnaud Lacombe lacom...@gmail.com:
Hi,
On Sat, Nov 5, 2011 at 10:13 AM, Kostik Belousov kostik...@gmail.com
wrote:
On Fri, Nov 04, 2011 at 06:03:39PM +0200, Kostik Belousov wrote:
Below is the KBI patch after vm_page_bits_t merge is done.
Again, I did not spent time converting all in-tree consumers
from the (potentially) loadable modules to the new KPI until it
is agreed upon.
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
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 lacom...@gmail.com: 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]]
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]]
Hi, On Tue, Nov 8, 2011 at 7:08 PM, Julian Elischer jul...@freebsd.org 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]]
Hi, On Tue, Nov 8, 2011 at 3:55 PM, Andriy Gapon a...@freebsd.org wrote: [cc list trimmed] on 08/11/2011 22:34 Attilio Rao said the following: 2011/11/8 Arnaud Lacombe lacom...@gmail.com: 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 3:34 PM, Attilio Rao atti...@freebsd.org wrote:
2011/11/8 Arnaud Lacombe lacom...@gmail.com:
Hi,
On Mon, Nov 7, 2011 at 2:03 PM, Arnaud Lacombe lacom...@gmail.com wrote:
On Mon, Nov 7, 2011 at 4:36 AM, Attilio Rao atti...@freebsd.org wrote:
2011/11/7 Arnaud Lacombe lacom...@gmail.com:
Hi,
On Sat, Nov 5, 2011 at 10:13 AM, Kostik Belousov kostik...@gmail.com
wrote:
On Fri, Nov 04, 2011 at 06:03:39PM +0200, Kostik Belousov wrote:
Below is the KBI patch after vm_page_bits_t merge is done.
Again, I did not spent time converting all in-tree consumers
from the (potentially) loadable modules to the new KPI until it
is agreed upon.
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
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 lacom...@gmail.com wrote: Hi, On Tue, Nov 8, 2011 at 3:55 PM, Andriy Gapon a...@freebsd.org wrote: [cc list trimmed] on 08/11/2011 22:34 Attilio Rao said the following: 2011/11/8 Arnaud Lacombe lacom...@gmail.com: 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 7:08 PM, Julian Elischer jul...@freebsd.org 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]]
On 11/8/11 5:22 PM, Arnaud Lacombe wrote:
Hi,
On Tue, Nov 8, 2011 at 3:34 PM, Attilio Raoatti...@freebsd.org wrote:
2011/11/8 Arnaud Lacombelacom...@gmail.com:
Hi,
On Mon, Nov 7, 2011 at 2:03 PM, Arnaud Lacombelacom...@gmail.com wrote:
On Mon, Nov 7, 2011 at 4:36 AM, Attilio Raoatti...@freebsd.org wrote:
2011/11/7 Arnaud Lacombelacom...@gmail.com:
Hi,
On Sat, Nov 5, 2011 at 10:13 AM, Kostik Belousovkostik...@gmail.com wrote:
On Fri, Nov 04, 2011 at 06:03:39PM +0200, Kostik Belousov wrote:
Below is the KBI patch after vm_page_bits_t merge is done.
Again, I did not spent time converting all in-tree consumers
from the (potentially) loadable modules to the new KPI until it
is agreed upon.
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
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 Elischerjul...@freebsd.org 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]]
Hi, On Tue, Nov 8, 2011 at 9:35 PM, Julian Elischer jul...@freebsd.org wrote: On 11/8/11 5:52 PM, Arnaud Lacombe wrote: Hi, On Tue, Nov 8, 2011 at 7:08 PM, Julian Elischerjul...@freebsd.org 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 build; assertion are disabled. 2/ I can run gdb through firewire on it on ANY standard unmodified kernel
Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]
2011/11/7 Arnaud Lacombe lacom...@gmail.com:
Hi,
On Sat, Nov 5, 2011 at 10:13 AM, Kostik Belousov kostik...@gmail.com wrote:
On Fri, Nov 04, 2011 at 06:03:39PM +0200, Kostik Belousov wrote:
Below is the KBI patch after vm_page_bits_t merge is done.
Again, I did not spent time converting all in-tree consumers
from the (potentially) loadable modules to the new KPI until it
is agreed upon.
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]
2011/11/7 Attilio Rao atti...@freebsd.org:
2011/11/7 Arnaud Lacombe lacom...@gmail.com:
Hi,
On Sat, Nov 5, 2011 at 10:13 AM, Kostik Belousov kostik...@gmail.com wrote:
On Fri, Nov 04, 2011 at 06:03:39PM +0200, Kostik Belousov wrote:
Below is the KBI patch after vm_page_bits_t merge is done.
Again, I did not spent time converting all in-tree consumers
from the (potentially) loadable modules to the new KPI until it
is agreed upon.
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 depending by the compiling options.
I hope I can provide a patch asap.
Attilio
--
Peace can only be achieved by understanding - A. Einstein
___
freebsd-current@freebsd.org mailing list
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 Belousovkostik...@gmail.com wrote: On Fri, Nov 04, 2011 at 06:03:39PM +0200, Kostik Belousov wrote: Below is the KBI patch after vm_page_bits_t merge is done. Again, I did not spent time converting all in-tree consumers from the (potentially) loadable modules to the new KPI until it is agreed upon. I like my bikeshed orange... I would think a more canonical name would be get/set rather than read/write, especially since these operations aren't reading and writing the page (neither are they getting the page, but at least set is pretty unambiguous). Look at the vm_page.h:385. vm_page_set_valid() is already taken. I don't feel good about creating an interface under which we have functions named vm_page_set_valid() and vm_page_write_valid(). I think that we should take a step back and look at the whole of set of functions that exist for manipulating the page's valid and dirty field and see if we can come up with a logical schema for naming them. I wouldn't then be surprised if this results in renaming some of the existing functions. However, this should not delay the changes to address the vm_page_lock problem. I had two questions about that part of your patch. First, is there any reason that you didn't include vm_page_lockptr()? If we created the page locking macros that you, jhb@, and I were talking about last week, then vm_page_lockptr() would be required. Second, there seems to be precedent for naming the locking functions _vm_page_lock() instead of vm_page_lock_func(), for example, the mutex code, i.e., sys/mutex.h, and the vm map locking functions. I think vm_page_lockptr() should be included when some kind of reloc-iterating macros are actually introduced into the tree. And, I have a hope that they can be wrapped around a function with the signature like void vm_page_relock(vm_page_t locked_page, vm_page_t unlocked_page); which moves the lock from locked_page to unlocked_page. Ok. That sounds reasonable. Regarding the _vm_page_lock() vs. vm_page_lock_func(), the mutex.h has a lot of violations in regard of the namespaces, IMO. The __* namespace is reserved for the language implementation, so our freestanding program (kernel) ignores the requirements of the C standard with the names like __mtx_lock_spin(). Using the name _vm_page_lock() is valid, but makes it not unreasonable for other developers to introduce reserved names. So I decided to use the suffixes. vm_map.h locking is free of these violations. Ok. I'll offer one final suggestion. Please consider an alternative suffix to func. Perhaps, kbi or KBI. In other words, something that hints at the function's reason for existing. Alan ___ 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 atti...@freebsd.org wrote:
2011/11/7 Arnaud Lacombe lacom...@gmail.com:
Hi,
On Sat, Nov 5, 2011 at 10:13 AM, Kostik Belousov kostik...@gmail.com wrote:
On Fri, Nov 04, 2011 at 06:03:39PM +0200, Kostik Belousov wrote:
Below is the KBI patch after vm_page_bits_t merge is done.
Again, I did not spent time converting all in-tree consumers
from the (potentially) loadable modules to the new KPI until it
is agreed upon.
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 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_
Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3
Hi,
On Wed, Nov 2, 2011 at 6:32 AM, Andriy Gapon a...@freebsd.org 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}
personal opinionThis mess is utterly broken./personal opinion
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 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: 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 lacom...@gmail.com wrote: On Mon, Nov 7, 2011 at 4:36 AM, Attilio Rao atti...@freebsd.org 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 7, 2011 at 11:35 AM, Kostik Belousov kostik...@gmail.com 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]
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 kostik...@gmail.com 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]
Hi,
On Mon, Nov 7, 2011 at 2:35 PM, Kostik Belousov kostik...@gmail.com 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 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 Belousovkostik...@gmail.com wrote: On Fri, Nov 04, 2011 at 06:03:39PM +0200, Kostik Belousov wrote: Below is the KBI patch after vm_page_bits_t merge is done. Again, I did not spent time converting all in-tree consumers from the (potentially) loadable modules to the new KPI until it is agreed upon. I like my bikeshed orange... I would think a more canonical name would be get/set rather than read/write, especially since these operations aren't reading and writing the page (neither are they getting the page, but at least set is pretty unambiguous). Look at the vm_page.h:385. vm_page_set_valid() is already taken. I don't feel good about creating an interface under which we have functions named vm_page_set_valid() and vm_page_write_valid(). I think that we should take a step back and look at the whole of set of functions that exist for manipulating the page's valid and dirty field and see if we can come up with a logical schema for naming them. I wouldn't then be surprised if this results in renaming some of the existing functions. However, this should not delay the changes to address the vm_page_lock problem. I had two questions about that part of your patch. First, is there any reason that you didn't include vm_page_lockptr()? If we created the page locking macros that you, jhb@, and I were talking about last week, then vm_page_lockptr() would be required. Second, there seems to be precedent for naming the locking functions _vm_page_lock() instead of vm_page_lock_func(), for example, the mutex code, i.e., sys/mutex.h, and the vm map locking functions. I think vm_page_lockptr() should be included when some kind of reloc-iterating macros are actually introduced into the tree. And, I have a hope that they can be wrapped around a function with the signature like void vm_page_relock(vm_page_t locked_page, vm_page_t unlocked_page); which moves the lock from locked_page to unlocked_page. 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 Sun, Nov 6, 2011 at 4:43 AM, Kostik Belousov kostik...@gmail.com 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 Sun, Nov 06, 2011 at 07:22:51AM -0800, m...@freebsd.org wrote: On Sun, Nov 6, 2011 at 4:43 AM, Kostik Belousov kostik...@gmail.com 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]
Hi,
On Sat, Nov 5, 2011 at 10:13 AM, Kostik Belousov kostik...@gmail.com wrote:
On Fri, Nov 04, 2011 at 06:03:39PM +0200, Kostik Belousov wrote:
Below is the KBI patch after vm_page_bits_t merge is done.
Again, I did not spent time converting all in-tree consumers
from the (potentially) loadable modules to the new KPI until it
is agreed upon.
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_valid(m) vm_page_read_valid_func((m))
+#define vm_page_write_valid(m, v) vm_page_write_valid_func((m),
(v))
+
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 kostik...@gmail.com 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 kostik...@gmail.com 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
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 for vm_page.
@@
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 kostik...@gmail.com wrote:
On Fri, Nov 04, 2011 at 06:03:39PM +0200, Kostik Belousov wrote:
Below is the KBI patch after vm_page_bits_t merge is done.
Again, I did not spent time converting all in-tree consumers
from the (potentially) loadable modules to the new KPI until it
is agreed upon.
I like my bikeshed orange...
I would think a more canonical name would be get/set rather than
read/write, especially since these operations aren't reading and
writing the page (neither are they getting the page, but at least set
is pretty unambiguous).
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((m)))
#define vm_page_trylock(m) mtx_trylock(vm_page_lockptr((m)))
#define vm_page_lock_assert(m, a)
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 kostik...@gmail.com wrote: On Fri, Nov 04, 2011 at 06:03:39PM +0200, Kostik Belousov wrote: Below is the KBI patch after vm_page_bits_t merge is done. Again, I did not spent time converting all in-tree consumers from the (potentially) loadable modules to the new KPI until it is agreed upon. I like my bikeshed orange... I would think a more canonical name would be get/set rather than read/write, especially since these operations aren't reading and writing the page (neither are they getting the page, but at least set is pretty unambiguous). Look at the vm_page.h:385. vm_page_set_valid() is already taken. 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 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 Belousovkostik...@gmail.com wrote: On Fri, Nov 04, 2011 at 06:03:39PM +0200, Kostik Belousov wrote: Below is the KBI patch after vm_page_bits_t merge is done. Again, I did not spent time converting all in-tree consumers from the (potentially) loadable modules to the new KPI until it is agreed upon. I like my bikeshed orange... I would think a more canonical name would be get/set rather than read/write, especially since these operations aren't reading and writing the page (neither are they getting the page, but at least set is pretty unambiguous). Look at the vm_page.h:385. vm_page_set_valid() is already taken. I don't feel good about creating an interface under which we have functions named vm_page_set_valid() and vm_page_write_valid(). I think that we should take a step back and look at the whole of set of functions that exist for manipulating the page's valid and dirty field and see if we can come up with a logical schema for naming them. I wouldn't then be surprised if this results in renaming some of the existing functions. However, this should not delay the changes to address the vm_page_lock problem. I had two questions about that part of your patch. First, is there any reason that you didn't include vm_page_lockptr()? If we created the page locking macros that you, jhb@, and I were talking about last week, then vm_page_lockptr() would be required. Second, there seems to be precedent for naming the locking functions _vm_page_lock() instead of vm_page_lock_func(), for example, the mutex code, i.e., sys/mutex.h, and the vm map locking functions. 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
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/vm/vm_page.h
index 23637bb..7d1a529 100644
--- a/sys/vm/vm_page.h
+++ b/sys/vm/vm_page.h
@@ -113,6 +113,21 @@
TAILQ_HEAD(pglist,
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);
+ bits = vm_page_bits(base, size);
if
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 VM_PAGE_BITS_ALL 0xu
+typedef uint16_t vm_page_bits_t;
+#elif PAGE_SIZE == 16384
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 +113,20 @@
TAILQ_HEAD(pglist, vm_page);
+#if PAGE_SIZE == 4096
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 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_t dirty; /* map of dirty
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 support one bit per
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 km...@freebsd.org 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 a...@freebsd.org 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
[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 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
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 km...@freebsd.org 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 bsdb...@gmail.com 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 bsdb...@gmail.com 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 sean...@yahoo-inc.com 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
