On 03/08/2015 12:19, Konstantin Belousov wrote:
On Mon, Aug 03, 2015 at 03:52:21AM -0700, Peter Wemm wrote:
On Monday, August 03, 2015 11:31:58 AM Steven Hartland wrote:
On 03/08/2015 10:47, Slawa Olhovchenkov wrote:
On Mon, Aug 03, 2015 at 09:34:10AM +0000, Steven Hartland wrote:
Author: smh
Date: Mon Aug 3 09:34:09 2015
New Revision: 286223
URL: https://svnweb.freebsd.org/changeset/base/286223
Log:
Fix KSTACK_PAGES check in ZFS module
The check introduced by r285946 failed to add the dependency on
opt_kstack_pages.h which meant the default value for the platform
instead
of the customised options KSTACK_PAGES=X was being tested.
Also wrap in #ifdef __FreeBSD__ for portability.
/usr/src/sys/kern/kern_proc.c:int kstack_pages = KSTACK_PAGES;
May be check variable kstack_pages is best way?
Eliminate dependency on foreign opt_XXXX.
I did think of that but as other modules such as dtrace, which is also
cddl code, already have this dependency I went with this.
I'm easy though, if there's a concusses that kstack_pages or possibly
curthread->td_kstack_pages, which would take into account the
possibility of varied thread stack sizes, then I can make that change.
What do others think?
The whole thing has missing the point.
Changing the default for the entire kernel just because the zfs compat
wrappers can't be bothered requesting a suitable value is.. unfortunate..
particularly when it is in freebsd-provided code, not upstream zfs code.
Fix the kproc_kthread_add() calls in do_thread_create() and zvol_geom_run()
instead. Enforce a lower bound there for zfs threads instead of making the
entire rest of the kernel use more memory.
eg: I'm thinking along these lines:
Index: cddl/compat/opensolaris/sys/proc.h
====================================================
--- cddl/compat/opensolaris/sys/proc.h (revision 286224)
+++ cddl/compat/opensolaris/sys/proc.h (working copy)
@@ -77,6 +77,8 @@
ASSERT(state == TS_RUN);
ASSERT(pp == &p0);
+ if (stksize < 16384)
+ stksize = 16384; /* Enforce lower bound on ZFS threads */
error = kproc_kthread_add(proc, arg, &zfsproc, &td, RFSTOPPED,
stksize / PAGE_SIZE, "zfskern", "solthread %p", proc);
if (error == 0) {
Beware, some platforms have large pages (eg: ia64 in -stable has 8k, 16k or
32k pages, from memory). Specifying an arbitrary number of pages in code
that's supposed to be portable isn't a good idea.
This would not help. Issue is the size of the thread0 stack, which
overflows in this case.
I looked at the possibility of making default kernel stack size
configurable by a loader tunable, and the issue is that thread0 gets its
stack set up too early (locore for i386, hammer_time() for amd64). I.e.,
it is possible to make the setting effective for all threads after thread0,
but not for the one which causes the issue.
I do not want to modify ABI between loader and kernel to pass the parameter.
I've created a review for the current proposed change to look at the
kernel var kstack_pages vs
the compile time define KSTACK_PAGES.
For this change I don't want to get into fixing the thread0 stack size,
which can be done later, just
to provide a reasonable warning to the user that smaller values could
cause a panic.
@slw I've added peter and kib as reviewers if you phabricator account
then feel free to add yourself.
Regards
Steve
_______________________________________________
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
