Re: svn commit: r286223 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs
On Wed, Aug 05, 2015 at 12:10:23PM +0100, Steven Hartland wrote:
> >> Just in case you didn't notice kib committed a fix for i386 thread0 in
> >> r286288 so this may not be needed at all any more which is good news :)
> > If I understund kib fix (and you about ZFS stack requirements) you
> > need check curthread->td_kstack_pages (for case old, unfixed kerenel,
> > depended from KSTACK_PAGES in Thread0).
> >
> > I.e. for all cases:
> >
> > - unfixed kernel with KSTACK_PAGES < 4
> > - unfixed kernel with KSTACK_PAGES >= 4
> > - fixed kernel with KSTACK_PAGES < 4
> > - fixed kernel with KSTACK_PAGES >= 4
> > - compiling zfs.ko separately from kernel with different KSTACK_PAGES
> > - using zfs.ko with old kernel
> >
> > checking curthread->td_kstack_pages is right way (or, may be
> > curthread->td_kstack_pages*PAGE_SIZE -- I am not cleanly understund
> > what need to check -- size in bytes or size in pages).
> > Checking KSTACK_PAGES by ifdef can produce vrong result.
> Its not clear to me if curthread will be thread0 however in unfixed
printf("zfs__init thread check: curthread %p thread0 %p\n", curthread,
&thread0) ?
> thread0 stack size would have been kstack_pages so at the time this was
> still correct IMO.
Not clear to me where is real limitaion -- in thread0 or in thread
executed zfs__init.
Just for me, what is correct?
1) limitation in thread0 (some code from ZFS executed later in
thread0, not in current tread), not in zfs__init
2) limitation in zfs__init (or in some code executed later in current
thread), currently zfs__init exeuted in thread0
> Just to be clear this is not checked by an ifdef, that's just OS guard
> which is correct.
>
> Latest revision which now explicitly checks thread0 stack size is now
> available:
> https://reviews.freebsd.org/D3279
nice to me.
___
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"
Re: svn commit: r286223 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs
On 04/08/2015 21:24, Slawa Olhovchenkov wrote:
On Tue, Aug 04, 2015 at 09:06:55PM +0100, Steven Hartland wrote:
On 04/08/2015 17:14, Slawa Olhovchenkov wrote:
On Tue, Aug 04, 2015 at 09:30:30AM +0100, Steven Hartland wrote:
On 03/08/2015 21:48, Warner Losh wrote:
On Aug 3, 2015, at 1:44 PM, Slawa Olhovchenkov wrote:
On Tue, Aug 04, 2015 at 03:35:50AM +0800, Julian Elischer wrote:
On 8/3/15 8:03 PM, Konstantin Belousov wrote:
On Mon, Aug 03, 2015 at 12:50:19PM +0100, Steven Hartland wrote:
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.
Hmm, is it limited to the thread0 only ? I.e., would only increasing
the initial thread stack size be enough to boot the kernel ? The zfs
threads do request larger stack size, I know this.
Can somebody test the following patch in the i386 configuration which
does not boot ?
I think this is a reasonable thing to do. Thread0 (and proc0) are special.
I don't see why giving it a specially sized stack would be a problem.
This is always do for ARM.
May be need increase stack size for Thread0 on ARM too?
Seems reasonable. There should be a MI way of doing this, but all the code and
defines are buried in MD files, so each architecture needs some love to make
this a reality.
Warner
In the mean time are people happier with
https://reviews.freebsd.org/D3279 or should I just leave it using the
#define until someone has time to work on a full solution?
Checking by #ifdef you check only parametr at time of building zfs.ko,
checking variable you check actual value.
May be check thread stack best if only for current tread.
Not sure I follow you as its not a #ifdef check its straight if in the
new version i.e.
if (kstack_pages < ZFS_MIN_KSTACK_PAGES) {
This check checked how actual kernel compile vs how compile zfs.ko.
Remeber that kenel may be compiled independed from modules?
Correct that's the requirement as its thread0 that appears to be the key.
Just in case you didn't notice kib committed a fix for i386 thread0 in
r286288 so this may not be needed at all any more which is good news :)
If I understund kib fix (and you about ZFS stack requirements) you
need check curthread->td_kstack_pages (for case old, unfixed kerenel,
depended from KSTACK_PAGES in Thread0).
I.e. for all cases:
- unfixed kernel with KSTACK_PAGES < 4
- unfixed kernel with KSTACK_PAGES >= 4
- fixed kernel with KSTACK_PAGES < 4
- fixed kernel with KSTACK_PAGES >= 4
- compiling zfs.ko separately from kernel with different KSTACK_PAGES
- using zfs.ko with old kernel
checking curthread->td_kstack_pages is right way (or, may be
curthread->td_kstack_pages*PAGE_SIZE -- I am not cleanly understund
what need to check -- size in bytes or size in pages).
Checking KSTACK_PAGES by ifdef can produce vrong result.
Its not clear to me if curthread will be thread0 however in unfixed
thread0 stack size would have been kstack_pages so at the time this was
still correct IMO.
Just to be clear this is not checked by an ifdef, that's just OS guard
which is correct.
Latest revision which now explicitly checks thread0 stack size is now
available:
https://reviews.freebsd.org/D3279
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"
Re: svn commit: r286223 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs
On Tue, Aug 04, 2015 at 09:06:55PM +0100, Steven Hartland wrote:
>
>
> On 04/08/2015 17:14, Slawa Olhovchenkov wrote:
> > On Tue, Aug 04, 2015 at 09:30:30AM +0100, Steven Hartland wrote:
> >
> >>
> >> On 03/08/2015 21:48, Warner Losh wrote:
> On Aug 3, 2015, at 1:44 PM, Slawa Olhovchenkov wrote:
>
> On Tue, Aug 04, 2015 at 03:35:50AM +0800, Julian Elischer wrote:
>
> > On 8/3/15 8:03 PM, Konstantin Belousov wrote:
> >> On Mon, Aug 03, 2015 at 12:50:19PM +0100, Steven Hartland wrote:
> >>> 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.
> >> Hmm, is it limited to the thread0 only ? I.e., would only increasing
> >> the initial thread stack size be enough to boot the kernel ? The zfs
> >> threads do request larger stack size, I know this.
> >>
> >> Can somebody test the following patch in the i386 configuration which
> >> does not boot ?
> > I think this is a reasonable thing to do. Thread0 (and proc0) are
> > special.
> > I don't see why giving it a specially sized stack would be a problem.
> This is always do for ARM.
> May be need increase stack size for Thread0 on ARM too?
> >>> Seems reasonable. There should be a MI way of doing this, but all the
> >>> code and defines are buried in MD files, so each architecture needs some
> >>> love to make this a reality.
> >>>
> >>> Warner
> >> In the mean time are people happier with
> >> https://reviews.freebsd.org/D3279 or should I just leave it using the
> >> #define until someone has time to work on a full solution?
> > Checking by #ifdef you check only parametr at time of building zfs.ko,
> > checking variable you check actual value.
> > May be check thread stack best if only for current tread.
> Not sure I follow you as its not a #ifdef check its straight if in the
> new version i.e.
> if (kstack_pages < ZFS_MIN_KSTACK_PAGES) {
This check checked how actual kernel compile vs how compile zfs.ko.
Remeber that kenel may be compiled independed from modules?
> Just in case you didn't notice kib committed a fix for i386 thread0 in
> r286288 so this may not be needed at all any more which is good news :)
If I understund kib fix (and you about ZFS stack requirements) you
need check curthread->td_kstack_pages (for case old, unfixed kerenel,
depended from KSTACK_PAGES in Thread0).
I.e. for all cases:
- unfixed kernel with KSTACK_PAGES < 4
- unfixed kernel with KSTACK_PAGES >= 4
- fixed kernel with KSTACK_PAGES < 4
- fixed kernel with KSTACK_PAGES >= 4
- compiling zfs.ko separately from kernel with different KSTACK_PAGES
- using zfs.ko with old kernel
checking curthread->td_kstack_pages is right way (or, may be
curthread->td_kstack_pages*PAGE_SIZE -- I am not cleanly understund
what need to check -- size in bytes or size in pages).
Checking KSTACK_PAGES by ifdef can produce vrong result.
___
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"
Re: svn commit: r286223 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs
On 04/08/2015 17:14, Slawa Olhovchenkov wrote:
On Tue, Aug 04, 2015 at 09:30:30AM +0100, Steven Hartland wrote:
On 03/08/2015 21:48, Warner Losh wrote:
On Aug 3, 2015, at 1:44 PM, Slawa Olhovchenkov wrote:
On Tue, Aug 04, 2015 at 03:35:50AM +0800, Julian Elischer wrote:
On 8/3/15 8:03 PM, Konstantin Belousov wrote:
On Mon, Aug 03, 2015 at 12:50:19PM +0100, Steven Hartland wrote:
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.
Hmm, is it limited to the thread0 only ? I.e., would only increasing
the initial thread stack size be enough to boot the kernel ? The zfs
threads do request larger stack size, I know this.
Can somebody test the following patch in the i386 configuration which
does not boot ?
I think this is a reasonable thing to do. Thread0 (and proc0) are special.
I don't see why giving it a specially sized stack would be a problem.
This is always do for ARM.
May be need increase stack size for Thread0 on ARM too?
Seems reasonable. There should be a MI way of doing this, but all the code and
defines are buried in MD files, so each architecture needs some love to make
this a reality.
Warner
In the mean time are people happier with
https://reviews.freebsd.org/D3279 or should I just leave it using the
#define until someone has time to work on a full solution?
Checking by #ifdef you check only parametr at time of building zfs.ko,
checking variable you check actual value.
May be check thread stack best if only for current tread.
Not sure I follow you as its not a #ifdef check its straight if in the
new version i.e.
if (kstack_pages < ZFS_MIN_KSTACK_PAGES) {
Just in case you didn't notice kib committed a fix for i386 thread0 in
r286288 so this may not be needed at all any more which is good news :)
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"
Re: svn commit: r286223 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs
On Tue, Aug 04, 2015 at 09:30:30AM +0100, Steven Hartland wrote: > > > On 03/08/2015 21:48, Warner Losh wrote: > >> On Aug 3, 2015, at 1:44 PM, Slawa Olhovchenkov wrote: > >> > >> On Tue, Aug 04, 2015 at 03:35:50AM +0800, Julian Elischer wrote: > >> > >>> On 8/3/15 8:03 PM, Konstantin Belousov wrote: > On Mon, Aug 03, 2015 at 12:50:19PM +0100, Steven Hartland wrote: > > 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. > Hmm, is it limited to the thread0 only ? I.e., would only increasing > the initial thread stack size be enough to boot the kernel ? The zfs > threads do request larger stack size, I know this. > > Can somebody test the following patch in the i386 configuration which > does not boot ? > >>> I think this is a reasonable thing to do. Thread0 (and proc0) are special. > >>> I don't see why giving it a specially sized stack would be a problem. > >> This is always do for ARM. > >> May be need increase stack size for Thread0 on ARM too? > > Seems reasonable. There should be a MI way of doing this, but all the code > > and defines are buried in MD files, so each architecture needs some love to > > make this a reality. > > > > Warner > In the mean time are people happier with > https://reviews.freebsd.org/D3279 or should I just leave it using the > #define until someone has time to work on a full solution? Checking by #ifdef you check only parametr at time of building zfs.ko, checking variable you check actual value. May be check thread stack best if only for current tread. ___ 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"
Re: svn commit: r286223 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs
On 03/08/2015 21:48, Warner Losh wrote: On Aug 3, 2015, at 1:44 PM, Slawa Olhovchenkov wrote: On Tue, Aug 04, 2015 at 03:35:50AM +0800, Julian Elischer wrote: On 8/3/15 8:03 PM, Konstantin Belousov wrote: On Mon, Aug 03, 2015 at 12:50:19PM +0100, Steven Hartland wrote: 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. Hmm, is it limited to the thread0 only ? I.e., would only increasing the initial thread stack size be enough to boot the kernel ? The zfs threads do request larger stack size, I know this. Can somebody test the following patch in the i386 configuration which does not boot ? I think this is a reasonable thing to do. Thread0 (and proc0) are special. I don't see why giving it a specially sized stack would be a problem. This is always do for ARM. May be need increase stack size for Thread0 on ARM too? Seems reasonable. There should be a MI way of doing this, but all the code and defines are buried in MD files, so each architecture needs some love to make this a reality. Warner In the mean time are people happier with https://reviews.freebsd.org/D3279 or should I just leave it using the #define until someone has time to work on a full solution? 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"
Re: svn commit: r286223 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs
> On Aug 3, 2015, at 1:44 PM, Slawa Olhovchenkov wrote: > > On Tue, Aug 04, 2015 at 03:35:50AM +0800, Julian Elischer wrote: > >> On 8/3/15 8:03 PM, Konstantin Belousov wrote: >>> On Mon, Aug 03, 2015 at 12:50:19PM +0100, Steven Hartland wrote: 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. >>> Hmm, is it limited to the thread0 only ? I.e., would only increasing >>> the initial thread stack size be enough to boot the kernel ? The zfs >>> threads do request larger stack size, I know this. >>> >>> Can somebody test the following patch in the i386 configuration which >>> does not boot ? >> >> I think this is a reasonable thing to do. Thread0 (and proc0) are special. >> I don't see why giving it a specially sized stack would be a problem. > > This is always do for ARM. > May be need increase stack size for Thread0 on ARM too? Seems reasonable. There should be a MI way of doing this, but all the code and defines are buried in MD files, so each architecture needs some love to make this a reality. Warner signature.asc Description: Message signed with OpenPGP using GPGMail
Re: svn commit: r286223 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs
On Tue, Aug 04, 2015 at 03:35:50AM +0800, Julian Elischer wrote: > On 8/3/15 8:03 PM, Konstantin Belousov wrote: > > On Mon, Aug 03, 2015 at 12:50:19PM +0100, Steven Hartland wrote: > >> 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. > > Hmm, is it limited to the thread0 only ? I.e., would only increasing > > the initial thread stack size be enough to boot the kernel ? The zfs > > threads do request larger stack size, I know this. > > > > Can somebody test the following patch in the i386 configuration which > > does not boot ? > > I think this is a reasonable thing to do. Thread0 (and proc0) are special. > I don't see why giving it a specially sized stack would be a problem. This is always do for ARM. May be need increase stack size for Thread0 on ARM too? ___ 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"
Re: svn commit: r286223 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs
On 8/3/15 8:03 PM, Konstantin Belousov wrote: On Mon, Aug 03, 2015 at 12:50:19PM +0100, Steven Hartland wrote: 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. Hmm, is it limited to the thread0 only ? I.e., would only increasing the initial thread stack size be enough to boot the kernel ? The zfs threads do request larger stack size, I know this. Can somebody test the following patch in the i386 configuration which does not boot ? I think this is a reasonable thing to do. Thread0 (and proc0) are special. I don't see why giving it a specially sized stack would be a problem. ___ 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"
Re: svn commit: r286223 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs
On 03/08/2015 13:03, Konstantin Belousov wrote: On Mon, Aug 03, 2015 at 12:50:19PM +0100, Steven Hartland wrote: 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. Hmm, is it limited to the thread0 only ? I.e., would only increasing the initial thread stack size be enough to boot the kernel ? The zfs threads do request larger stack size, I know this. Can somebody test the following patch in the i386 configuration which does not boot ? diff --git a/sys/i386/i386/genassym.c b/sys/i386/i386/genassym.c index 7a00740..6a00d23 100644 --- a/sys/i386/i386/genassym.c +++ b/sys/i386/i386/genassym.c @@ -103,6 +103,7 @@ ASSYM(V_SYSCALL, offsetof(struct vmmeter, v_syscall)); ASSYM(V_INTR, offsetof(struct vmmeter, v_intr)); /* ASSYM(UPAGES, UPAGES);*/ ASSYM(KSTACK_PAGES, KSTACK_PAGES); +ASSYM(TD0_KSTACK_PAGES, TD0_KSTACK_PAGES); ASSYM(PAGE_SIZE, PAGE_SIZE); ASSYM(NPTEPG, NPTEPG); ASSYM(NPDEPG, NPDEPG); diff --git a/sys/i386/i386/locore.s b/sys/i386/i386/locore.s index 5bf7944..4d8e22f 100644 --- a/sys/i386/i386/locore.s +++ b/sys/i386/i386/locore.s @@ -731,7 +731,7 @@ no_kernend: movl%esi,R(IdlePTD) /* Allocate KSTACK */ - ALLOCPAGES(KSTACK_PAGES) + ALLOCPAGES(TD0_KSTACK_PAGES) movl%esi,R(p0kpa) addl$KERNBASE, %esi movl%esi, R(proc0kstack) @@ -800,7 +800,7 @@ no_kernend: /* Map proc0's KSTACK in the physical way ... */ movlR(p0kpa), %eax - movl$(KSTACK_PAGES), %ecx + movl$(TD0_KSTACK_PAGES), %ecx fillkptphys($PG_RW) /* Map ISA hole */ diff --git a/sys/i386/i386/machdep.c b/sys/i386/i386/machdep.c index 2be5dbc..76790f0 100644 --- a/sys/i386/i386/machdep.c +++ b/sys/i386/i386/machdep.c @@ -2445,7 +2445,7 @@ init386(first) #endif thread0.td_kstack = proc0kstack; - thread0.td_kstack_pages = KSTACK_PAGES; + thread0.td_kstack_pages = TD0_KSTACK_PAGES; /* * This may be done better later if it gets more high level diff --git a/sys/i386/include/param.h b/sys/i386/include/param.h index b3fd85f..bc79c20 100644 --- a/sys/i386/include/param.h +++ b/sys/i386/include/param.h @@ -114,6 +114,11 @@ #define KSTACK_PAGES 2/* Includes pcb! */ #endif #define KSTACK_GUARD_PAGES 1 /* pages of kstack guard; 0 disables */ +#if KSTACK_PAGES < 3 +#defineTD0_KSTACK_PAGES 4 +#else +#defineTD0_KSTACK_PAGES KSTACK_PAGES +#endif /* * Ceiling on amount of swblock kva space, can be changed via I don't have a reproduction box here I'm afraid, might be an idea to post the patch to the following bug reports to see if one of the reporters can test: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=201859 https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=189355 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"
Re: svn commit: r286223 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs
On Mon, Aug 03, 2015 at 12:50:19PM +0100, Steven Hartland wrote: > 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. Hmm, is it limited to the thread0 only ? I.e., would only increasing the initial thread stack size be enough to boot the kernel ? The zfs threads do request larger stack size, I know this. Can somebody test the following patch in the i386 configuration which does not boot ? diff --git a/sys/i386/i386/genassym.c b/sys/i386/i386/genassym.c index 7a00740..6a00d23 100644 --- a/sys/i386/i386/genassym.c +++ b/sys/i386/i386/genassym.c @@ -103,6 +103,7 @@ ASSYM(V_SYSCALL, offsetof(struct vmmeter, v_syscall)); ASSYM(V_INTR, offsetof(struct vmmeter, v_intr)); /* ASSYM(UPAGES, UPAGES);*/ ASSYM(KSTACK_PAGES, KSTACK_PAGES); +ASSYM(TD0_KSTACK_PAGES, TD0_KSTACK_PAGES); ASSYM(PAGE_SIZE, PAGE_SIZE); ASSYM(NPTEPG, NPTEPG); ASSYM(NPDEPG, NPDEPG); diff --git a/sys/i386/i386/locore.s b/sys/i386/i386/locore.s index 5bf7944..4d8e22f 100644 --- a/sys/i386/i386/locore.s +++ b/sys/i386/i386/locore.s @@ -731,7 +731,7 @@ no_kernend: movl%esi,R(IdlePTD) /* Allocate KSTACK */ - ALLOCPAGES(KSTACK_PAGES) + ALLOCPAGES(TD0_KSTACK_PAGES) movl%esi,R(p0kpa) addl$KERNBASE, %esi movl%esi, R(proc0kstack) @@ -800,7 +800,7 @@ no_kernend: /* Map proc0's KSTACK in the physical way ... */ movlR(p0kpa), %eax - movl$(KSTACK_PAGES), %ecx + movl$(TD0_KSTACK_PAGES), %ecx fillkptphys($PG_RW) /* Map ISA hole */ diff --git a/sys/i386/i386/machdep.c b/sys/i386/i386/machdep.c index 2be5dbc..76790f0 100644 --- a/sys/i386/i386/machdep.c +++ b/sys/i386/i386/machdep.c @@ -2445,7 +2445,7 @@ init386(first) #endif thread0.td_kstack = proc0kstack; - thread0.td_kstack_pages = KSTACK_PAGES; + thread0.td_kstack_pages = TD0_KSTACK_PAGES; /* * This may be done better later if it gets more high level diff --git a/sys/i386/include/param.h b/sys/i386/include/param.h index b3fd85f..bc79c20 100644 --- a/sys/i386/include/param.h +++ b/sys/i386/include/param.h @@ -114,6 +114,11 @@ #define KSTACK_PAGES 2 /* Includes pcb! */ #endif #define KSTACK_GUARD_PAGES 1 /* pages of kstack guard; 0 disables */ +#if KSTACK_PAGES < 3 +#defineTD0_KSTACK_PAGES 4 +#else +#defineTD0_KSTACK_PAGES KSTACK_PAGES +#endif /* * Ceiling on amount of swblock kva space, can be changed via ___ 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"
Re: svn commit: r286223 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs
On Mon, Aug 03, 2015 at 12:50:19PM +0100, Steven Hartland wrote: > > 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. I am currently don't have phabricator account, thanks. ___ 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"
Re: svn commit: r286223 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs
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 +, 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_.
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"
Re: svn commit: r286223 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs
On Mon, Aug 03, 2015 at 02:19:42PM +0300, Konstantin Belousov wrote:
> > 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.
Parsing kenv by asm too complex?
___
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"
Re: svn commit: r286223 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs
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 +, 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_.
> >
> > 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.
___
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"
Re: svn commit: r286223 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs
On Mon, Aug 03, 2015 at 11:31:58AM +0100, Steven Hartland wrote: > On 03/08/2015 10:47, Slawa Olhovchenkov wrote: > > On Mon, Aug 03, 2015 at 09:34:10AM +, 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_. > > > 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? ZFS currently don't have true direct dependency from KSTACK_PAGES (no code like this: /usr/src/sys/i386/i386/mp_machdep.c:bootSTK = (char *)bootstacks[cpu] + KSTACK_PAGES * PAGE_SIZE - 4; This is simple guard check. I.e. change KSTACK_PAGES don't want recompile zfs.ko. And less work to somebody converted KSTACK_PAGES to loader tunable. Most of this code already may be conveted to use kstack_pages w/o performance impact. Only 5 lines in 3 asm files need some more work. ___ 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"
Re: svn commit: r286223 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs
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 +, 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_.
>
> 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.
--
Peter Wemm - pe...@wemm.org; pe...@freebsd.org; pe...@yahoo-inc.com; KI6FJV
UTF-8: for when a ' or ... just won\342\200\231t do\342\200\246
signature.asc
Description: This is a digitally signed message part.
Re: svn commit: r286223 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs
On 03/08/2015 10:47, Slawa Olhovchenkov wrote: On Mon, Aug 03, 2015 at 09:34:10AM +, 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_. 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? 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"
Re: svn commit: r286223 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs
On Mon, Aug 03, 2015 at 09:34:10AM +, 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_.
> MFC after: 3 days
> Sponsored by: Multiplay
>
> Modified:
> head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c
>
> Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c
> ==
> --- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c Mon Aug
> 3 08:04:31 2015(r286222)
> +++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c Mon Aug
> 3 09:34:09 2015(r286223)
> @@ -132,6 +132,9 @@
> * distinguish between the operation failing, and
> * deserialization failing.
> */
> +#ifdef __FreeBSD__
> +#include "opt_kstack_pages.h"
> +#endif
>
> #include
> #include
> @@ -6491,18 +6494,22 @@ static void zfs_shutdown(void *, int);
>
> static eventhandler_tag zfs_shutdown_event_tag;
>
> +#ifdef __FreeBSD__
> #define ZFS_MIN_KSTACK_PAGES 4
> +#endif
>
> int
> zfs__init(void)
> {
>
> +#ifdef __FreeBSD__
> #if KSTACK_PAGES < ZFS_MIN_KSTACK_PAGES
> printf("ZFS NOTICE: KSTACK_PAGES is %d which could result in stack "
> "overflow panic!\nPlease consider adding "
> "'options KSTACK_PAGES=%d' to your kernel config\n", KSTACK_PAGES,
> ZFS_MIN_KSTACK_PAGES);
> #endif
> +#endif
> zfs_root_token = root_mount_hold("ZFS");
>
> mutex_init(&zfs_share_lock, NULL, MUTEX_DEFAULT, NULL);
> ___
> svn-src-...@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/svn-src-all
> To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
___
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"
svn commit: r286223 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs
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.
MFC after:3 days
Sponsored by: Multiplay
Modified:
head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c
Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c
==
--- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c Mon Aug
3 08:04:31 2015(r286222)
+++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c Mon Aug
3 09:34:09 2015(r286223)
@@ -132,6 +132,9 @@
* distinguish between the operation failing, and
* deserialization failing.
*/
+#ifdef __FreeBSD__
+#include "opt_kstack_pages.h"
+#endif
#include
#include
@@ -6491,18 +6494,22 @@ static void zfs_shutdown(void *, int);
static eventhandler_tag zfs_shutdown_event_tag;
+#ifdef __FreeBSD__
#define ZFS_MIN_KSTACK_PAGES 4
+#endif
int
zfs__init(void)
{
+#ifdef __FreeBSD__
#if KSTACK_PAGES < ZFS_MIN_KSTACK_PAGES
printf("ZFS NOTICE: KSTACK_PAGES is %d which could result in stack "
"overflow panic!\nPlease consider adding "
"'options KSTACK_PAGES=%d' to your kernel config\n", KSTACK_PAGES,
ZFS_MIN_KSTACK_PAGES);
#endif
+#endif
zfs_root_token = root_mount_hold("ZFS");
mutex_init(&zfs_share_lock, NULL, MUTEX_DEFAULT, NULL);
___
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"
