Re: ldd: check read return value to avoid unitialized struct fields
Greg Steuck wrote: > Thanks for the patch. > > I could see some value in tightening the conditions to always check > `!= expected`. I don't see enough improvement from separating the error > case of -1 from the incomplete read case considering the otherwise > identical behavior. Hmm, that is a valid point. We want this code to be as simple as possible, and the effect of these errors is identical.
Re: hardclock(9), roundrobin: make roundrobin() an independent clock interrupt
On Thu, Aug 10, 2023 at 07:32:05PM +0200, Martin Pieuchot wrote:
> On 10/08/23(Thu) 12:18, Scott Cheloha wrote:
> > On Thu, Aug 10, 2023 at 01:05:27PM +0200, Martin Pieuchot wrote:
> > [...]
> > > Can we get rid of `hardclock_period' and use a variable set to 100ms?
> > > This should be tested on alpha which has a hz of 1024 but I'd argue this
> > > is an improvement.
> >
> > Sure, that's cleaner. The updated patch below adds a new
> > "roundrobin_period" variable initialized during clockintr_init().
>
> I'd rather see this variable initialized in sched_bsd.c to 100ms without
> depending on `hz'. Is is possible? My point is to untangle this completely
> from `hz'.
Yes, but we need to do that in a separate patch. This patch isolates
roundrobin() from hardclock() without changing any other behavior.
We can then use the isolated roundrobin() as a known-working starting
point for e.g.:
const uint32_t roundrobin_period = 1; /* 100ms */
Index: kern/sched_bsd.c
===
RCS file: /cvs/src/sys/kern/sched_bsd.c,v
retrieving revision 1.79
diff -u -p -r1.79 sched_bsd.c
--- kern/sched_bsd.c5 Aug 2023 20:07:55 - 1.79
+++ kern/sched_bsd.c11 Aug 2023 02:47:03 -
@@ -54,9 +54,8 @@
#include
#endif
-
+uint32_t roundrobin_period;/* [I] roundrobin period (ns) */
intlbolt; /* once a second sleep address */
-intrrticks_init; /* # of hardclock ticks per roundrobin() */
#ifdef MULTIPROCESSOR
struct __mp_lock sched_lock;
@@ -69,21 +68,23 @@ uint32_tdecay_aftersleep(uint32_t, uin
* Force switch among equal priority processes every 100ms.
*/
void
-roundrobin(struct cpu_info *ci)
+roundrobin(struct clockintr *cl, void *cf)
{
+ struct cpu_info *ci = curcpu();
struct schedstate_percpu *spc = >ci_schedstate;
+ uint64_t count;
- spc->spc_rrticks = rrticks_init;
+ count = clockintr_advance(cl, roundrobin_period);
if (ci->ci_curproc != NULL) {
- if (spc->spc_schedflags & SPCF_SEENRR) {
+ if (spc->spc_schedflags & SPCF_SEENRR || count >= 2) {
/*
* The process has already been through a roundrobin
* without switching and may be hogging the CPU.
* Indicate that the process should yield.
*/
atomic_setbits_int(>spc_schedflags,
- SPCF_SHOULDYIELD);
+ SPCF_SEENRR | SPCF_SHOULDYIELD);
} else {
atomic_setbits_int(>spc_schedflags,
SPCF_SEENRR);
@@ -695,8 +696,6 @@ scheduler_start(void)
* its job.
*/
timeout_set(_to, schedcpu, _to);
-
- rrticks_init = hz / 10;
schedcpu(_to);
#ifndef SMALL_KERNEL
Index: kern/kern_sched.c
===
RCS file: /cvs/src/sys/kern/kern_sched.c,v
retrieving revision 1.84
diff -u -p -r1.84 kern_sched.c
--- kern/kern_sched.c 5 Aug 2023 20:07:55 - 1.84
+++ kern/kern_sched.c 11 Aug 2023 02:47:03 -
@@ -102,6 +102,12 @@ sched_init_cpu(struct cpu_info *ci)
if (spc->spc_profclock == NULL)
panic("%s: clockintr_establish profclock", __func__);
}
+ if (spc->spc_roundrobin == NULL) {
+ spc->spc_roundrobin = clockintr_establish(>ci_queue,
+ roundrobin);
+ if (spc->spc_roundrobin == NULL)
+ panic("%s: clockintr_establish roundrobin", __func__);
+ }
kthread_create_deferred(sched_kthreads_create, ci);
Index: kern/kern_clockintr.c
===
RCS file: /cvs/src/sys/kern/kern_clockintr.c,v
retrieving revision 1.30
diff -u -p -r1.30 kern_clockintr.c
--- kern/kern_clockintr.c 5 Aug 2023 20:07:55 - 1.30
+++ kern/kern_clockintr.c 11 Aug 2023 02:47:03 -
@@ -69,6 +69,7 @@ clockintr_init(u_int flags)
KASSERT(hz > 0 && hz <= 10);
hardclock_period = 10 / hz;
+ roundrobin_period = hardclock_period * 10;
KASSERT(stathz >= 1 && stathz <= 10);
@@ -204,6 +205,11 @@ clockintr_cpu_init(const struct intrcloc
clockintr_stagger(spc->spc_profclock, profclock_period,
multiplier, MAXCPUS);
}
+ if (spc->spc_roundrobin->cl_expiration == 0) {
+ clockintr_stagger(spc->spc_roundrobin, hardclock_period,
+ multiplier, MAXCPUS);
+ }
+ clockintr_advance(spc->spc_roundrobin, roundrobin_period);
if (reset_cq_intrclock)
SET(cq->cq_flags, CQ_INTRCLOCK);
Index: kern/kern_clock.c
===
RCS file:
Re: ldd: check read return value to avoid unitialized struct fields
Greg Steuck wrote:
> Thanks for the patch.
>
> I could see some value in tightening the conditions to always check
> `!= expected`. I don't see enough improvement from separating the error
> case of -1 from the incomplete read case considering the otherwise
> identical behavior.
I'll refer to https://marc.info/?l=openbsd-tech=169120238326202=2 :
> What errno is being printed here?
So then it becomes a matter of wanting to signal the user that something
went wrong other than a short read.
Will it be fine to fold into `!= expected` and warning that an
incomplete {ELF,program} header was encountered (the warnx)?
-Lucas
Re: ldd: check read return value to avoid unitialized struct fields
Thanks for the patch.
I could see some value in tightening the conditions to always check
`!= expected`. I don't see enough improvement from separating the error
case of -1 from the incomplete read case considering the otherwise
identical behavior.
Lucas writes:
> Bump.
>
> ---
> commit 92f58b2a1cd576c3e72303004388ab1e9709e327 (ldd-read-rv)
> from: Lucas
> date: Sat Aug 5 16:34:16 2023 UTC
>
> Check {,p}read return values consistently
>
> Check that read performs a full header read. Explicitly check against -1
> for failure instead of < 0. Split pread error message between error
> handling and short reads. Promote size from int to size_t.
>
> M libexec/ld.so/ldd/ldd.c
>
> diff 194ff02fb6be247e27fe964e901d891d99ec0b74
> 92f58b2a1cd576c3e72303004388ab1e9709e327
> commit - 194ff02fb6be247e27fe964e901d891d99ec0b74
> commit + 92f58b2a1cd576c3e72303004388ab1e9709e327
> blob - 9e8c5065cd843ff36d91efcb868b94ffd4c98365
> blob + 532feb9855a03480c289fa2188768657a4f82e7c
> --- libexec/ld.so/ldd/ldd.c
> +++ libexec/ld.so/ldd/ldd.c
> @@ -96,7 +96,9 @@ doit(char *name)
> {
> Elf_Ehdr ehdr;
> Elf_Phdr *phdr;
> - int fd, i, size, status, interp=0;
> + size_t size;
> + ssize_t nr;
> + int fd, i, status, interp=0;
> char buf[PATH_MAX];
> struct stat st;
> void * dlhandle;
> @@ -118,11 +120,16 @@ doit(char *name)
> return 1;
> }
>
> - if (read(fd, , sizeof(ehdr)) < 0) {
> + if ((nr = read(fd, , sizeof(ehdr))) == -1) {
> warn("read(%s)", name);
> close(fd);
> return 1;
> }
> + if (nr != sizeof(ehdr)) {
> + warnx("%s: incomplete ELF header", name);
> + close(fd);
> + return 1;
> + }
>
> if (!IS_ELF(ehdr) || ehdr.e_machine != ELF_TARG_MACH) {
> warnx("%s: not an ELF executable", name);
> @@ -140,12 +147,18 @@ doit(char *name)
> err(1, "reallocarray");
> size = ehdr.e_phnum * sizeof(Elf_Phdr);
>
> - if (pread(fd, phdr, size, ehdr.e_phoff) != size) {
> - warn("read(%s)", name);
> + if ((nr = pread(fd, phdr, size, ehdr.e_phoff)) == -1) {
> + warn("pread(%s)", name);
> close(fd);
> free(phdr);
> return 1;
> }
> + if (nr != size) {
> + warnx("%s: incomplete program header", name);
> + close(fd);
> + free(phdr);
> + return 1;
> + }
> close(fd);
>
> for (i = 0; i < ehdr.e_phnum; i++)
Re: ldd: check read return value to avoid unitialized struct fields
Bump.
---
commit 92f58b2a1cd576c3e72303004388ab1e9709e327 (ldd-read-rv)
from: Lucas
date: Sat Aug 5 16:34:16 2023 UTC
Check {,p}read return values consistently
Check that read performs a full header read. Explicitly check against -1
for failure instead of < 0. Split pread error message between error
handling and short reads. Promote size from int to size_t.
M libexec/ld.so/ldd/ldd.c
diff 194ff02fb6be247e27fe964e901d891d99ec0b74
92f58b2a1cd576c3e72303004388ab1e9709e327
commit - 194ff02fb6be247e27fe964e901d891d99ec0b74
commit + 92f58b2a1cd576c3e72303004388ab1e9709e327
blob - 9e8c5065cd843ff36d91efcb868b94ffd4c98365
blob + 532feb9855a03480c289fa2188768657a4f82e7c
--- libexec/ld.so/ldd/ldd.c
+++ libexec/ld.so/ldd/ldd.c
@@ -96,7 +96,9 @@ doit(char *name)
{
Elf_Ehdr ehdr;
Elf_Phdr *phdr;
- int fd, i, size, status, interp=0;
+ size_t size;
+ ssize_t nr;
+ int fd, i, status, interp=0;
char buf[PATH_MAX];
struct stat st;
void * dlhandle;
@@ -118,11 +120,16 @@ doit(char *name)
return 1;
}
- if (read(fd, , sizeof(ehdr)) < 0) {
+ if ((nr = read(fd, , sizeof(ehdr))) == -1) {
warn("read(%s)", name);
close(fd);
return 1;
}
+ if (nr != sizeof(ehdr)) {
+ warnx("%s: incomplete ELF header", name);
+ close(fd);
+ return 1;
+ }
if (!IS_ELF(ehdr) || ehdr.e_machine != ELF_TARG_MACH) {
warnx("%s: not an ELF executable", name);
@@ -140,12 +147,18 @@ doit(char *name)
err(1, "reallocarray");
size = ehdr.e_phnum * sizeof(Elf_Phdr);
- if (pread(fd, phdr, size, ehdr.e_phoff) != size) {
- warn("read(%s)", name);
+ if ((nr = pread(fd, phdr, size, ehdr.e_phoff)) == -1) {
+ warn("pread(%s)", name);
close(fd);
free(phdr);
return 1;
}
+ if (nr != size) {
+ warnx("%s: incomplete program header", name);
+ close(fd);
+ free(phdr);
+ return 1;
+ }
close(fd);
for (i = 0; i < ehdr.e_phnum; i++)
Re: agtimer(4/arm64): simplify agtimer_delay()
> Date: Thu, 10 Aug 2023 16:01:10 -0500
> From: Scott Cheloha
>
> On Tue, Aug 08, 2023 at 08:00:47PM +0200, Mark Kettenis wrote:
> > > From: Dale Rahn
> > > Date: Tue, 8 Aug 2023 12:36:45 -0400
> > >
> > > Switching the computation of cycles/delaycnt to a proper 64 value math
> > > instead of the '32 bit safe' complex math is likely a good idea.
> > > However I am not completely convinced that switching to 'yield' (current
> > > CPU_BUSY_CYCLE implementation) for every loop of a 'short wait' in the
> > > wait
> > > loop makes sense. In a hypervisor environment, this could cause a very
> > > short wait between register writes to become very long, In a
> > > non-hypervisor
> > > environment there is essentially no improvement because the yield doesn't
> > > really have any benefits on non-hypervisor.
> >
> > Dale, I think you're confused here. There is no arcitectural way to
> > trap a YIELD instruction. The docs explicitly state that SMT and SMP
> > systems. I suspect that this instruction was primarily introduced for
> > SMT systems that never materialized; I completely believe you that on
> > current hardware it does nothing.
> >
> > Even without a YIELD instruction a hypervisor might interrupt us and
> > schedule a different vCPU onto the core. And on real hardware
> > external interrupts may happen. So you really can't count on delay(9)
> > being accurate.
> >
> > Linux does use YIELD in it delay loop.
>
> Okay good.
>
> > > To my current understanding, there is no useful 'wait short period' on arm
> > > cores.
> >
> > There is WFET (and WFIT), but that is Armv8.7 material, so not
> > availble on actual hardware that we run on. Linux uses that
> > instruction in its delay loop when available.
>
> These look great. Can't wait for them to be implemented outside of a
> functional simulator.
>
> > On machines with a working Generic Timer event stream, Linux uses WFE
> > if the delay is long enough.
>
> I have a prototype for this. Obviously it's a separate patch.
>
> Anyway, can I go ahead with the patch below to start?
>
> - Use 64-bit arithmetic to simplify agtimer_delay().
>
> - Use CPU_BUSY_CYCLE() ("yield" on arm64) in the loop to match other
> delay(9) implementations.
ok kettenis@
> Index: agtimer.c
> ===
> RCS file: /cvs/src/sys/arch/arm64/dev/agtimer.c,v
> retrieving revision 1.23
> diff -u -p -r1.23 agtimer.c
> --- agtimer.c 25 Jul 2023 18:16:19 - 1.23
> +++ agtimer.c 10 Aug 2023 20:59:27 -
> @@ -323,32 +323,12 @@ agtimer_cpu_initclocks(void)
> void
> agtimer_delay(u_int usecs)
> {
> - uint64_tclock, oclock, delta, delaycnt;
> - uint64_tcsec, usec;
> - volatile intj;
> + uint64_t cycles, start;
>
> - if (usecs > (0x8000 / agtimer_frequency)) {
> - csec = usecs / 1;
> - usec = usecs % 1;
> -
> - delaycnt = (agtimer_frequency / 100) * csec +
> - (agtimer_frequency / 100) * usec / 1;
> - } else {
> - delaycnt = agtimer_frequency * usecs / 100;
> - }
> - if (delaycnt <= 1)
> - for (j = 100; j > 0; j--)
> - ;
> -
> - oclock = agtimer_readcnt64();
> - while (1) {
> - for (j = 100; j > 0; j--)
> - ;
> - clock = agtimer_readcnt64();
> - delta = clock - oclock;
> - if (delta > delaycnt)
> - break;
> - }
> + start = agtimer_readcnt64();
> + cycles = (uint64_t)usecs * agtimer_frequency / 100;
> + while (agtimer_readcnt64() - start < cycles)
> + CPU_BUSY_CYCLE();
> }
>
> void
>
>
Re: agtimer(4/arm64): simplify agtimer_delay()
On Tue, Aug 08, 2023 at 08:00:47PM +0200, Mark Kettenis wrote:
> > From: Dale Rahn
> > Date: Tue, 8 Aug 2023 12:36:45 -0400
> >
> > Switching the computation of cycles/delaycnt to a proper 64 value math
> > instead of the '32 bit safe' complex math is likely a good idea.
> > However I am not completely convinced that switching to 'yield' (current
> > CPU_BUSY_CYCLE implementation) for every loop of a 'short wait' in the wait
> > loop makes sense. In a hypervisor environment, this could cause a very
> > short wait between register writes to become very long, In a non-hypervisor
> > environment there is essentially no improvement because the yield doesn't
> > really have any benefits on non-hypervisor.
>
> Dale, I think you're confused here. There is no arcitectural way to
> trap a YIELD instruction. The docs explicitly state that SMT and SMP
> systems. I suspect that this instruction was primarily introduced for
> SMT systems that never materialized; I completely believe you that on
> current hardware it does nothing.
>
> Even without a YIELD instruction a hypervisor might interrupt us and
> schedule a different vCPU onto the core. And on real hardware
> external interrupts may happen. So you really can't count on delay(9)
> being accurate.
>
> Linux does use YIELD in it delay loop.
Okay good.
> > To my current understanding, there is no useful 'wait short period' on arm
> > cores.
>
> There is WFET (and WFIT), but that is Armv8.7 material, so not
> availble on actual hardware that we run on. Linux uses that
> instruction in its delay loop when available.
These look great. Can't wait for them to be implemented outside of a
functional simulator.
> On machines with a working Generic Timer event stream, Linux uses WFE
> if the delay is long enough.
I have a prototype for this. Obviously it's a separate patch.
Anyway, can I go ahead with the patch below to start?
- Use 64-bit arithmetic to simplify agtimer_delay().
- Use CPU_BUSY_CYCLE() ("yield" on arm64) in the loop to match other
delay(9) implementations.
Index: agtimer.c
===
RCS file: /cvs/src/sys/arch/arm64/dev/agtimer.c,v
retrieving revision 1.23
diff -u -p -r1.23 agtimer.c
--- agtimer.c 25 Jul 2023 18:16:19 - 1.23
+++ agtimer.c 10 Aug 2023 20:59:27 -
@@ -323,32 +323,12 @@ agtimer_cpu_initclocks(void)
void
agtimer_delay(u_int usecs)
{
- uint64_tclock, oclock, delta, delaycnt;
- uint64_tcsec, usec;
- volatile intj;
+ uint64_t cycles, start;
- if (usecs > (0x8000 / agtimer_frequency)) {
- csec = usecs / 1;
- usec = usecs % 1;
-
- delaycnt = (agtimer_frequency / 100) * csec +
- (agtimer_frequency / 100) * usec / 1;
- } else {
- delaycnt = agtimer_frequency * usecs / 100;
- }
- if (delaycnt <= 1)
- for (j = 100; j > 0; j--)
- ;
-
- oclock = agtimer_readcnt64();
- while (1) {
- for (j = 100; j > 0; j--)
- ;
- clock = agtimer_readcnt64();
- delta = clock - oclock;
- if (delta > delaycnt)
- break;
- }
+ start = agtimer_readcnt64();
+ cycles = (uint64_t)usecs * agtimer_frequency / 100;
+ while (agtimer_readcnt64() - start < cycles)
+ CPU_BUSY_CYCLE();
}
void
plans for DISTFILES/MASTER_SITES
Now that we got .VARIABLES in make, this is going to be put to great use
in bsd.port.mk.
Namely, instead of the currently (very clunky)
DISTFILES = sometarball:0
MASTER_SITES0 = where to get this
I propose we do something like:
DISTFILES.site = sometarball
MASTER_SITES.site = where to get this
Pros:
- infinite number of alternate sites if need be
- less cumbersome syntax
Before .VARIABLES, there would need to be a way to list those suffixes,
but now, it's about as complicated as
${.VARIABLES:MDISTFILES*}
the bsd.port.mk are reasonably trivial. Repercussions to sqlports and dpb
need a bit more work (it's not complicated, it just needs to be dealt with
This ought to lift a lot of limitations eventually, along with thfr work
(still morphing thru lots of feedback)
Some of the basic support ought to hit the ports tree soon, the rest
requires make's .VARIABLES support to be generally available in snapshots.
Re: hardclock(9), roundrobin: make roundrobin() an independent clock interrupt
On 10/08/23(Thu) 12:18, Scott Cheloha wrote:
> On Thu, Aug 10, 2023 at 01:05:27PM +0200, Martin Pieuchot wrote:
> [...]
> > Can we get rid of `hardclock_period' and use a variable set to 100ms?
> > This should be tested on alpha which has a hz of 1024 but I'd argue this
> > is an improvement.
>
> Sure, that's cleaner. The updated patch below adds a new
> "roundrobin_period" variable initialized during clockintr_init().
I'd rather see this variable initialized in sched_bsd.c to 100ms without
depending on `hz'. Is is possible? My point is to untangle this completely
from `hz'.
> Index: kern/sched_bsd.c
> ===
> RCS file: /cvs/src/sys/kern/sched_bsd.c,v
> retrieving revision 1.79
> diff -u -p -r1.79 sched_bsd.c
> --- kern/sched_bsd.c 5 Aug 2023 20:07:55 - 1.79
> +++ kern/sched_bsd.c 10 Aug 2023 17:15:53 -
> @@ -54,9 +54,8 @@
> #include
> #endif
>
> -
> +uint32_t roundrobin_period; /* [I] roundrobin period (ns) */
> int lbolt; /* once a second sleep address */
> -int rrticks_init; /* # of hardclock ticks per roundrobin() */
>
> #ifdef MULTIPROCESSOR
> struct __mp_lock sched_lock;
> @@ -69,21 +68,23 @@ uint32_t decay_aftersleep(uint32_t, uin
> * Force switch among equal priority processes every 100ms.
> */
> void
> -roundrobin(struct cpu_info *ci)
> +roundrobin(struct clockintr *cl, void *cf)
> {
> + struct cpu_info *ci = curcpu();
> struct schedstate_percpu *spc = >ci_schedstate;
> + uint64_t count;
>
> - spc->spc_rrticks = rrticks_init;
> + count = clockintr_advance(cl, roundrobin_period);
>
> if (ci->ci_curproc != NULL) {
> - if (spc->spc_schedflags & SPCF_SEENRR) {
> + if (spc->spc_schedflags & SPCF_SEENRR || count >= 2) {
> /*
>* The process has already been through a roundrobin
>* without switching and may be hogging the CPU.
>* Indicate that the process should yield.
>*/
> atomic_setbits_int(>spc_schedflags,
> - SPCF_SHOULDYIELD);
> + SPCF_SEENRR | SPCF_SHOULDYIELD);
> } else {
> atomic_setbits_int(>spc_schedflags,
> SPCF_SEENRR);
> @@ -695,8 +696,6 @@ scheduler_start(void)
>* its job.
>*/
> timeout_set(_to, schedcpu, _to);
> -
> - rrticks_init = hz / 10;
> schedcpu(_to);
>
> #ifndef SMALL_KERNEL
> Index: kern/kern_sched.c
> ===
> RCS file: /cvs/src/sys/kern/kern_sched.c,v
> retrieving revision 1.84
> diff -u -p -r1.84 kern_sched.c
> --- kern/kern_sched.c 5 Aug 2023 20:07:55 - 1.84
> +++ kern/kern_sched.c 10 Aug 2023 17:15:53 -
> @@ -102,6 +102,12 @@ sched_init_cpu(struct cpu_info *ci)
> if (spc->spc_profclock == NULL)
> panic("%s: clockintr_establish profclock", __func__);
> }
> + if (spc->spc_roundrobin == NULL) {
> + spc->spc_roundrobin = clockintr_establish(>ci_queue,
> + roundrobin);
> + if (spc->spc_roundrobin == NULL)
> + panic("%s: clockintr_establish roundrobin", __func__);
> + }
>
> kthread_create_deferred(sched_kthreads_create, ci);
>
> Index: kern/kern_clockintr.c
> ===
> RCS file: /cvs/src/sys/kern/kern_clockintr.c,v
> retrieving revision 1.30
> diff -u -p -r1.30 kern_clockintr.c
> --- kern/kern_clockintr.c 5 Aug 2023 20:07:55 - 1.30
> +++ kern/kern_clockintr.c 10 Aug 2023 17:15:53 -
> @@ -69,6 +69,7 @@ clockintr_init(u_int flags)
>
> KASSERT(hz > 0 && hz <= 10);
> hardclock_period = 10 / hz;
> + roundrobin_period = hardclock_period * 10;
>
> KASSERT(stathz >= 1 && stathz <= 10);
>
> @@ -204,6 +205,11 @@ clockintr_cpu_init(const struct intrcloc
> clockintr_stagger(spc->spc_profclock, profclock_period,
> multiplier, MAXCPUS);
> }
> + if (spc->spc_roundrobin->cl_expiration == 0) {
> + clockintr_stagger(spc->spc_roundrobin, hardclock_period,
> + multiplier, MAXCPUS);
> + }
> + clockintr_advance(spc->spc_roundrobin, roundrobin_period);
>
> if (reset_cq_intrclock)
> SET(cq->cq_flags, CQ_INTRCLOCK);
> Index: kern/kern_clock.c
> ===
> RCS file: /cvs/src/sys/kern/kern_clock.c,v
> retrieving revision 1.111
> diff -u -p -r1.111 kern_clock.c
> --- kern/kern_clock.c 5 Aug 2023 20:07:55 - 1.111
> +++ kern/kern_clock.c 10 Aug 2023 17:15:54 -
> @@ -113,9 +113,6 @@ hardclock(struct clockframe *frame)
> {
> struct cpu_info
Re: installer: proper disk crypto passphrase prompt loop
On Wed, Aug 02, 2023 at 11:38:57AM +, Klemens Nanni wrote:
> This needs "bioctl: do not confirm new passphrases on stdin" on tech@.
>
> Current code tries thrice to get matching passphrases before aborting;
> simple enough to get the feature going, but also due to code limitations.
>
> One possible fix is to let the installer (not bioctl) prompt the passphrase
> like it does for the root password and pass it to bioctl non-interactively.
>
> This means
> * a familiar question style and endless retry behaviour, not bioctl's prompt
> * manual empty string check, bioctl already it
> * installer duplicates existing bioctl prompt functionality
>
>
> Setting OpenBSD MBR partition to whole sd0...done.
> -New passphrase:
> -Re-type passphrase:
> +Passphrase for the root disk? (again)
> +Passphrase for the root disk? (will not echo)
> sd1 at scsibus1 targ 1 lun 0:
>
>
> Feedback?
Ping. Rebased after -Cforce landed.
Index: install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1253
diff -u -p -r1.1253 install.sub
--- install.sub 10 Aug 2023 17:09:34 - 1.1253
+++ install.sub 10 Aug 2023 17:19:57 -
@@ -3075,7 +3075,7 @@ do_autoinstall() {
}
encrypt_root() {
- local _chunk _tries=0
+ local _chunk
[[ $MDBOOTSR == y ]] || return
@@ -3097,10 +3097,12 @@ encrypt_root() {
md_prep_fdisk $_chunk
echo 'RAID *' | disklabel -w -A -T- $_chunk
- until bioctl -Cforce -cC -l${_chunk}a softraid0 >/dev/null; do
- # Most likely botched passphrases, silently retry twice.
- ((++_tries < 3)) || exit
- done
+ while :; do
+ ask_password 'Passphrase for the root disk?'
+ [[ -n "$_password" ]] && break
+ echo 'The passphrase must be set.'
+
+ print -r -- "$_password" | bioctl -s -Cforce -cC -l${_chunk}a softraid0
>/dev/null
# No volumes existed before asking, but we just created one.
ROOTDISK=$(get_softraid_volumes)
Re: hardclock(9), roundrobin: make roundrobin() an independent clock interrupt
On Thu, Aug 10, 2023 at 01:05:27PM +0200, Martin Pieuchot wrote:
> On 05/08/23(Sat) 17:17, Scott Cheloha wrote:
> > This is the next piece of the clock interrupt reorganization patch
> > series.
>
> The round robin logic is here to make sure process doesn't hog a CPU.
> The period to tell a process it should yield doesn't have to be tied
> to the hardclock period. We want to be sure a process doesn't run more
> than 100ms at a time.
> Is the priority of this new clock interrupt the same as the hardlock?
Yes. Clock interrupts on a given CPU are dispatched in order of
expiration. If two clock interrupts on the same CPU have the same
expiration value they are dispatched in FIFO order.
> I don't understand what clockintr_advance() is doing. Maybe you could
> write a manual for it?
clockintr_advance() is a convenience wrapper for clockintr_schedule().
It reschedules periodic interrupts without drift.
The manpage update is a work in progress.
> I'm afraid we could wait 200ms now? Or what `count' of 2 mean?
No. roundrobin() is still scheduled to run every 100ms. The code
change ensures we properly account for situations where roundrobin()
is so late that two or more roundrobin periods have elapsed:
> @@ -69,21 +68,23 @@ uint32_t decay_aftersleep(uint32_t, uin
> * Force switch among equal priority processes every 100ms.
> */
> void
> -roundrobin(struct cpu_info *ci)
> +roundrobin(struct clockintr *cl, void *cf)
> {
> + struct cpu_info *ci = curcpu();
> struct schedstate_percpu *spc = >ci_schedstate;
> + uint64_t count;
>
> - spc->spc_rrticks = rrticks_init;
> + count = clockintr_advance(cl, roundrobin_period);
>
> if (ci->ci_curproc != NULL) {
> - if (spc->spc_schedflags & SPCF_SEENRR) {
> + if (spc->spc_schedflags & SPCF_SEENRR || count >= 2) {
> /*
>* The process has already been through a roundrobin
>* without switching and may be hogging the CPU.
>* Indicate that the process should yield.
>*/
> atomic_setbits_int(>spc_schedflags,
> - SPCF_SHOULDYIELD);
> + SPCF_SEENRR | SPCF_SHOULDYIELD);
> } else {
> atomic_setbits_int(>spc_schedflags,
> SPCF_SEENRR);
In such a situation, we want to set both SPCF_SEENRR and
SPCF_SHOULDYIELD on the thread. This simulates what would have
happened under normal circumstances, i.e. the thread would have
been interrupted by roundrobin() two separate times.
> Same question for clockintr_stagger().
clockintr_stagger() adjusts the starting offset for the given clock
interrupt. We use it to keep identical clock interrupts from expiring
simultaneously across every CPU in the system.
> Can we get rid of `hardclock_period' and use a variable set to 100ms?
> This should be tested on alpha which has a hz of 1024 but I'd argue this
> is an improvement.
Sure, that's cleaner. The updated patch below adds a new
"roundrobin_period" variable initialized during clockintr_init().
Index: kern/sched_bsd.c
===
RCS file: /cvs/src/sys/kern/sched_bsd.c,v
retrieving revision 1.79
diff -u -p -r1.79 sched_bsd.c
--- kern/sched_bsd.c5 Aug 2023 20:07:55 - 1.79
+++ kern/sched_bsd.c10 Aug 2023 17:15:53 -
@@ -54,9 +54,8 @@
#include
#endif
-
+uint32_t roundrobin_period;/* [I] roundrobin period (ns) */
intlbolt; /* once a second sleep address */
-intrrticks_init; /* # of hardclock ticks per roundrobin() */
#ifdef MULTIPROCESSOR
struct __mp_lock sched_lock;
@@ -69,21 +68,23 @@ uint32_tdecay_aftersleep(uint32_t, uin
* Force switch among equal priority processes every 100ms.
*/
void
-roundrobin(struct cpu_info *ci)
+roundrobin(struct clockintr *cl, void *cf)
{
+ struct cpu_info *ci = curcpu();
struct schedstate_percpu *spc = >ci_schedstate;
+ uint64_t count;
- spc->spc_rrticks = rrticks_init;
+ count = clockintr_advance(cl, roundrobin_period);
if (ci->ci_curproc != NULL) {
- if (spc->spc_schedflags & SPCF_SEENRR) {
+ if (spc->spc_schedflags & SPCF_SEENRR || count >= 2) {
/*
* The process has already been through a roundrobin
* without switching and may be hogging the CPU.
* Indicate that the process should yield.
*/
atomic_setbits_int(>spc_schedflags,
- SPCF_SHOULDYIELD);
+ SPCF_SEENRR | SPCF_SHOULDYIELD);
} else {
atomic_setbits_int(>spc_schedflags,
SPCF_SEENRR);
@@ -695,8 +696,6 @@
Re: hardclock(9), roundrobin: make roundrobin() an independent clock interrupt
On 05/08/23(Sat) 17:17, Scott Cheloha wrote:
> This is the next piece of the clock interrupt reorganization patch
> series.
The round robin logic is here to make sure process doesn't hog a CPU.
The period to tell a process it should yield doesn't have to be tied
to the hardclock period. We want to be sure a process doesn't run more
than 100ms at a time.
Is the priority of this new clock interrupt the same as the hardlock?
I don't understand what clockintr_advance() is doing. Maybe you could
write a manual for it? I'm afraid we could wait 200ms now? Or what
`count' of 2 mean?
Same question for clockintr_stagger().
Can we get rid of `hardclock_period' and use a variable set to 100ms?
This should be tested on alpha which has a hz of 1024 but I'd argue this
is an improvement.
> This patch removes the roundrobin() call from hardclock() and makes
> roundrobin() an independent clock interrupt.
>
> - Revise roundrobin() to make it a valid clock interrupt callback.
> It remains periodic. It still runs at one tenth of the hardclock
> frequency.
>
> - Account for multiple expirations in roundrobin(). If two or more
> intervals have elapsed we set SPCF_SHOULDYIELD immediately.
>
> This preserves existing behavior: hardclock() is called multiple
> times during clockintr_hardclock() if clock interrupts are blocked
> for long enough.
>
> - Each schedstate_percpu has its own roundrobin() handle, spc_roundrobin.
> spc_roundrobin is established during sched_init_cpu(), staggered during
> the first clockintr_cpu_init() call, and advanced during
> clockintr_cpu_init().
> Expirations during suspend/resume are discarded.
>
> - spc_rrticks and rrticks_init are now useless. Delete them.
>
> ok?
>
> Also, yes, I see the growing pile of scheduler-controlled clock
> interrupt handles. My current plan is to move the setup code at the
> end of clockintr_cpu_init() to a different routine, maybe something
> like "sched_start_cpu()". On the primary CPU you'd call it immediately
> after cpu_initclocks(). On secondary CPUs you'd call it at the end of
> cpu_hatch() just before cpu_switchto().
>
> In any case, we will need to find a home for that code someplace. It
> can't stay in clockintr_cpu_init() forever.
>
> Index: kern/sched_bsd.c
> ===
> RCS file: /cvs/src/sys/kern/sched_bsd.c,v
> retrieving revision 1.79
> diff -u -p -r1.79 sched_bsd.c
> --- kern/sched_bsd.c 5 Aug 2023 20:07:55 - 1.79
> +++ kern/sched_bsd.c 5 Aug 2023 22:15:25 -
> @@ -56,7 +56,6 @@
>
>
> int lbolt; /* once a second sleep address */
> -int rrticks_init; /* # of hardclock ticks per roundrobin() */
>
> #ifdef MULTIPROCESSOR
> struct __mp_lock sched_lock;
> @@ -69,21 +68,23 @@ uint32_t decay_aftersleep(uint32_t, uin
> * Force switch among equal priority processes every 100ms.
> */
> void
> -roundrobin(struct cpu_info *ci)
> +roundrobin(struct clockintr *cl, void *cf)
> {
> + struct cpu_info *ci = curcpu();
> struct schedstate_percpu *spc = >ci_schedstate;
> + uint64_t count;
>
> - spc->spc_rrticks = rrticks_init;
> + count = clockintr_advance(cl, hardclock_period * 10);
>
> if (ci->ci_curproc != NULL) {
> - if (spc->spc_schedflags & SPCF_SEENRR) {
> + if (spc->spc_schedflags & SPCF_SEENRR || count >= 2) {
> /*
>* The process has already been through a roundrobin
>* without switching and may be hogging the CPU.
>* Indicate that the process should yield.
>*/
> atomic_setbits_int(>spc_schedflags,
> - SPCF_SHOULDYIELD);
> + SPCF_SEENRR | SPCF_SHOULDYIELD);
> } else {
> atomic_setbits_int(>spc_schedflags,
> SPCF_SEENRR);
> @@ -695,8 +696,6 @@ scheduler_start(void)
>* its job.
>*/
> timeout_set(_to, schedcpu, _to);
> -
> - rrticks_init = hz / 10;
> schedcpu(_to);
>
> #ifndef SMALL_KERNEL
> Index: kern/kern_sched.c
> ===
> RCS file: /cvs/src/sys/kern/kern_sched.c,v
> retrieving revision 1.84
> diff -u -p -r1.84 kern_sched.c
> --- kern/kern_sched.c 5 Aug 2023 20:07:55 - 1.84
> +++ kern/kern_sched.c 5 Aug 2023 22:15:25 -
> @@ -102,6 +102,12 @@ sched_init_cpu(struct cpu_info *ci)
> if (spc->spc_profclock == NULL)
> panic("%s: clockintr_establish profclock", __func__);
> }
> + if (spc->spc_roundrobin == NULL) {
> + spc->spc_roundrobin = clockintr_establish(>ci_queue,
> + roundrobin);
> + if (spc->spc_roundrobin == NULL)
> + panic("%s: clockintr_establish roundrobin", __func__);
> + }
>
Re: installer: always create new softraid volume
On Fri, Aug 04, 2023 at 11:45:00AM +, Klemens Nanni wrote:
> If the root disk contains a valid CRYPTO volume, bioctl(8) by default
> unlocks that instead of creating a new one.
>
> Use `-C force' to prevent reuse of old volumes, which happens if you, e.g.
> restart an encrypted installation past this point or install onto an old
> disk without wiping it first:
>
> Encrypt the root disk with a passphrase? [no] y
>
> Configuring the crypto chunk sd0...
>
> Disk: sd0 geometry: 652/255/63 [10485760 Sectors]
> Offset: 0 Signature: 0xAA55
> Starting Ending LBA Info:
>#: id C H S - C H S [ start:size ]
>
> ---
>0: 00 0 0 0 - 0 0 0 [ 0: 0 ]
> Unused
>1: 00 0 0 0 - 0 0 0 [ 0: 0 ]
> Unused
>2: 00 0 0 0 - 0 0 0 [ 0: 0 ]
> Unused
> *3: A6 0 1 2 -652 180 40 [ 64:10485696 ]
> OpenBSD
> Use (W)hole disk MBR, whole disk (G)PT, (O)penBSD area or (E)dit?
> [OpenBSD]
> Passphrase:
> sd1 at scsibus1 targ 1 lun 0:
>
> There bioctl once prompts for the old existing
> Passphrase:
> instead of
> New passphrase:
> Re-type passphrase:
>
>
> Feedback? Objection? OK?
Anyone takers? One likes it, otherwise no replies.
I'm inclined to go ahead with this one soon unless there are objections
is it fixes a few cases users stumbled over.
Index: install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1252
diff -u -p -r1.1252 install.sub
--- install.sub 2 Aug 2023 08:51:16 - 1.1252
+++ install.sub 10 Aug 2023 07:54:36 -
@@ -3097,7 +3097,7 @@ encrypt_root() {
md_prep_fdisk $_chunk
echo 'RAID *' | disklabel -w -A -T- $_chunk
- until bioctl -c C -l ${_chunk}a softraid0 >/dev/null; do
+ until bioctl -Cforce -cC -l${_chunk}a softraid0 >/dev/null; do
# Most likely botched passphrases, silently retry twice.
((++_tries < 3)) || exit
done
