Re: pause.3: misc cleanup
Hi Scott, Scott Cheloha wrote on Sat, Dec 10, 2022 at 09:25:48AM -0600: > Sure, I will keep the ERRORS section. > I think the current phrasing in ERRORS is odd, though. > "may set the global variable..." is what we normally say here, but it > isn't a "may" in this case, it's an "always". I wouldn't go as far as saying "'may set the global variable...' is what we normally say. It occurs in a very small number of section 3 manuals. It might occasionally make sense in section 3 because some library functions might fail both due to syscall failures and for other reasons not related to syscalls, in which case they may sometimes set errno and sometimes not - even though some library functions also set errno themselves, not relying on failing syscalls to do that. Even in section 3, i see less than ten instances: - __thrsleep(2) - clock_settime(2) - setlogin(2) - setrlimit(2) - read(2) - recv(2) - send(2) - truncate(2) - write(2) And all these only occur after the usual wording "will fail if". So it doesn't mean that any syscall can fail without setting errno; this wording merely indicates that for specific functions, additional errno values may occur instead of the ones listed right above. That said, if something is guaranteed to set errno, i clearly prefer saying so. Let's try to avoid needless ambiguity. > Check if my tweak in the attached patch feels right. I dislike the wording "return an error" for setting errno, even in the existing pages, in particular for a function that has an "int" return value. I see a slight risk of confusing people. Also, for this function, setting errno(2) does not really indicate any error, or does it? The function is a bit unusual in so far as it always returns the same value, never fails, always sets errno, and always to the same value, so the usual wordings don't fit too well. I would prefer something like this: .Fn pause blocks the calling thread until it receives an unmasked signal and then returns. .Sh RETURN VALUES .Fn pause always returns \-1. .Sh ERRORS .Fn pause always sets .Xr errno 2 to the following value: .Bl -tag -width Er .It Bq Er EINTR The call was interrupted by a signal. .El Feel free to use part or all of my ideas, or commit your patch commit as-is. OK schwarze@ either way. Yours, Ingo > Index: pause.3 > === > RCS file: /cvs/src/lib/libc/gen/pause.3,v > retrieving revision 1.16 > diff -u -p -r1.16 pause.3 > --- pause.3 9 Nov 2022 06:48:29 - 1.16 > +++ pause.3 10 Dec 2022 15:23:47 - > @@ -32,7 +32,7 @@ > .Os > .Sh NAME > .Nm pause > -.Nd stop until signal > +.Nd wait for a signal > .Sh SYNOPSIS > .In unistd.h > .Ft int > @@ -40,40 +40,31 @@ > .Sh DESCRIPTION > .Bf -symbolic > .Fn pause > -is made obsolete by > +is obsoleted by > .Xr sigsuspend 2 . > .Ef > .Pp > -The > .Fn pause > -function forces a process to pause until a signal is received from either the > -.Xr kill 2 > -function or an interval timer > -(see > -.Xr setitimer 2 ) . > -.Pp > -Upon termination of a signal handler started during a > -.Fn pause , > -the > -.Fn pause > -call will return. > +blocks the calling thread until it receives an unmasked signal. > .Sh RETURN VALUES > -Always returns \-1. > -.Sh ERRORS > -The > +On receipt of a signal, > .Fn pause > -function may set the global variable > +returns \-1 and the global variable > .Va errno > -to the following error: > +is set to indicate the error. > +.Sh ERRORS > +.Fn pause > +always returns the following error: > .Bl -tag -width Er > .It Bq Er EINTR > -The call was interrupted. > +The call was interrupted by a signal. > .El > .Sh SEE ALSO > .Xr kill 2 , > -.Xr select 2 , > .Xr setitimer 2 , > -.Xr sigsuspend 2 > +.Xr sigprocmask 2 , > +.Xr sigsuspend 2 , > +.Xr signal 3 > .Sh HISTORY > A > .Fn pause
Re: Remove unused experimental ICMP6 redirect low water bits
ok mvs@
> On 11 Dec 2022, at 01:46, Klemens Nanni wrote:
>
> Dead since introduction in 2001 with icmp6.c r1.31:
>implement upper limit to icmp6 redirects (experimental, turned off)
>negative value to {mtudisc,redirect}_{hi,lo}wat will turn off the
> limitation.
>sync with kame.
>
> icmp6_redirect_lowat was always -1 and never hit the empty conditional.
>
> icmp6_redirect_hiwat never existed.
>
> icmp6_mtudisc_{hi,lo}wat are exposed as net.inet6.icmp6.mtudisc_{hi,lo}wat
> sysctl(2)s, so don't touch those for now.
>
> Feedback? OK?
>
> diff --git a/sys/netinet6/icmp6.c b/sys/netinet6/icmp6.c
> index 94612a560c3..7a64861d15f 100644
> --- a/sys/netinet6/icmp6.c
> +++ b/sys/netinet6/icmp6.c
> @@ -129,9 +129,6 @@ static int icmp6_mtudisc_lowat = 256;
> */
> struct rttimer_queue icmp6_redirect_timeout_q;
>
> -/* XXX experimental, turned off */
> -static int icmp6_redirect_lowat = -1;
> -
> void icmp6_errcount(int, int);
> int icmp6_ratelimit(const struct in6_addr *, const int, const int);
> const char *icmp6_redirect_diag(struct in6_addr *, struct in6_addr *,
> @@ -1386,12 +1383,6 @@ icmp6_redirect_input(struct mbuf *m, int off)
> rtcount = rt_timer_queue_count(_redirect_timeout_q);
> if (0 <= ip6_maxdynroutes && rtcount >= ip6_maxdynroutes)
> goto freeit;
> - else if (0 <= icmp6_redirect_lowat &&
> - rtcount > icmp6_redirect_lowat) {
> - /*
> - * XXX nuke a victim, install the new one.
> - */
> - }
>
> bzero(, sizeof(sdst));
> bzero(, sizeof(sgw));
>
vmd(8): create a proper e820 bios memory map
tech@,
The below diff tweaks how vmd and vmm define memory ranges (adding a
"type" attribute) so we can properly build an e820 memory map to hand to
things like SeaBIOS or the OpenBSD ramdisk kernel (when direct booting
bsd.rd).
Why do it? We've been carrying a few patches to SeaBIOS in the ports
tree to hack around how vmd articulates some memory range details. By
finally implementing a proper bios memory map table we can drop some of
those patches. (Diff to ports@ coming shortly.)
Bonus is it cleans up how we were hacking a bios memory map for direct
booting ramdisk kernels.
Note: the below diff *will* work with the current SeaBIOS
(vmm-firmware), so you do *not* need to build the port.
You will, however, need to:
- build, install, & reboot into a new kernel
- make sure you update /usr/include/amd64/vmmvar.h with a copy of
symlink to sys/arch/amd64/include/vmmvar.h
- rebuild & install vmctl
- rebuild & install vmd
This should *not* result in any behavioral changes of current vmd
guests. If you notice any, especially guests failing to start, please
rebuild a kernel with VMM_DEBUG to help diagnose the regression.
-dv
diff refs/heads/master refs/heads/vmd-e820
commit - a96642fb40af450c6576e205fab247cdbce0b5ed
commit + f3cb01998127d200e95ff9984a7503eb16c2a8d8
blob - 3f7e0ce405ae3c6b0b4a787de341839886f97436
blob + f2a464217838d3f0a50e4131b5b074b315e490fb
--- sys/arch/amd64/amd64/vmm.c
+++ sys/arch/amd64/amd64/vmm.c
@@ -1643,21 +1643,27 @@ vm_create_check_mem_ranges(struct vm_create_params *vc
const paddr_t maxgpa = VMM_MAX_VM_MEM_SIZE;
if (vcp->vcp_nmemranges == 0 ||
- vcp->vcp_nmemranges > VMM_MAX_MEM_RANGES)
+ vcp->vcp_nmemranges > VMM_MAX_MEM_RANGES) {
+ DPRINTF("invalid number of guest memory ranges\n");
return (0);
+ }
for (i = 0; i < vcp->vcp_nmemranges; i++) {
vmr = >vcp_memranges[i];
/* Only page-aligned addresses and sizes are permitted */
if ((vmr->vmr_gpa & PAGE_MASK) || (vmr->vmr_va & PAGE_MASK) ||
- (vmr->vmr_size & PAGE_MASK) || vmr->vmr_size == 0)
+ (vmr->vmr_size & PAGE_MASK) || vmr->vmr_size == 0) {
+ DPRINTF("memory range %zu is not page aligned\n", i);
return (0);
+ }
/* Make sure that VMM_MAX_VM_MEM_SIZE is not exceeded */
if (vmr->vmr_gpa >= maxgpa ||
- vmr->vmr_size > maxgpa - vmr->vmr_gpa)
+ vmr->vmr_size > maxgpa - vmr->vmr_gpa) {
+ DPRINTF("exceeded max memory size\n");
return (0);
+ }
/*
* Make sure that all virtual addresses are within the address
@@ -1667,39 +1673,55 @@ vm_create_check_mem_ranges(struct vm_create_params *vc
*/
if (vmr->vmr_va < VM_MIN_ADDRESS ||
vmr->vmr_va >= VM_MAXUSER_ADDRESS ||
- vmr->vmr_size >= VM_MAXUSER_ADDRESS - vmr->vmr_va)
+ vmr->vmr_size >= VM_MAXUSER_ADDRESS - vmr->vmr_va) {
+ DPRINTF("guest va not within range or wraps\n");
return (0);
+ }
/*
* Specifying ranges within the PCI MMIO space is forbidden.
* Disallow ranges that start inside the MMIO space:
* [VMM_PCI_MMIO_BAR_BASE .. VMM_PCI_MMIO_BAR_END]
*/
- if (vmr->vmr_gpa >= VMM_PCI_MMIO_BAR_BASE &&
- vmr->vmr_gpa <= VMM_PCI_MMIO_BAR_END)
+ if (vmr->vmr_type == VM_MEM_RAM &&
+ vmr->vmr_gpa >= VMM_PCI_MMIO_BAR_BASE &&
+ vmr->vmr_gpa <= VMM_PCI_MMIO_BAR_END) {
+ DPRINTF("guest RAM range %zu cannot being in mmio range"
+ " (gpa=0x%lx)\n", i, vmr->vmr_gpa);
return (0);
+ }
/*
* ... and disallow ranges that end inside the MMIO space:
* (VMM_PCI_MMIO_BAR_BASE .. VMM_PCI_MMIO_BAR_END]
*/
- if (vmr->vmr_gpa + vmr->vmr_size > VMM_PCI_MMIO_BAR_BASE &&
- vmr->vmr_gpa + vmr->vmr_size <= VMM_PCI_MMIO_BAR_END)
+ if (vmr->vmr_type == VM_MEM_RAM &&
+ vmr->vmr_gpa + vmr->vmr_size > VMM_PCI_MMIO_BAR_BASE &&
+ vmr->vmr_gpa + vmr->vmr_size <= VMM_PCI_MMIO_BAR_END) {
+ DPRINTF("guest RAM range %zu cannot end in mmio range"
+ " (gpa=0x%lx, sz=0x%lx)\n", i, vmr->vmr_gpa,
+ vmr->vmr_size);
return (0);
+ }
/*
* Make sure that guest physical memory ranges do not overlap
* and that they are ascending.
*/
-
Remove unused experimental ICMP6 redirect low water bits
Dead since introduction in 2001 with icmp6.c r1.31:
implement upper limit to icmp6 redirects (experimental, turned off)
negative value to {mtudisc,redirect}_{hi,lo}wat will turn off the
limitation.
sync with kame.
icmp6_redirect_lowat was always -1 and never hit the empty conditional.
icmp6_redirect_hiwat never existed.
icmp6_mtudisc_{hi,lo}wat are exposed as net.inet6.icmp6.mtudisc_{hi,lo}wat
sysctl(2)s, so don't touch those for now.
Feedback? OK?
diff --git a/sys/netinet6/icmp6.c b/sys/netinet6/icmp6.c
index 94612a560c3..7a64861d15f 100644
--- a/sys/netinet6/icmp6.c
+++ b/sys/netinet6/icmp6.c
@@ -129,9 +129,6 @@ static int icmp6_mtudisc_lowat = 256;
*/
struct rttimer_queue icmp6_redirect_timeout_q;
-/* XXX experimental, turned off */
-static int icmp6_redirect_lowat = -1;
-
void icmp6_errcount(int, int);
inticmp6_ratelimit(const struct in6_addr *, const int, const int);
const char *icmp6_redirect_diag(struct in6_addr *, struct in6_addr *,
@@ -1386,12 +1383,6 @@ icmp6_redirect_input(struct mbuf *m, int off)
rtcount = rt_timer_queue_count(_redirect_timeout_q);
if (0 <= ip6_maxdynroutes && rtcount >= ip6_maxdynroutes)
goto freeit;
- else if (0 <= icmp6_redirect_lowat &&
- rtcount > icmp6_redirect_lowat) {
- /*
-* XXX nuke a victim, install the new one.
-*/
- }
bzero(, sizeof(sdst));
bzero(, sizeof(sgw));
Re: nd6: Replace net lock with DAD specific adress list lock
On Sat, Dec 10, 2022 at 11:32:08PM +0300, Vitaliy Makkoveev wrote: > The netlock removal within nd6_dad_timer() is wrong. It protects > `ifa’, `ia6’ and ip6 output path. Yes... Sorry for noise, I'll come back with a less stupid diff.
Don't bzero() dp->dad_timer_ch within nd6_dad_start()
It's not required because `dp' was just allocated with M_ZERO flag. Index: sys/netinet6/nd6_nbr.c === RCS file: /cvs/src/sys/netinet6/nd6_nbr.c,v retrieving revision 1.138 diff -u -p -r1.138 nd6_nbr.c --- sys/netinet6/nd6_nbr.c 2 Dec 2022 15:35:35 - 1.138 +++ sys/netinet6/nd6_nbr.c 10 Dec 2022 20:41:22 - @@ -1101,7 +1101,6 @@ nd6_dad_start(struct ifaddr *ifa) ifa->ifa_ifp ? ifa->ifa_ifp->if_xname : "???"); return; } - bzero(>dad_timer_ch, sizeof(dp->dad_timer_ch)); TAILQ_INSERT_TAIL(, dp, dad_list); ip6_dad_pending++;
Re: nd6: Replace net lock with DAD specific adress list lock
The netlock removal within nd6_dad_timer() is wrong. It protects
`ifa’, `ia6’ and ip6 output path.
> On 10 Dec 2022, at 15:16, Klemens Nanni wrote:
>
> nd6_nbr.c's static list of IPs to perform Duplicate Address Detection on
> is protected by the too broad exclusive net lock.
>
> Switch nd6_dad_timer() and helpers to a dedicated dad_lk rwlock(9) to
> take pressure of the net lock.
>
> A mutex(9) must not be used as this code path may sleep:
> nd6_dad_timer() -> rtm_addr() -> route_input() -> solock().
>
> Document all struct dadq's fields and their protection accordingly.
>
> This makes DAD no longer grab the net lock itself; existing assertions
> must remain, though, and have been annotated to ease review.
>
> Static helper functions in this file accessing the static list do assert
> that the new lock is taken for now clarify the intention and ease
> testing/review.
>
> I'd like to commit without net lock assert comment and new lock assert.
>
> WITNESS, regress, daily usage and manual testing are fine for me.
>
> More tests?
> Feedback? OK?
>
> diff --git a/sys/netinet6/nd6_nbr.c b/sys/netinet6/nd6_nbr.c
> index acced08b3db..fedd3fdb42d 100644
> --- a/sys/netinet6/nd6_nbr.c
> +++ b/sys/netinet6/nd6_nbr.c
> @@ -42,6 +42,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -62,17 +63,25 @@
> #include
> #endif
>
> +/*
> + * Locks used to protect struct members in this file:
> + * D DAD lock
> + */
> +
> +static struct rwlock dad_lk =
> +RWLOCK_INITIALIZER("dad");
> +
> static TAILQ_HEAD(, dadq) dadq =
> TAILQ_HEAD_INITIALIZER(dadq); /* list of addresses to run DAD on */
> struct dadq {
> - TAILQ_ENTRY(dadq) dad_list;
> - struct ifaddr *dad_ifa;
> - int dad_count; /* max NS to send */
> - int dad_ns_tcount; /* # of trials to send NS */
> - int dad_ns_ocount; /* NS sent so far */
> - int dad_ns_icount;
> - int dad_na_icount;
> - struct timeout dad_timer_ch;
> + TAILQ_ENTRY(dadq) dad_list; /* [D] all struct dadqs are chained */
> + struct ifaddr *dad_ifa; /* [D] IP address to perform DAD on */
> + int dad_count; /* [D] max NS to send */
> + int dad_ns_tcount; /* [D] # of trials to send NS */
> + int dad_ns_ocount; /* [D] NS sent so far */
> + int dad_ns_icount; /* [D] NS received so far */
> + int dad_na_icount; /* [D] NA received so far */
> + struct timeout dad_timer_ch;/* [D] */
> };
>
> struct dadq *nd6_dad_find(struct ifaddr *);
> @@ -575,6 +584,8 @@ nd6_na_input(struct mbuf *m, int off, int icmp6len)
> struct nd_opts ndopts;
> char addr[INET6_ADDRSTRLEN], addr0[INET6_ADDRSTRLEN];
>
> + /* only called from icmp6_input() aka. .pr_input which is called from
> + * ip_deliver() with NET_ASSERT_LOCKED_EXCLUSIVE() */
> NET_ASSERT_LOCKED();
>
> ifp = if_get(m->m_pkthdr.ph_ifidx);
> @@ -652,6 +663,7 @@ nd6_na_input(struct mbuf *m, int off, int icmp6len)
> if (ifa && (ifatoia6(ifa)->ia6_flags & IN6_IFF_TENTATIVE)) {
> struct dadq *dp;
>
> + rw_enter_write(_lk);
> dp = nd6_dad_find(ifa);
> if (dp) {
> dp->dad_na_icount++;
> @@ -659,6 +671,7 @@ nd6_na_input(struct mbuf *m, int off, int icmp6len)
> /* remove the address. */
> nd6_dad_duplicated(dp);
> }
> + rw_exit_write(_lk);
> goto freeit;
> }
>
> @@ -1040,6 +1053,8 @@ nd6_dad_find(struct ifaddr *ifa)
> {
> struct dadq *dp;
>
> + rw_assert_anylock(_lk);
> +
> TAILQ_FOREACH(dp, , dad_list) {
> if (dp->dad_ifa == ifa)
> return dp;
> @@ -1050,6 +1065,8 @@ nd6_dad_find(struct ifaddr *ifa)
> void
> nd6_dad_starttimer(struct dadq *dp)
> {
> + rw_assert_wrlock(_lk);
> +
> timeout_set_proc(>dad_timer_ch, nd6_dad_timer, dp->dad_ifa);
> timeout_add_msec(>dad_timer_ch, RETRANS_TIMER);
> }
> @@ -1057,6 +1074,8 @@ nd6_dad_starttimer(struct dadq *dp)
> void
> nd6_dad_stoptimer(struct dadq *dp)
> {
> + rw_assert_wrlock(_lk);
> +
> timeout_del(>dad_timer_ch);
> }
>
> @@ -1070,6 +1089,9 @@ nd6_dad_start(struct ifaddr *ifa)
> struct dadq *dp;
> char addr[INET6_ADDRSTRLEN];
>
> + /* only called from
> + * - in6_ifattach_linklocal() with NET_ASSERT_LOCKED()
> + * - in6_ioctl_change_ifaddr() with NET_LOCK() */
> NET_ASSERT_LOCKED();
>
> /*
> @@ -1088,8 +1110,9 @@ nd6_dad_start(struct ifaddr *ifa)
> }
>
> /* DAD already in progress */
> + rw_enter_write(_lk);
> if (nd6_dad_find(ifa) != NULL)
> - return;
> + goto out;
>
> dp = malloc(sizeof(*dp), M_IP6NDP, M_NOWAIT | M_ZERO);
> if (dp == NULL) {
> @@ -1097,7 +1120,7 @@ nd6_dad_start(struct ifaddr *ifa)
>
Re: amptimer(4): switch to clockintr
On Fri, Dec 09 2022, Scott Cheloha wrote: > Next up for the armv7 clockintr switch: amptimer(4). > > - Remove amptimer-specific clock interrupt scheduling bits and > randomized statclock bits. > - Remove debug evcounts. The interrupt's evcount counts all clock > interrupts from now on. > - Remove unused USE_GTIMER_CMP pieces. > - Wire up amptimer_intrclock. > > jca: You have a cubox with this device. Does this compile? > If so, does it boot? Builds but the machine didn't come up after reboot. I'm not at home right now so it'll have to wait a day or two, unless someone else can test on their own machine. -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: sparc64: don't install %TICK timecounter on UltraSPARC IIe
On Sat, Dec 10, 2022 at 10:24:52AM -0700, Theo de Raadt wrote:
> Scott Cheloha wrote:
>
> > On Sat, Dec 10, 2022 at 09:33:55AM -0700, Theo de Raadt wrote:
> > > Mark Kettenis wrote:
> > >
> > > > So really the question is whether we should register the "tick"
> > > > timecounter if we also provide the "stick" or "sys_tick" timecounter.
> > > >
> > > > Personaly I think the code we currently have is fine. If you make the
> > > > sconscious decision to use "tick" on a system that provides "stick" or
> > > > "sys_tick" you'd better make the conscious decision not to use the CPU
> > > > frequency scaling stuff.
> > >
> > > Right. Is it really any different than choosing a clock with known or
> > > unknown
> > > quirks on an x86 machine:
> > >
> > > kern.timecounter.choice=i8254(0) tsc(2000) acpihpet0(1000)
> > > acpitimer0(1000)
> >
> > I think this situation is different from the one on i386/amd64.
> >
> > When we offer a timecounter to the end user, we're saying (1) this
> > clock is monotonic, and (2) this clock counts at a fixed frequency.
> > The quality number then gives you a hint about which clock is best.
> >
> > So, yes, some of those x86 clocks are slow compared with the TSC, but
> > they still count time correctly.
>
> Really. Are you willing to put money on that? That on x86, none of the
> clocks are variant anymore, especially after a resume? How much money.
> Anyone else want to put money on it? We'd be talking a 1 year long bet to
> discover a mistake...
There may be other problems remaining on x86, but that isn't relevant
to this thread.
Getting back to sparc64:
What I'm hearing from kettenis@ is that %tick might have a variable
frequency on *all* SPARC systems that have the %sys_tick register.
So, UltraSPARC IIe and everything after that.
If this is true, then %tick is not a real timecounter on those systems
and we should not offer it as a choice to the end-user.
We have a suitable timecounter on those systems -- %sys_tick (ASR24)
or the PCI-based %stick on the Hummingbird. You only need one working
timecounter to boot. There is no reason to *also* offer a %tick-based
timecounter if we know out of the gate that it doesn't have a constant
frequency.
Index: clock.c
===
RCS file: /cvs/src/sys/arch/sparc64/sparc64/clock.c,v
retrieving revision 1.72
diff -u -p -r1.72 clock.c
--- clock.c 10 Nov 2022 07:08:01 - 1.72
+++ clock.c 10 Dec 2022 17:15:55 -
@@ -567,21 +567,28 @@ cpu_initclocks(void)
/* Default to 200MHz clock X */
cpu_clockrate = 2;
- tick_timecounter.tc_frequency = cpu_clockrate;
- tc_init(_timecounter);
-
- /*
-* UltraSPARC IIe processors do have a STICK register, but it
-* lives on the PCI host bridge and isn't accessible through
-* ASR24.
-*/
if (CPU_ISSUN4U || CPU_ISSUN4US)
impl = (getver() & VER_IMPL) >> VER_IMPL_SHIFT;
+ /*
+* The %tick frequency is variable on systems with a %sys_tick
+* register, so if stick-frequency is non-zero we don't install
+* tick_timecounter.
+*
+* UltraSPARC IIe ("Hummingbird") processors have a %sys_tick
+* register, but it lives on the PCI host bridge and isn't
+* accessible through ASR24. psycho(4) provides a special
+* timecounter for those systems.
+*/
sys_tick_rate = getpropint(findroot(), "stick-frequency", 0);
- if (sys_tick_rate > 0 && impl != IMPL_HUMMINGBIRD) {
- sys_tick_timecounter.tc_frequency = sys_tick_rate;
- tc_init(_tick_timecounter);
+ if (sys_tick_rate > 0) {
+ if (impl != IMPL_HUMMINGBIRD) {
+ sys_tick_timecounter.tc_frequency = sys_tick_rate;
+ tc_init(_tick_timecounter);
+ }
+ } else {
+ tick_timecounter.tc_frequency = cpu_clockrate;
+ tc_init(_timecounter);
}
/*
Re: sparc64: don't install %TICK timecounter on UltraSPARC IIe
Scott Cheloha wrote: > On Sat, Dec 10, 2022 at 09:33:55AM -0700, Theo de Raadt wrote: > > Mark Kettenis wrote: > > > > > So really the question is whether we should register the "tick" > > > timecounter if we also provide the "stick" or "sys_tick" timecounter. > > > > > > Personaly I think the code we currently have is fine. If you make the > > > sconscious decision to use "tick" on a system that provides "stick" or > > > "sys_tick" you'd better make the conscious decision not to use the CPU > > > frequency scaling stuff. > > > > Right. Is it really any different than choosing a clock with known or > > unknown > > quirks on an x86 machine: > > > > kern.timecounter.choice=i8254(0) tsc(2000) acpihpet0(1000) acpitimer0(1000) > > I think this situation is different from the one on i386/amd64. > > When we offer a timecounter to the end user, we're saying (1) this > clock is monotonic, and (2) this clock counts at a fixed frequency. > The quality number then gives you a hint about which clock is best. > > So, yes, some of those x86 clocks are slow compared with the TSC, but > they still count time correctly. Really. Are you willing to put money on that? That on x86, none of the clocks are variant anymore, especially after a resume? How much money. Anyone else want to put money on it? We'd be talking a 1 year long bet to discover a mistake...
Re: sparc64: don't install %TICK timecounter on UltraSPARC IIe
On Sat, Dec 10, 2022 at 09:33:55AM -0700, Theo de Raadt wrote:
> Mark Kettenis wrote:
>
> > So really the question is whether we should register the "tick"
> > timecounter if we also provide the "stick" or "sys_tick" timecounter.
> >
> > Personaly I think the code we currently have is fine. If you make the
> > sconscious decision to use "tick" on a system that provides "stick" or
> > "sys_tick" you'd better make the conscious decision not to use the CPU
> > frequency scaling stuff.
>
> Right. Is it really any different than choosing a clock with known or unknown
> quirks on an x86 machine:
>
> kern.timecounter.choice=i8254(0) tsc(2000) acpihpet0(1000) acpitimer0(1000)
I think this situation is different from the one on i386/amd64.
When we offer a timecounter to the end user, we're saying (1) this
clock is monotonic, and (2) this clock counts at a fixed frequency.
The quality number then gives you a hint about which clock is best.
So, yes, some of those x86 clocks are slow compared with the TSC, but
they still count time correctly.
What I'm hearing from kettenis@ is that %tick might have a variable
frequency on SPARC systems that have %sys_tick. If this is true, then
%tick is not a real timecounter on those systems and we should not
offer it as a choice to the end user.
We have a suitable timecounter on those systems, %sys_tick or the
PCI-based %stick... why offer a second clock we know is unsuitable?
It just leaves room for end-user error. It's a footgun.
Index: clock.c
===
RCS file: /cvs/src/sys/arch/sparc64/sparc64/clock.c,v
retrieving revision 1.72
diff -u -p -r1.72 clock.c
--- clock.c 10 Nov 2022 07:08:01 - 1.72
+++ clock.c 10 Dec 2022 17:15:55 -
@@ -567,21 +567,28 @@ cpu_initclocks(void)
/* Default to 200MHz clock X */
cpu_clockrate = 2;
- tick_timecounter.tc_frequency = cpu_clockrate;
- tc_init(_timecounter);
-
- /*
-* UltraSPARC IIe processors do have a STICK register, but it
-* lives on the PCI host bridge and isn't accessible through
-* ASR24.
-*/
if (CPU_ISSUN4U || CPU_ISSUN4US)
impl = (getver() & VER_IMPL) >> VER_IMPL_SHIFT;
+ /*
+* The %tick frequency is variable on systems with a %sys_tick
+* register, so if stick-frequency is non-zero we don't install
+* tick_timecounter.
+*
+* UltraSPARC IIe ("Hummingbird") processors have a %sys_tick
+* register, but it lives on the PCI host bridge and isn't
+* accessible through ASR24. psycho(4) provides a special
+* timecounter for those systems.
+*/
sys_tick_rate = getpropint(findroot(), "stick-frequency", 0);
- if (sys_tick_rate > 0 && impl != IMPL_HUMMINGBIRD) {
- sys_tick_timecounter.tc_frequency = sys_tick_rate;
- tc_init(_tick_timecounter);
+ if (sys_tick_rate > 0) {
+ if (impl != IMPL_HUMMINGBIRD) {
+ sys_tick_timecounter.tc_frequency = sys_tick_rate;
+ tc_init(_tick_timecounter);
+ }
+ } else {
+ tick_timecounter.tc_frequency = cpu_clockrate;
+ tc_init(_timecounter);
}
/*
Re: sparc64: don't install %TICK timecounter on UltraSPARC IIe
Mark Kettenis wrote: > So really the question is whether we should register the "tick" > timecounter if we also provide the "stick" or "sys_tick" timecounter. > > Personaly I think the code we currently have is fine. If you make the > sconscious decision to use "tick" on a system that provides "stick" or > "sys_tick" you'd better make the conscious decision not to use the CPU > frequency scaling stuff. Right. Is it really any different than choosing a clock with known or unknown quirks on an x86 machine: kern.timecounter.choice=i8254(0) tsc(2000) acpihpet0(1000) acpitimer0(1000)
Re: sparc64: don't install %TICK timecounter on UltraSPARC IIe
> Date: Sat, 10 Dec 2022 09:43:32 -0600
> From: Scott Cheloha
>
> On Sat, Dec 10, 2022 at 03:14:37PM +0100, Mark Kettenis wrote:
> > > Date: Fri, 9 Dec 2022 16:27:59 -0600
> > > From: Scott Cheloha
> > >
> > > The UltraSPARC IIe's %TICK register has a variable frequency. See
> > > section 2.3 in this document here:
> > >
> > > https://web.archive.org/web/20221028065731/https://www.oracle.com/technetwork/server-storage/sun-sparc-enterprise/documentation/ultrasparc-iie-2516664.pdf
> > >
> > > Timecounters need to have a constant frequency, so we should not
> > > install tick_timecounter if the implementation is an UltraSPARC IIe
> > > ("Hummingbird").
> > >
> > > As far as I know this issue is unique to the IIe. I can't find any
> > > reference to a varying %TICK frequency in the documentation for the
> > > IIi or the UltraSPARC III.
> > >
> > > miod@ confirmed that the problem is real.
> > >
> > > ok?
> >
> > I don't think so.
> >
> > IIRC, UltraSPARC IIi and UltraSPARC IIIi also have a potentially
> > varying %tick frequency.
>
> Where would this be written down? And how do we know that later
> revisions have a fixed %tick frequency?
>
> > Those chips implement %sys_tick though and
> > since we give the associated timecounter a higher quality, it wins.
>
> Gotcha.
Not sure if it is documented anywhere. But my interpretation has
always been that %sys_tick was introduced when Sun started building
SPARCv9 CPUs that had clock frequencies that could be changed. The
UltraSPARC-IIe processor was part of that transition. A timecounter
that didn't scale with the CPU frequency was introduced in the
hardware, but the %sys_tick ASR wasn't added to the instruction set
yet.
Anyway, I think one has to assume that when %sys_tick exists, %tick
may not run at a constant frequency.
> > With your diff UltraSPARC IIe will end up without a timecounter. That
> > would bad isn't it?
>
> We have a stick_timecounter in dev/psycho.c that uses stick(). Is
> that sufficient?
Ah, wait, that's the one that gets used. I probably wrote that code.
I'm getting old.
So really the question is whether we should register the "tick"
timecounter if we also provide the "stick" or "sys_tick" timecounter.
Personaly I think the code we currently have is fine. If you make the
sconscious decision to use "tick" on a system that provides "stick" or
"sys_tick" you'd better make the conscious decision not to use the CPU
frequency scaling stuff.
Re: pause.3: misc cleanup
Updated patch looks good to me. - todd
iostat(8): implement periodic display with setitimer(2)
As with e.g. kstat(1), implementing a periodic display with
nanosleep(2) causes the display interval to drift. Doing it with
setitimer(2) prevents drift.
ok?
Index: iostat.c
===
RCS file: /cvs/src/usr.sbin/iostat/iostat.c,v
retrieving revision 1.45
diff -u -p -r1.45 iostat.c
--- iostat.c4 Dec 2022 23:50:51 - 1.45
+++ iostat.c10 Dec 2022 14:00:50 -
@@ -68,10 +68,12 @@
#include
#include
+#include
#include
#include
#include
#include
+#include
#include
#include
@@ -85,7 +87,8 @@ extern intdk_ndrive;
kvm_t *kd;
char *nlistf, *memf;
-inthz, reps, interval;
+inthz, reps;
+time_t interval;
static int todo = 0;
volatile sig_atomic_t wantheader;
@@ -100,6 +103,7 @@ volatile sig_atomic_t wantheader;
static void cpustats(void);
static void disk_stats(double);
static void disk_stats2(double);
+static void sigalarm(int);
static void sigheader(int);
static void header(void);
static void usage(void);
@@ -113,9 +117,10 @@ int dkinit(int);
int
main(int argc, char *argv[])
{
+ struct itimerval itv;
const char *errstr;
+ sigset_t empty;
int ch, hdrcnt;
- struct timespec ts;
while ((ch = getopt(argc, argv, "Cc:dDIM:N:Tw:")) != -1)
switch(ch) {
@@ -146,7 +151,7 @@ main(int argc, char *argv[])
todo |= SHOW_TTY;
break;
case 'w':
- interval = strtonum(optarg, 1, 1, );
+ interval = strtonum(optarg, 1, UINT_MAX, );
if (errstr)
errx(1, "wait is %s", errstr);
break;
@@ -169,12 +174,20 @@ main(int argc, char *argv[])
dkreadstats();
selectdrives(argv);
- ts.tv_sec = interval;
- ts.tv_nsec = 0;
-
/* print a new header on sigcont */
signal(SIGCONT, sigheader);
+ if (interval != 0) {
+ if (signal(SIGALRM, sigalarm) == SIG_ERR)
+ err(1, "signal");
+ sigemptyset();
+ itv.it_value.tv_sec = interval;
+ itv.it_value.tv_usec = 0;
+ itv.it_interval = itv.it_value;
+ if (setitimer(ITIMER_REAL, , NULL) == -1)
+ err(1, "setitimer");
+ }
+
for (hdrcnt = 1;;) {
if (!--hdrcnt || wantheader) {
header();
@@ -188,7 +201,7 @@ main(int argc, char *argv[])
if (reps >= 0 && --reps <= 0)
break;
- nanosleep(, NULL);
+ sigsuspend();
dkreadstats();
if (last.dk_ndrive != cur.dk_ndrive)
wantheader = 1;
@@ -196,6 +209,11 @@ main(int argc, char *argv[])
exit(0);
}
+static void
+sigalarm(int signo)
+{
+}
+
/*ARGSUSED*/
static void
sigheader(int signo)
@@ -421,7 +439,7 @@ selectdrives(char *argv[])
errx(1, "invalid interval or drive name: %s", *argv);
}
if (*argv) {
- interval = strtonum(*argv, 1, 1, );
+ interval = strtonum(*argv, 1, UINT_MAX, );
if (errstr)
errx(1, "interval is %s", errstr);
if (*++argv) {
Re: sparc64: don't install %TICK timecounter on UltraSPARC IIe
On Sat, Dec 10, 2022 at 02:43:50PM +0100, Christian Weisgerber wrote: > Scott Cheloha: > > > The UltraSPARC IIe's %TICK register has a variable frequency. > > miod@ confirmed that the problem is real. > > Indeed, I remember clocking down the Blade 100 with apm(8). > > Somebody with access to the hardware might want to adapt the NetBSD > code that adds timecounter support for the STICK register. > Unfortunately it can't be read from userland. I have terrific news for you! Check out: sys/arch/sparc64/dev/psycho.c :)
Re: sparc64: don't install %TICK timecounter on UltraSPARC IIe
On Sat, Dec 10, 2022 at 03:14:37PM +0100, Mark Kettenis wrote:
> > Date: Fri, 9 Dec 2022 16:27:59 -0600
> > From: Scott Cheloha
> >
> > The UltraSPARC IIe's %TICK register has a variable frequency. See
> > section 2.3 in this document here:
> >
> > https://web.archive.org/web/20221028065731/https://www.oracle.com/technetwork/server-storage/sun-sparc-enterprise/documentation/ultrasparc-iie-2516664.pdf
> >
> > Timecounters need to have a constant frequency, so we should not
> > install tick_timecounter if the implementation is an UltraSPARC IIe
> > ("Hummingbird").
> >
> > As far as I know this issue is unique to the IIe. I can't find any
> > reference to a varying %TICK frequency in the documentation for the
> > IIi or the UltraSPARC III.
> >
> > miod@ confirmed that the problem is real.
> >
> > ok?
>
> I don't think so.
>
> IIRC, UltraSPARC IIi and UltraSPARC IIIi also have a potentially
> varying %tick frequency.
Where would this be written down? And how do we know that later
revisions have a fixed %tick frequency?
> Those chips implement %sys_tick though and
> since we give the associated timecounter a higher quality, it wins.
Gotcha.
> With your diff UltraSPARC IIe will end up without a timecounter. That
> would bad isn't it?
We have a stick_timecounter in dev/psycho.c that uses stick(). Is
that sufficient?
Re: pause.3: misc cleanup
Sorry for the delay. On Thu, Nov 10, 2022 at 08:19:51PM +0100, Ingo Schwarze wrote: > Hi Scott, > > thanks for repeatedly working on time-related library documentation. :-) > > Unless noted otherwise, i agree with Todd's comments. > > Todd C. Miller wrote on Wed, Nov 09, 2022 at 10:31:15AM -0700: > > On Wed, 09 Nov 2022 16:47:22 +, Scott Cheloha wrote: > > >> RETURN VALUES > >> > >> - Pull ERRORS into this section. No need to put the one error in a > >> .Bl/.El, we can just mention it inline. > > > OK > > I somewhat strongly object to this change. > > If there is content for a standard section, that section ought to > be present, even if it is very short. The point is that a manual > page should not only contain the expected information, but all that > information should also be in the expected places, such that by merely > looking at the section headers, you can already tell whether a given > standard section has content, without reading any of the text. > > For example, the RETURN VALUES section ought to be absent if and only > if all functions documented in the page are void. > > For example, the ERRORS section ought to be absent if and only if > none of the functions ever touch the errno. Admittedly, our pages > may not be perfect in that respect, some libc pages may be missing > an ERRORS section even though the functions in questions sometimes > set errno, but that's a (mild) defect. We certainly should not > delete an existing ERRORS section for a function that definitely does > set the errno. Alright, I see your point. > Also, in sections with a strongly conventional syntax (like ERRORS), > it is preferable to use the standard syntax even in corner cases. > In this case, yes, i do recommend a list with a single element. > Similarly, a utility program that can take exactly one option still > gets the standard DESCRIPTION sentence "The options are as follows:" > and then a one-element list. The advantage for readers is having the > familiar layout, and for developers the update becomes easier when > the second option is added; not that anything will ever be added to > pause(3), but consistency in style is still a good thing. Similarly, > in code, using getopt(3) is recommended even in a program that only > takes a single option. Sure, I will keep the ERRORS section. I think the current phrasing in ERRORS is odd, though. "may set the global variable..." is what we normally say here, but it isn't a "may" in this case, it's an "always". Check if my tweak in the attached patch feels right. > >> SEE ALSO > >> > >> - Nix select(2) and setitimer(2), they aren't directly relevant. > > > OK. The reason select(2) is there is probably because you can > > emulate pause() using select(). > > I don't feel strongly about this. > > Removing select(2) is probably OK because the reason explained > by Todd is mostly historical. In Perl, you still use select() > to sleep for less than one second, but that's irrelevant to C > programming. > > I'm less convinced about removing setitimer(2). To me, setitimer(2) > feels just as relevant as kill(2). Both cause signals to be delivered > (asap or later), which is quite relevant because that causes pause(3) > to return. That makes sense to me. We'll keep setitimer(2). > >> HISTORY > >> > >> - We still have sigpause(3) and sigblock(3) in userspace. Should > >> we .Xr them? They aren't systems calls anymore, but they were > >> at that time. Unsure what to do here. > > Polishing the HISTORY section should in no case hold up your other > work. > > > I think it is better to use .Fn for sigpause(3) and sigblock(3) > > rather than .Xr here. > > In this very special case, where sigpause(3) and sigblock(3) are > strongly deprecated, i agree. Pointing out their continued existence - > even below HISTORY - risks accidentally encouraging their use. > > > Note that in 4.3-Reno pause(3) was still implemented in terms of > > sigpause(3) and sigblock(3), it is just that those functions were > > themselves wrappers instead of system calls. Personally, I would > > drop the bit about 4.3-Reno since it is not really correct in my > > opinion. > > It could be argued either way: IMHO, a wrapper around a wrapper around > foo is still a wrapper around foo, so at least my version that's > currently in the tree can be argued to be correct. Admittedly, Scott's > version less so; pause(3) certainly wasn't reimplemented for Reno. > Then again, replacing the last phase (starting at "and around") > with the following would avoid the semantic disagreement: > > It has been using sigsuspend(2) and sigprocmask(2) since 4.3-BSD Reno. I am just going to leave the History section alone. -- How are we looking? Index: pause.3 === RCS file: /cvs/src/lib/libc/gen/pause.3,v retrieving revision 1.16 diff -u -p -r1.16 pause.3 --- pause.3 9 Nov 2022 06:48:29 - 1.16 +++ pause.3 10
Re: sparc64: don't install %TICK timecounter on UltraSPARC IIe
> Date: Fri, 9 Dec 2022 16:27:59 -0600
> From: Scott Cheloha
>
> The UltraSPARC IIe's %TICK register has a variable frequency. See
> section 2.3 in this document here:
>
> https://web.archive.org/web/20221028065731/https://www.oracle.com/technetwork/server-storage/sun-sparc-enterprise/documentation/ultrasparc-iie-2516664.pdf
>
> Timecounters need to have a constant frequency, so we should not
> install tick_timecounter if the implementation is an UltraSPARC IIe
> ("Hummingbird").
>
> As far as I know this issue is unique to the IIe. I can't find any
> reference to a varying %TICK frequency in the documentation for the
> IIi or the UltraSPARC III.
>
> miod@ confirmed that the problem is real.
>
> ok?
I don't think so.
IIRC, UltraSPARC IIi and UltraSPARC IIIi also have a potentially
varying %tick frequency. Those chips implement %sys_tick though and
since we give the associated timecounter a higher quality, it wins.
With your diff UltraSPARC IIe will end up without a timecounter. That
would bad isn't it?
So I think there are two options:
1. Implement a timecounter for Hummingbird's STICK logic. Shouldn't
be too difficult as we have a stick() function that returns the
counter.
2. Disable the hummingbird_setperf() implementation.
Cheers,
Mark
> Index: clock.c
> ===
> RCS file: /cvs/src/sys/arch/sparc64/sparc64/clock.c,v
> retrieving revision 1.72
> diff -u -p -r1.72 clock.c
> --- clock.c 10 Nov 2022 07:08:01 - 1.72
> +++ clock.c 9 Dec 2022 22:19:33 -
> @@ -567,17 +567,23 @@ cpu_initclocks(void)
> /* Default to 200MHz clock X */
> cpu_clockrate = 2;
>
> - tick_timecounter.tc_frequency = cpu_clockrate;
> - tc_init(_timecounter);
> + if (CPU_ISSUN4U || CPU_ISSUN4US)
> + impl = (getver() & VER_IMPL) >> VER_IMPL_SHIFT;
> +
> + /*
> + * The TICK frequency varies on the UltraSPARc IIe, so it isn't
> + * a suitable timecounter.
> + */
> + if (impl != IMPL_HUMMINGBIRD) {
> + tick_timecounter.tc_frequency = cpu_clockrate;
> + tc_init(_timecounter);
> + }
>
> /*
>* UltraSPARC IIe processors do have a STICK register, but it
>* lives on the PCI host bridge and isn't accessible through
>* ASR24.
>*/
> - if (CPU_ISSUN4U || CPU_ISSUN4US)
> - impl = (getver() & VER_IMPL) >> VER_IMPL_SHIFT;
> -
> sys_tick_rate = getpropint(findroot(), "stick-frequency", 0);
> if (sys_tick_rate > 0 && impl != IMPL_HUMMINGBIRD) {
> sys_tick_timecounter.tc_frequency = sys_tick_rate;
>
>
Re: sparc64: don't install %TICK timecounter on UltraSPARC IIe
Scott Cheloha: > The UltraSPARC IIe's %TICK register has a variable frequency. > miod@ confirmed that the problem is real. Indeed, I remember clocking down the Blade 100 with apm(8). Somebody with access to the hardware might want to adapt the NetBSD code that adds timecounter support for the STICK register. Unfortunately it can't be read from userland. > ok? I don't have a sparc64 to test, but this makes sense. ok naddy@ -- Christian "naddy" Weisgerber na...@mips.inka.de
nd6: Replace net lock with DAD specific adress list lock
nd6_nbr.c's static list of IPs to perform Duplicate Address Detection on
is protected by the too broad exclusive net lock.
Switch nd6_dad_timer() and helpers to a dedicated dad_lk rwlock(9) to
take pressure of the net lock.
A mutex(9) must not be used as this code path may sleep:
nd6_dad_timer() -> rtm_addr() -> route_input() -> solock().
Document all struct dadq's fields and their protection accordingly.
This makes DAD no longer grab the net lock itself; existing assertions
must remain, though, and have been annotated to ease review.
Static helper functions in this file accessing the static list do assert
that the new lock is taken for now clarify the intention and ease
testing/review.
I'd like to commit without net lock assert comment and new lock assert.
WITNESS, regress, daily usage and manual testing are fine for me.
More tests?
Feedback? OK?
diff --git a/sys/netinet6/nd6_nbr.c b/sys/netinet6/nd6_nbr.c
index acced08b3db..fedd3fdb42d 100644
--- a/sys/netinet6/nd6_nbr.c
+++ b/sys/netinet6/nd6_nbr.c
@@ -42,6 +42,7 @@
#include
#include
#include
+#include
#include
#include
@@ -62,17 +63,25 @@
#include
#endif
+/*
+ * Locks used to protect struct members in this file:
+ * D DAD lock
+ */
+
+static struct rwlock dad_lk =
+RWLOCK_INITIALIZER("dad");
+
static TAILQ_HEAD(, dadq) dadq =
TAILQ_HEAD_INITIALIZER(dadq); /* list of addresses to run DAD on */
struct dadq {
- TAILQ_ENTRY(dadq) dad_list;
- struct ifaddr *dad_ifa;
- int dad_count; /* max NS to send */
- int dad_ns_tcount; /* # of trials to send NS */
- int dad_ns_ocount; /* NS sent so far */
- int dad_ns_icount;
- int dad_na_icount;
- struct timeout dad_timer_ch;
+ TAILQ_ENTRY(dadq) dad_list; /* [D] all struct dadqs are chained */
+ struct ifaddr *dad_ifa; /* [D] IP address to perform DAD on */
+ int dad_count; /* [D] max NS to send */
+ int dad_ns_tcount; /* [D] # of trials to send NS */
+ int dad_ns_ocount; /* [D] NS sent so far */
+ int dad_ns_icount; /* [D] NS received so far */
+ int dad_na_icount; /* [D] NA received so far */
+ struct timeout dad_timer_ch;/* [D] */
};
struct dadq *nd6_dad_find(struct ifaddr *);
@@ -575,6 +584,8 @@ nd6_na_input(struct mbuf *m, int off, int icmp6len)
struct nd_opts ndopts;
char addr[INET6_ADDRSTRLEN], addr0[INET6_ADDRSTRLEN];
+ /* only called from icmp6_input() aka. .pr_input which is called from
+* ip_deliver() with NET_ASSERT_LOCKED_EXCLUSIVE() */
NET_ASSERT_LOCKED();
ifp = if_get(m->m_pkthdr.ph_ifidx);
@@ -652,6 +663,7 @@ nd6_na_input(struct mbuf *m, int off, int icmp6len)
if (ifa && (ifatoia6(ifa)->ia6_flags & IN6_IFF_TENTATIVE)) {
struct dadq *dp;
+ rw_enter_write(_lk);
dp = nd6_dad_find(ifa);
if (dp) {
dp->dad_na_icount++;
@@ -659,6 +671,7 @@ nd6_na_input(struct mbuf *m, int off, int icmp6len)
/* remove the address. */
nd6_dad_duplicated(dp);
}
+ rw_exit_write(_lk);
goto freeit;
}
@@ -1040,6 +1053,8 @@ nd6_dad_find(struct ifaddr *ifa)
{
struct dadq *dp;
+ rw_assert_anylock(_lk);
+
TAILQ_FOREACH(dp, , dad_list) {
if (dp->dad_ifa == ifa)
return dp;
@@ -1050,6 +1065,8 @@ nd6_dad_find(struct ifaddr *ifa)
void
nd6_dad_starttimer(struct dadq *dp)
{
+ rw_assert_wrlock(_lk);
+
timeout_set_proc(>dad_timer_ch, nd6_dad_timer, dp->dad_ifa);
timeout_add_msec(>dad_timer_ch, RETRANS_TIMER);
}
@@ -1057,6 +1074,8 @@ nd6_dad_starttimer(struct dadq *dp)
void
nd6_dad_stoptimer(struct dadq *dp)
{
+ rw_assert_wrlock(_lk);
+
timeout_del(>dad_timer_ch);
}
@@ -1070,6 +1089,9 @@ nd6_dad_start(struct ifaddr *ifa)
struct dadq *dp;
char addr[INET6_ADDRSTRLEN];
+ /* only called from
+* - in6_ifattach_linklocal() with NET_ASSERT_LOCKED()
+* - in6_ioctl_change_ifaddr() with NET_LOCK() */
NET_ASSERT_LOCKED();
/*
@@ -1088,8 +1110,9 @@ nd6_dad_start(struct ifaddr *ifa)
}
/* DAD already in progress */
+ rw_enter_write(_lk);
if (nd6_dad_find(ifa) != NULL)
- return;
+ goto out;
dp = malloc(sizeof(*dp), M_IP6NDP, M_NOWAIT | M_ZERO);
if (dp == NULL) {
@@ -1097,7 +1120,7 @@ nd6_dad_start(struct ifaddr *ifa)
__func__, inet_ntop(AF_INET6, >ia_addr.sin6_addr,
addr, sizeof(addr)),
ifa->ifa_ifp ? ifa->ifa_ifp->if_xname : "???");
- return;
+ goto out;
}
bzero(>dad_timer_ch,
Re: audioctl: display variables periodically
I agree with ratchov that in this instance, precise timing isn't important.
10 Dec 2022 05:53:48 Alexandre Ratchov :
> On Fri, Dec 09, 2022 at 12:43:31PM -0600, Scott Cheloha wrote:
>> On Fri, Dec 09, 2022 at 12:10:59PM +0100, Alexandre Ratchov wrote:
>>> This diff adds an option to display variables periodically. Basically
>>> it replaces this usage:
>>>
>>> while sleep 1; do audioctl play.errors; done
>>>
>>> by
>>>
>>> audioctl -w 1 play.errors
>>>
>>> The purpose of above audioctl commands is to debug underruns, so we
>>> don't want to fork a new process and reopen the device. This would
>>> trigger longer kernel code-paths and may cause additional underruns
>>> than the ones being investigated.
>>>
>>> OK?
>>
>> I like the idea, but I would like to tweak some things.
>>
>> - Add [-w wait] to the first synoptic form in the manpage. It's legal
>> to do e.g.
>>
>> # audioctl -w 1
>>
>
> done
>
>> - Call the variable "wait" in audioctl.c to match the manpage.
>>
>
> done (used wait_sec, as there's a global wait() symbol).
>
>> - Update usagestr to mention [-w wait].
>>
>
> done
>
>> - When polling variables periodically, it's better to use setitimer(2)
>> and sigsuspend(2) instead of sleep(3). setitimer(2) keeps the period
>> from drifting.
>>
>> - Let the user SIGINT (^C) out of the program without returning an
>> error to the shell.
>>
>> I'm unsure about this one, but it seems logical to give the user a
>> way to gracefully terminate the program. You say in the manpage that
>> the program will continue printing until it is interrupted.
>>
>
> I just tried these. Synchronizing the display to a clock might make
> sense if it was the sound card's clock, but here the result was boiler
> with no benefit. The intent of -w is to just show the variables from
> time to time, so keeping the code trivial is more important,
> IMHO. I've added a comment to say so.
>
> About ^C, I've changed the man page text to "audioctl will display
> variables forever." which implies that ^C is out of the scope.
>
> Index: audioctl.8
> ===
> RCS file: /cvs/src/usr.bin/audioctl/audioctl.8,v
> retrieving revision 1.4
> diff -u -p -r1.4 audioctl.8
> --- audioctl.8 23 Apr 2020 00:16:59 - 1.4
> +++ audioctl.8 10 Dec 2022 05:50:05 -
> @@ -35,9 +35,11 @@
> .Sh SYNOPSIS
> .Nm audioctl
> .Op Fl f Ar file
> +.Op Fl w Ar wait
> .Nm audioctl
> .Op Fl n
> .Op Fl f Ar file
> +.Op Fl w Ar wait
> .Ar name ...
> .Nm audioctl
> .Op Fl nq
> @@ -59,6 +61,12 @@ The default is
> Suppress printing of the variable name.
> .It Fl q
> Suppress all output when setting a variable.
> +.It Fl w Ar wait
> +Pause
> +.Ar wait
> +seconds between each display.
> +.Nm
> +will display variables forever.
> .It Ar name Ns = Ns Ar value
> Attempt to set the specified variable
> .Ar name
> @@ -130,10 +138,10 @@ audio control devices
> audio devices
> .El
> .Sh EXAMPLES
> -Display the number of bytes of silence inserted during play buffer
> -underruns since device started:
> +Once per-second, display the number of bytes of silence inserted due to
> buffer
> +underruns (since the device started playback):
> .Bd -literal -offset indent
> -# audioctl play.errors
> +# audioctl -w 1 play.errors
> .Ed
> .Pp
> Use signed 24-bit samples and 44100Hz sample rate:
> Index: audioctl.c
> ===
> RCS file: /cvs/src/usr.bin/audioctl/audioctl.c,v
> retrieving revision 1.43
> diff -u -p -r1.43 audioctl.c
> --- audioctl.c 12 Jul 2021 15:09:19 - 1.43
> +++ audioctl.c 10 Dec 2022 05:50:05 -
> @@ -43,6 +43,7 @@ struct field {
> #define STR 2
> #define ENC 3
> int type;
> + int show;
> int set;
> } fields[] = {
> {"name", , NULL, STR},
> @@ -63,11 +64,11 @@ struct field {
> };
>
> const char usagestr[] =
> - "usage: audioctl [-f file]\n"
> - " audioctl [-n] [-f file] name ...\n"
> + "usage: audioctl [-f file] [-w wait_sec]\n"
> + " audioctl [-n] [-f file] [-w wait_sec] name ...\n"
> " audioctl [-nq] [-f file] name=value ...\n";
>
> -int fd, show_names = 1, quiet = 0;
> +int fd, show_names = 1, quiet = 0, wait_sec = 0;
>
> /*
> * parse encoding string (examples: s8, u8, s16, s16le, s24be ...)
> @@ -198,20 +199,9 @@ audio_main(int argc, char **argv)
> char *lhs, *rhs;
> int set = 0;
>
> - if (ioctl(fd, AUDIO_GETSTATUS, ) == -1)
> - err(1, "AUDIO_GETSTATUS");
> - if (ioctl(fd, AUDIO_GETDEV, ) == -1)
> - err(1, "AUDIO_GETDEV");
> - if (ioctl(fd, AUDIO_GETPAR, ) == -1)
> - err(1, "AUDIO_GETPAR");
> - if (ioctl(fd, AUDIO_GETPOS, ) == -1)
> - err(1, "AUDIO_GETPOS");
> if (argc == 0) {
> - for (f = fields; f->name != NULL; f++) {
> - printf("%s=", f->name);
> - print_field(f, f->raddr);
> - printf("\n");
> - }
> + for (f = fields;
