Page MenuHomeFreeBSD

Improve kernel stack pages check in ZFS module
AbandonedPublic

Authored by smh on Aug 3 2015, 11:43 AM.

Details

Reviewers
kib
Summary

This switches to testing kstack_pages at runtime instead of at compile time as its thread0 which causes the issue hence its the kernel's kstack_size size not the one which the ZFS module compiled with which needs testing.

This is still just a validation check, it would be good if this could if kernel stack size could be configured on the fly via loader setting, but that's something for another day.

Diff Detail

Repository
rS FreeBSD src repository
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

smh updated this revision to Diff 7607.Aug 3 2015, 11:43 AM
smh retitled this revision from to Improve kernel stack pages check in ZFS module.
smh updated this object.
smh edited the test plan for this revision. (Show Details)
smh added subscribers: peter, kib.Aug 3 2015, 11:45 AM
smh added reviewers: peter, kib.Aug 3 2015, 11:51 AM
smh removed subscribers: kib, peter.
kib edited edge metadata.Aug 3 2015, 12:05 PM

I consider it silly to make kernel spit a reprimand to user.

andrew added a subscriber: andrew.Aug 3 2015, 12:06 PM
andrew added inline comments.
sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c
6503–6508

Shouldn't this be curthread->td_kstack_pages? It may be different than kstack_pages.

smh added inline comments.Aug 3 2015, 1:25 PM
sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c
6503–6508

Its been noted that its mainly thread0 so no.

smh added a comment.Aug 3 2015, 1:29 PM

This patch is purely about visibility, making the user aware before they hit a kernel panic, not fixing the underlying problem; so yes it is a temporary thing not a fix for the underlying issue.

smh updated this revision to Diff 7673.Aug 5 2015, 11:04 AM
smh edited edge metadata.

Switched check to thread0 as this is the key issue and r286288 introduced a change to alter thread0 stack size on i386.

peter removed a reviewer: peter.Aug 15 2015, 10:09 PM
delphij added inline comments.Aug 15 2015, 11:14 PM
sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c
6504

I think the old message is fine (the new contents is useful as comment or commit message), it could be confusing for our average users who does not have idea what thread0 is.

smh added inline comments.Aug 27 2015, 12:52 PM
sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c
6504

I'm not sure I like the idea of the message not accurately reflecting the test being performed, which would be the case if we kept it as "KSTACK_PAGES is %d which could result in stack".

The message does include how the user can correct the situation i.e. add options KSTACK_PAGES=4, so even if they don't know what thread0 is, the fix should still be understood, which I think is enough.

What do others think?

smh added a comment.Dec 1 2015, 12:22 PM

Unless any objections? I'm going to get this is in soon...

smh abandoned this revision.Jan 4 2016, 5:36 PM

kib believes this is waste of kernel memory so abandoning this review:

I noted very early in the history of this review, that the task of teaching user how to use the system is usually delegated to the man page, handbook and similar facilities, and not to the kernel. Not to mention that it eats several hundred bytes of the kernel memory.
Also, on arches where use of ZFS is practically feasible, the kern.kstack_pages tunable works.
On i386 there is a quick, which causes thread0 to use much larger stack even if the regular threads stack size is small, so your warning is probably a dead code there.