Re: Broken suspend-resume (suspend to RAM) with enabled INVARIANTS on 11-CURRENT - with workaround
On Monday, February 08, 2016 12:52:30 PM Konstantin Belousov wrote:
> > dev/hwpmc/hwpmc_mod.c-
> > dev/hwpmc/hwpmc_mod.c- /* issue an attach event to a configured log file */
> > dev/hwpmc/hwpmc_mod.c- if (pm->pm_owner->po_flags & PMC_PO_OWNS_LOGFILE) {
> > dev/hwpmc/hwpmc_mod.c: if (p->p_flag & P_KTHREAD) {
> > dev/hwpmc/hwpmc_mod.c- fullpath = kernelname;
> > dev/hwpmc/hwpmc_mod.c- freepath = NULL;
> > dev/hwpmc/hwpmc_mod.c- } else {
> > dev/hwpmc/hwpmc_mod.c- pmc_getfilename(p->p_textvp,
> > &fullpath, &freepath);
> > dev/hwpmc/hwpmc_mod.c- pmclog_process_pmcattach(pm,
> > p->p_pid, fullpath);
> > dev/hwpmc/hwpmc_mod.c- }
> What is wrong with this code ? proc0 has NULL p_textvp, so the call
> to pmc_getfilename() does not do anything except setting pointers to NULL.
proc0 should use the kernel as its filename, not a NULL filename. I wonder
if this bug is why 'pmcstat -t 0' in top mode doesn't show any events?
I doubt any third party code uses kthread_suspend() or kproc_suspend().
kthread/proc_suspend() already relies on the kthread in question using
kthread/proc_suspend_check() in its main loop so it doesn't work on a variety
of kthreads that have P_KTHREAD set (e.g. taskqueue threads, ithreads).
Note that thread0 does have TDP_KTHREAD set explicitly, so you can already
do 'kthread_suspend(&thread0)' without EINVAL, just not 'kproc(&proc0)'.
Both cases would hang of course since swapper() doesn't check for suspend.
Given that, I think it would be more consistent to set P_KTHREAD for proc0
than to not.
Also, P_KTHREAD should probably be renamed to P_KPROC.
--
John Baldwin
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Broken suspend-resume (suspend to RAM) with enabled INVARIANTS on 11-CURRENT - with workaround
On Sun, Feb 07, 2016 at 10:59:48PM +0100, Oliver Pinter wrote:
> On 2/6/16, Konstantin Belousov wrote:
> > On Fri, Feb 05, 2016 at 07:34:02PM +0100, Oliver Pinter wrote:
> >> Not yet tested, but possible fix:
> >>
> >> diff --git a/sys/kern/init_main.c b/sys/kern/init_main.c
> >> index cb952da..25bae84 100644
> >> --- a/sys/kern/init_main.c
> >> +++ b/sys/kern/init_main.c
> >> @@ -482,7 +482,7 @@ proc0_init(void *dummy __unused)
> >> session0.s_leader = p;
> >>
> >> p->p_sysent = &null_sysvec;
> >> - p->p_flag = P_SYSTEM | P_INMEM;
> >> + p->p_flag = P_SYSTEM | P_INMEM | P_KTHREAD;
> >> p->p_flag2 = 0;
> >> p->p_state = PRS_NORMAL;
> > So did you tested this ? Did you do an audit to see whether P_KTHREAD
> > other usages possibly conflict with the proc0 specifics ?
>
> Tested and working as expected.
In fact, I do not want to mark proc0 as P_KTHREAD. This would allow
the kthread_suspend() on the proc0, and I did not audited uses of
the KPI to guarantee the effect, also I am not sure about third-party
code which could have relied on kthread_suspend() rejecting proc0.
I do agree that the assert outlived its usefulness. I restructured
the comments to put it before stop_all_proc() and removed the assert,
together with disabling the compilation of sysctl_debug_stop_all_proc().
> Other uses would not conflict, since the codes already checks for
> P_SYSTEM and the P_KTHREAD flag is almost kern_kthread.c's "private"
> flag.
>
> And this change probably fixes one issue with hwpmc too, in the kernel case:
>
> --
> dev/hwpmc/hwpmc_mod.c-
> dev/hwpmc/hwpmc_mod.c- /* issue an attach event to a configured log file */
> dev/hwpmc/hwpmc_mod.c- if (pm->pm_owner->po_flags & PMC_PO_OWNS_LOGFILE) {
> dev/hwpmc/hwpmc_mod.c: if (p->p_flag & P_KTHREAD) {
> dev/hwpmc/hwpmc_mod.c- fullpath = kernelname;
> dev/hwpmc/hwpmc_mod.c- freepath = NULL;
> dev/hwpmc/hwpmc_mod.c- } else {
> dev/hwpmc/hwpmc_mod.c- pmc_getfilename(p->p_textvp,
> &fullpath, &freepath);
> dev/hwpmc/hwpmc_mod.c- pmclog_process_pmcattach(pm,
> p->p_pid, fullpath);
> dev/hwpmc/hwpmc_mod.c- }
What is wrong with this code ? proc0 has NULL p_textvp, so the call
to pmc_getfilename() does not do anything except setting pointers to NULL.
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Broken suspend-resume (suspend to RAM) with enabled INVARIANTS on 11-CURRENT - with workaround
On 02/05/2016 10:25, Oliver Pinter wrote: I used this gdb macro, to traverse the proc list, and print out the relevant p_flag's flags: Is it normally working though? Because every time I tried in the past I could never get it working. Yuri ___ freebsd-current@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Broken suspend-resume (suspend to RAM) with enabled INVARIANTS on 11-CURRENT - with workaround
On 2/6/16, Konstantin Belousov wrote:
> On Fri, Feb 05, 2016 at 07:34:02PM +0100, Oliver Pinter wrote:
>> Not yet tested, but possible fix:
>>
>> diff --git a/sys/kern/init_main.c b/sys/kern/init_main.c
>> index cb952da..25bae84 100644
>> --- a/sys/kern/init_main.c
>> +++ b/sys/kern/init_main.c
>> @@ -482,7 +482,7 @@ proc0_init(void *dummy __unused)
>> session0.s_leader = p;
>>
>> p->p_sysent = &null_sysvec;
>> - p->p_flag = P_SYSTEM | P_INMEM;
>> + p->p_flag = P_SYSTEM | P_INMEM | P_KTHREAD;
>> p->p_flag2 = 0;
>> p->p_state = PRS_NORMAL;
> So did you tested this ? Did you do an audit to see whether P_KTHREAD
> other usages possibly conflict with the proc0 specifics ?
Tested and working as expected.
Other uses would not conflict, since the codes already checks for
P_SYSTEM and the P_KTHREAD flag is almost kern_kthread.c's "private"
flag.
And this change probably fixes one issue with hwpmc too, in the kernel case:
--
dev/hwpmc/hwpmc_mod.c-
dev/hwpmc/hwpmc_mod.c- /* issue an attach event to a configured log file */
dev/hwpmc/hwpmc_mod.c- if (pm->pm_owner->po_flags & PMC_PO_OWNS_LOGFILE) {
dev/hwpmc/hwpmc_mod.c: if (p->p_flag & P_KTHREAD) {
dev/hwpmc/hwpmc_mod.c- fullpath = kernelname;
dev/hwpmc/hwpmc_mod.c- freepath = NULL;
dev/hwpmc/hwpmc_mod.c- } else {
dev/hwpmc/hwpmc_mod.c- pmc_getfilename(p->p_textvp,
&fullpath, &freepath);
dev/hwpmc/hwpmc_mod.c- pmclog_process_pmcattach(pm,
p->p_pid, fullpath);
dev/hwpmc/hwpmc_mod.c- }
If you want to commit this change, then please add this line: "This
work was sponsored by HardenedBSD."
>
>> #ifdef PAX
>
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Broken suspend-resume (suspend to RAM) with enabled INVARIANTS on 11-CURRENT - with workaround
On Fri, Feb 05, 2016 at 07:34:02PM +0100, Oliver Pinter wrote: > Not yet tested, but possible fix: > > diff --git a/sys/kern/init_main.c b/sys/kern/init_main.c > index cb952da..25bae84 100644 > --- a/sys/kern/init_main.c > +++ b/sys/kern/init_main.c > @@ -482,7 +482,7 @@ proc0_init(void *dummy __unused) > session0.s_leader = p; > > p->p_sysent = &null_sysvec; > - p->p_flag = P_SYSTEM | P_INMEM; > + p->p_flag = P_SYSTEM | P_INMEM | P_KTHREAD; > p->p_flag2 = 0; > p->p_state = PRS_NORMAL; So did you tested this ? Did you do an audit to see whether P_KTHREAD other usages possibly conflict with the proc0 specifics ? > #ifdef PAX ___ freebsd-current@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Broken suspend-resume (suspend to RAM) with enabled INVARIANTS on 11-CURRENT - with workaround
On 2/5/16, Oliver Pinter wrote:
> Hi all!
>
> I used this gdb macro, to traverse the proc list, and print out the
> relevant p_flag's flags:
>
> set $curr_proc=0
> def next_proc
> if $curr_proc == 0
> set $curr_proc = allproc.lh_first
> else
> set $curr_proc = $curr_proc.p_list.le_next
> end
> set $curr_p_flag = $curr_proc->p_flag
> set $curr_name = $curr_proc->p_comm
> p $curr_name
> p/x $curr_p_flag
> if ($curr_p_flag & 0x4) != 0
> printf "is P_KTHREAD\n"
> else
> printf "isn't P_KTHREAD\n"
> end
> if ($curr_p_flag & 0x00200) != 0
> printf "is P_SYSTEM\n"
> else
> printf "isn't P_SYSTEM\n"
> end
> end
>
> And seems like the "kernel" process don't have the P_KTHREAD flag,
> which fires the KASSERT in sys/kern/kern_proc.c's stop_all_proc(void)
> function.
>
> The relevant part of the code is:
> 2986 void
> 2987 stop_all_proc(void)
> 2988 {
> 2989 struct proc *cp, *p;
> 2990 int r, gen;
> 2991 bool restart, seen_stopped, seen_exiting, stopped_some;
> 2992
> 2993 cp = curproc;
> 2994 /*
> 2995 * stop_all_proc() assumes that all process which have
> 2996 * usermode must be stopped, except current process, for
> 2997 * obvious reasons. Since other threads in the process
> 2998 * establishing global stop could unstop something, disable
> 2999 * calls from multithreaded processes as precaution. The
> 3000 * service must not be user-callable anyway.
> 3001 */
> *3002 KASSERT((cp->p_flag & P_HADTHREADS) == 0 ||
> *3003 (cp->p_flag & P_KTHREAD) != 0, ("mt stop_all_proc"));
> 3004
> 3005 allproc_loop:
>
> If I comment out this KASSERT or just whitelist the "kernel" process,
> then I get a completely working suspend to ram and resume on my laptop
> (which is a HP 430G1) with one additional small tweak
> (hw.acpi.reset_video=1).
>
> So the question is that the KASSERT is bogus or too strict, or the
> P_KTHREAD flags is missing from "kernel" process?
>
> The gdb macros output is:
> [...]
> (kgdb)
> $139 = 0xf800029773c8 "rand_harvestq"
> $140 = 0x1204
> is P_KTHREAD
> is P_SYSTEM
> (kgdb)
> $141 = 0xf80002977940 "init"
> $142 = 0x10004200
> isn't P_KTHREAD
> is P_SYSTEM
> (kgdb)
> $143 = 0xf800029783c8 "audit"
> $144 = 0x1204
> is P_KTHREAD
> is P_SYSTEM
> (kgdb)
> $145 = 0x80f1b4d0 "kernel"
> $146 = 0x1280
> isn't P_KTHREAD
> is P_SYSTEM
>
> I've CC-ed Konstantin to this discussion, because he added this
> functionality to the kernel.
>
> Additional info:
> If I try to trigger the KASSERT with the debug.stop_all_proc=1 sysctl,
> then it's never fires, but when I try to put my machine to S3 with
> ctrl+alt+space then it fires.
>
Not yet tested, but possible fix:
diff --git a/sys/kern/init_main.c b/sys/kern/init_main.c
index cb952da..25bae84 100644
--- a/sys/kern/init_main.c
+++ b/sys/kern/init_main.c
@@ -482,7 +482,7 @@ proc0_init(void *dummy __unused)
session0.s_leader = p;
p->p_sysent = &null_sysvec;
- p->p_flag = P_SYSTEM | P_INMEM;
+ p->p_flag = P_SYSTEM | P_INMEM | P_KTHREAD;
p->p_flag2 = 0;
p->p_state = PRS_NORMAL;
#ifdef PAX
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Broken suspend-resume (suspend to RAM) with enabled INVARIANTS on 11-CURRENT - with workaround
Hi all!
I used this gdb macro, to traverse the proc list, and print out the
relevant p_flag's flags:
set $curr_proc=0
def next_proc
if $curr_proc == 0
set $curr_proc = allproc.lh_first
else
set $curr_proc = $curr_proc.p_list.le_next
end
set $curr_p_flag = $curr_proc->p_flag
set $curr_name = $curr_proc->p_comm
p $curr_name
p/x $curr_p_flag
if ($curr_p_flag & 0x4) != 0
printf "is P_KTHREAD\n"
else
printf "isn't P_KTHREAD\n"
end
if ($curr_p_flag & 0x00200) != 0
printf "is P_SYSTEM\n"
else
printf "isn't P_SYSTEM\n"
end
end
And seems like the "kernel" process don't have the P_KTHREAD flag,
which fires the KASSERT in sys/kern/kern_proc.c's stop_all_proc(void)
function.
The relevant part of the code is:
2986 void
2987 stop_all_proc(void)
2988 {
2989 struct proc *cp, *p;
2990 int r, gen;
2991 bool restart, seen_stopped, seen_exiting, stopped_some;
2992
2993 cp = curproc;
2994 /*
2995 * stop_all_proc() assumes that all process which have
2996 * usermode must be stopped, except current process, for
2997 * obvious reasons. Since other threads in the process
2998 * establishing global stop could unstop something, disable
2999 * calls from multithreaded processes as precaution. The
3000 * service must not be user-callable anyway.
3001 */
*3002 KASSERT((cp->p_flag & P_HADTHREADS) == 0 ||
*3003 (cp->p_flag & P_KTHREAD) != 0, ("mt stop_all_proc"));
3004
3005 allproc_loop:
If I comment out this KASSERT or just whitelist the "kernel" process,
then I get a completely working suspend to ram and resume on my laptop
(which is a HP 430G1) with one additional small tweak
(hw.acpi.reset_video=1).
So the question is that the KASSERT is bogus or too strict, or the
P_KTHREAD flags is missing from "kernel" process?
The gdb macros output is:
[...]
(kgdb)
$139 = 0xf800029773c8 "rand_harvestq"
$140 = 0x1204
is P_KTHREAD
is P_SYSTEM
(kgdb)
$141 = 0xf80002977940 "init"
$142 = 0x10004200
isn't P_KTHREAD
is P_SYSTEM
(kgdb)
$143 = 0xf800029783c8 "audit"
$144 = 0x1204
is P_KTHREAD
is P_SYSTEM
(kgdb)
$145 = 0x80f1b4d0 "kernel"
$146 = 0x1280
isn't P_KTHREAD
is P_SYSTEM
I've CC-ed Konstantin to this discussion, because he added this
functionality to the kernel.
Additional info:
If I try to trigger the KASSERT with the debug.stop_all_proc=1 sysctl,
then it's never fires, but when I try to put my machine to S3 with
ctrl+alt+space then it fires.
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
