Re: sleep.1: "while true;" -> "while :;"
On 22:49:38, 20.12.16, Jason McIntyre wrote: > On Tue, Dec 20, 2016 at 10:58:40PM +0100, Michal Mazurek wrote: > > While there is nothing wrong with "while true", "while :" is better > > and used a lot more often in the source tree. > > > > OK? > > > > i'm not sure why "while :" is better in this example, but "while true" > is clearer, i think. allowing for differences in preference, is their a > good reason to change what's there? ":" is a builtin, and appears to be the proper idiom as evidenced by the source tree: $ grep -r 'while true' . | wc -l 11 $ grep -r 'while :' . | wc -l 156 -- Michal Mazurek
Re: sleep.1: "while true;" -> "while :;"
On Wed, Dec 21, 2016 at 09:59:18AM +0100, Michal Mazurek wrote: > On 22:49:38, 20.12.16, Jason McIntyre wrote: > > On Tue, Dec 20, 2016 at 10:58:40PM +0100, Michal Mazurek wrote: > > > While there is nothing wrong with "while true", "while :" is better > > > and used a lot more often in the source tree. > > > > > > OK? > > > > > > > i'm not sure why "while :" is better in this example, but "while true" > > is clearer, i think. allowing for differences in preference, is their a > > good reason to change what's there? > > ":" is a builtin, and appears to be the proper idiom as evidenced by the > source tree: > > $ grep -r 'while true' . | wc -l > 11 > > $ grep -r 'while :' . | wc -l > 156 > morning. "true" is also a built-in. the only difference i can see really is that ":" is classed as a special built-in, but in the sleep example i don;t see that being relevant. it is maybe not used so much in the tree but it is correct. i still don;t see the point of the change. jmc
vndcompress et al import?
Hi, Is anyone aware or interested in porting vndcompress et al from NetBSD to OpenBSD? Is there any technical reason against inclusion? We have a budget for this. If anyone is interested please let me know. Cheers, Franco
snmpd improvements
Hi, Switching from net-snmp to OpenBSD's snmpd raised two issues and I'd like to know if they make sense to address: A pid file is missing. Would a patch for this be accepted? The snmpd.conf can contain static values. If these values are rewritten/changed over time by rewriting the config, snmpd needs to be restarted. Is there a technical reason for not supporting e.g. reload using HUP? I'm only looking for input, can provide patches with a bit of help. :) Cheers, Franco
ospf6d: handle interface MTU changes
Hi, After ospfd here's a diff to make ospf6d refresh his view of an interface's MTU at runtime. This needs a fresh kernel. The parent should pass the IFINFO message to its children first, and then decide to react to a possible interface change. Like for ospfd, the engine runs the FSM only if the interface state has actually changed. ok? Index: ospf6ctl/ospf6ctl.c === RCS file: /d/cvs/src/usr.sbin/ospf6ctl/ospf6ctl.c,v retrieving revision 1.43 diff -u -p -r1.43 ospf6ctl.c --- ospf6ctl/ospf6ctl.c 5 Dec 2015 13:12:40 - 1.43 +++ ospf6ctl/ospf6ctl.c 5 Dec 2016 22:40:53 - @@ -409,9 +409,10 @@ show_interface_detail_msg(struct imsg *i iface->name, print_link(iface->flags)); printf(" Internet address %s Area %s\n", log_in6addr(&iface->addr), inet_ntoa(iface->area)); - printf(" Link type %s, state %s", + printf(" Link type %s, state %s, mtu %d", get_media_descr(get_ifms_type(iface->if_type)), - get_linkstate(iface->if_type, iface->linkstate)); + get_linkstate(iface->if_type, iface->linkstate), + iface->mtu); if (iface->linkstate != LINK_STATE_DOWN && iface->baudrate > 0) { printf(", "); Index: ospf6d/kroute.c === RCS file: /d/cvs/src/usr.sbin/ospf6d/kroute.c,v retrieving revision 1.48 diff -u -p -r1.48 kroute.c --- ospf6d/kroute.c 17 Jul 2015 20:12:38 - 1.48 +++ ospf6d/kroute.c 20 Dec 2016 16:09:14 - @@ -729,12 +729,6 @@ if_change(u_short ifindex, int flags, st return; } - isvalid = (iface->flags & IFF_UP) && - LINK_STATE_IS_UP(iface->linkstate); - - if (wasvalid == isvalid) - return; /* nothing changed wrt validity */ - /* inform engine and rde about state change if interface is used */ if (iface->cflags & F_IFACE_CONFIGURED) { main_imsg_compose_ospfe(IMSG_IFINFO, 0, iface, @@ -742,6 +736,12 @@ if_change(u_short ifindex, int flags, st main_imsg_compose_rde(IMSG_IFINFO, 0, iface, sizeof(struct iface)); } + + isvalid = (iface->flags & IFF_UP) && + LINK_STATE_IS_UP(iface->linkstate); + + if (wasvalid == isvalid) + return; /* nothing changed wrt validity */ /* update redistribute list */ RB_FOREACH(kr, kroute_tree, &krt) { Index: ospf6d/ospfe.c === RCS file: /d/cvs/src/usr.sbin/ospf6d/ospfe.c,v retrieving revision 1.49 diff -u -p -r1.49 ospfe.c --- ospf6d/ospfe.c 3 Sep 2016 10:25:36 - 1.49 +++ ospf6d/ospfe.c 21 Dec 2016 10:55:11 - @@ -260,7 +260,7 @@ ospfe_dispatch_main(int fd, short event, struct imsg imsg; struct imsgev *iev = bula; struct imsgbuf *ibuf = &iev->ibuf; - int n, stub_changed, shut = 0; + int n, stub_changed, shut = 0, isvalid, wasvalid; unsigned int ifindex; if (event & EV_READ) { @@ -293,11 +293,19 @@ ospfe_dispatch_main(int fd, short event, if (iface == NULL) fatalx("interface lost in ospfe"); + wasvalid = (iface->flags & IFF_UP) && + LINK_STATE_IS_UP(iface->linkstate); + if_update(iface, ifp->mtu, ifp->flags, ifp->if_type, ifp->linkstate, ifp->baudrate); - if ((iface->flags & IFF_UP) && - LINK_STATE_IS_UP(iface->linkstate)) { + isvalid = (iface->flags & IFF_UP) && + LINK_STATE_IS_UP(iface->linkstate); + + if (wasvalid == isvalid) + break; + + if (isvalid) { if_fsm(iface, IF_EVT_UP); log_warnx("interface %s up", iface->name); } else { -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: if attach/detach netlocks
On Wed, Dec 21, 2016 at 00:13 +0100, Alexander Bluhm wrote: > On Tue, Dec 20, 2016 at 06:47:31PM +0100, Mike Belopuhov wrote: > > @@ -1109,11 +1115,16 @@ if_clone_destroy(const char *name) > > s = splnet(); > > if_down(ifp); > > splx(s); > > } > > > > - return ((*ifc->ifc_destroy)(ifp)); > > + /* XXXSMP breaks atomicity */ > > + rw_exit_write(&netlock); > > + ret = (*ifc->ifc_destroy)(ifp); > > + rw_enter_write(&netlock); > > + > > + return (ret); > > } > > This broke pflow again. > > panic: netlock: lock not held > Stopped at Debugger+0x7: leave >TIDPIDUID PRFLAGS PFLAGS CPU COMMAND > *440943 4323 0 0x2 01 ifconfig > Debugger(d09facfd,f57a2ca8,d09d23c6,f57a2ca8,d76fd000) at Debugger+0x7 > panic(d09d23c6,d09dc32f,f57a2cec,d76fd000,0) at panic+0x71 > rw_assert_wrlock(d0b56f38,40,f57a2cec,d03e506b,d8e1eb00) at > rw_assert_wrlock+0x > 3d > rw_exit_write(d0b56f38,0,7a2d4c,d046b949,d76fd000) at rw_exit_write+0x19 > pflow_clone_destroy(d76fd000,0,c0186943,d055d8ed,d96ad7a0) at > pflow_clone_destr > oy+0x71 > if_clone_destroy(f57a2e74,0,d76fd000,1,d76fd000) at if_clone_destroy+0x7b > ifioctl(d9587648,80206979,f57a2e74,d97e22d8,d956d26c) at ifioctl+0x1ed > soo_ioctl(d96405a8,80206979,f57a2e74,d97e22d8,d97b1f64) at soo_ioctl+0x21c > sys_ioctl(d97e22d8,f57a2f5c,f57a2f7c,0,f57a2fa8) at sys_ioctl+0x19f > syscall() at syscall+0x250 > --- syscall (number 9) --- > 0x14: > https://www.openbsd.org/ddb.html describes the minimum info required in bug > reports. Insufficient info makes it difficult to find and fix bugs. > ddb{1}> > > Now we have two conflicting XXXSMP workarounds in if_clone_destroy() > and pflow_clone_destroy(). > > bluhm With the diff above, the workaround in pflow_clone_destroy is not needed. The diff below fixes this for me. However, I start doubting this approach. A lot of code is now run with the lock released. While I agree that the amount of lock recursions should be minimized, I don't think that eliminating them completely is a necessity at this point. Recursive rwlock would work just fine. We can add a flag to make rrw_enter fail to recurse for the purpose of minimizing the amount of recursions. Haesbaert has experimented extensively with (r)rwlocks instead of KERNEL_LOCKs (which is what we're doing, sort of) and found out (r)rwlocks to be about 25% slower in real life applications (make build) and produce a ton of IPI calls due to sleep/wakeup cycles. He then moved on to turnstile exclusive recursive locks (bmtx) [1] with better semantics to regain performance and lower down the number of IPIs. Anyways, OK for the diff below? [1] https://github.com/bitrig/bitrig/blob/smpns/sys/kern/kern_bmtx.c diff --git sys/net/if_pflow.c sys/net/if_pflow.c index 0df0b69fd8a..8592d98a743 100644 --- sys/net/if_pflow.c +++ sys/net/if_pflow.c @@ -265,14 +265,11 @@ pflow_clone_destroy(struct ifnet *ifp) if (timeout_initialized(&sc->sc_tmo_tmpl)) timeout_del(&sc->sc_tmo_tmpl); pflow_flush(sc); m_freem(sc->send_nam); if (sc->so != NULL) { - /* XXXSMP breaks atomicity */ - rw_exit_write(&netlock); error = soclose(sc->so); - rw_enter_write(&netlock); sc->so = NULL; } if (sc->sc_flowdst != NULL) free(sc->sc_flowdst, M_DEVBUF, sc->sc_flowdst->sa_len); if (sc->sc_flowsrc != NULL)
Re: if attach/detach netlocks
On Wed, Dec 21, 2016 at 12:45:37PM +0100, Mike Belopuhov wrote: > Anyways, OK for the diff below? OK bluhm@ > diff --git sys/net/if_pflow.c sys/net/if_pflow.c > index 0df0b69fd8a..8592d98a743 100644 > --- sys/net/if_pflow.c > +++ sys/net/if_pflow.c > @@ -265,14 +265,11 @@ pflow_clone_destroy(struct ifnet *ifp) > if (timeout_initialized(&sc->sc_tmo_tmpl)) > timeout_del(&sc->sc_tmo_tmpl); > pflow_flush(sc); > m_freem(sc->send_nam); > if (sc->so != NULL) { > - /* XXXSMP breaks atomicity */ > - rw_exit_write(&netlock); > error = soclose(sc->so); > - rw_enter_write(&netlock); > sc->so = NULL; > } > if (sc->sc_flowdst != NULL) > free(sc->sc_flowdst, M_DEVBUF, sc->sc_flowdst->sa_len); > if (sc->sc_flowsrc != NULL)
Re: sleep.1: "while true;" -> "while :;"
On 2016-12-20, Jason McIntyre wrote: > i'm not sure why "while :" is better in this example, but "while true" > is clearer, i think. allowing for differences in preference, is their a > good reason to change what's there? I agree that "true" is clearer and I see no reason to change this. -- Christian "naddy" Weisgerber na...@mips.inka.de
clang amd64 libm: declare copysign() correctly
libm uses copysign() and copysignf() internally, but fails to declare the amd64 assembly versions that way. When built with clang, this results in undefined references to _libm_copysign etc. Presumably gcc replaces those calls to copysign with a builtin, but clang doesn't. Index: arch/amd64/s_copysign.S === RCS file: /cvs/src/lib/libm/arch/amd64/s_copysign.S,v retrieving revision 1.5 diff -u -p -r1.5 s_copysign.S --- arch/amd64/s_copysign.S 12 Sep 2016 19:47:01 - 1.5 +++ arch/amd64/s_copysign.S 21 Dec 2016 11:46:53 - @@ -6,6 +6,8 @@ #include +#include "abi.h" + .Lpos: .quad 0x8000 .Lneg: @@ -18,4 +20,4 @@ ENTRY(copysign) pand%xmm3,%xmm0 por %xmm1,%xmm0 ret -END(copysign) +END_STD(copysign) Index: arch/amd64/s_copysignf.S === RCS file: /cvs/src/lib/libm/arch/amd64/s_copysignf.S,v retrieving revision 1.5 diff -u -p -r1.5 s_copysignf.S --- arch/amd64/s_copysignf.S12 Sep 2016 19:47:01 - 1.5 +++ arch/amd64/s_copysignf.S21 Dec 2016 11:48:56 - @@ -6,6 +6,8 @@ #include +#include "abi.h" + .Lneg: .long 0x7fff .Lpos: @@ -18,4 +20,4 @@ ENTRY(copysignf) pand%xmm3,%xmm0 por %xmm1,%xmm0 ret -END(copysignf) +END_STD(copysignf) -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: clang amd64 libm: declare copysign() correctly
> Date: Wed, 21 Dec 2016 13:28:26 +0100 > From: Christian Weisgerber > > libm uses copysign() and copysignf() internally, but fails to declare > the amd64 assembly versions that way. When built with clang, this > results in undefined references to _libm_copysign etc. > > Presumably gcc replaces those calls to copysign with a builtin, but > clang doesn't. Looks correct to me. Hopefully guenther@ can confirm? > Index: arch/amd64/s_copysign.S > === > RCS file: /cvs/src/lib/libm/arch/amd64/s_copysign.S,v > retrieving revision 1.5 > diff -u -p -r1.5 s_copysign.S > --- arch/amd64/s_copysign.S 12 Sep 2016 19:47:01 - 1.5 > +++ arch/amd64/s_copysign.S 21 Dec 2016 11:46:53 - > @@ -6,6 +6,8 @@ > > #include > > +#include "abi.h" > + > .Lpos: > .quad 0x8000 > .Lneg: > @@ -18,4 +20,4 @@ ENTRY(copysign) > pand%xmm3,%xmm0 > por %xmm1,%xmm0 > ret > -END(copysign) > +END_STD(copysign) > Index: arch/amd64/s_copysignf.S > === > RCS file: /cvs/src/lib/libm/arch/amd64/s_copysignf.S,v > retrieving revision 1.5 > diff -u -p -r1.5 s_copysignf.S > --- arch/amd64/s_copysignf.S 12 Sep 2016 19:47:01 - 1.5 > +++ arch/amd64/s_copysignf.S 21 Dec 2016 11:48:56 - > @@ -6,6 +6,8 @@ > > #include > > +#include "abi.h" > + > .Lneg: > .long 0x7fff > .Lpos: > @@ -18,4 +20,4 @@ ENTRY(copysignf) > pand%xmm3,%xmm0 > por %xmm1,%xmm0 > ret > -END(copysignf) > +END_STD(copysignf) > -- > Christian "naddy" Weisgerber na...@mips.inka.de > >
ND6 and splsoftnet()
This diff get rids of all splsoftet() in nd6 code. I converted timers to the NET_LOCK() even if ip6_output() doesn't assert for it for the moment. That's simply the way to go. Also because these functions iterates on global data structures that will need the NET_LOCK() or a rewrite. ok? Index: netinet6/nd6.c === RCS file: /cvs/src/sys/netinet6/nd6.c,v retrieving revision 1.199 diff -u -p -r1.199 nd6.c --- netinet6/nd6.c 20 Dec 2016 18:33:43 - 1.199 +++ netinet6/nd6.c 21 Dec 2016 12:47:31 - @@ -127,7 +127,7 @@ nd6_init(void) nd6_init_done = 1; /* start timer */ - timeout_set(&nd6_slowtimo_ch, nd6_slowtimo, NULL); + timeout_set_proc(&nd6_slowtimo_ch, nd6_slowtimo, NULL); timeout_add_sec(&nd6_slowtimo_ch, ND6_SLOWTIMER_INTERVAL); nd6_rs_init(); @@ -430,15 +430,16 @@ nd6_llinfo_timer(void *arg) void nd6_timer_work(void *null) { - int s; struct nd_defrouter *dr, *ndr; struct nd_prefix *pr, *npr; struct in6_ifaddr *ia6, *nia6; + int s; - s = splsoftnet(); timeout_set(&nd6_timer_ch, nd6_timer, NULL); timeout_add_sec(&nd6_timer_ch, nd6_prune); + NET_LOCK(s); + /* expire default router list */ TAILQ_FOREACH_SAFE(dr, &nd_defrouter, dr_entry, ndr) if (dr->expire && dr->expire < time_uptime) @@ -482,7 +483,7 @@ nd6_timer_work(void *null) prelist_remove(pr); } } - splx(s); + NET_UNLOCK(s); } void @@ -1120,7 +1121,6 @@ nd6_ioctl(u_long cmd, caddr_t data, stru struct in6_nbrinfo *nbi = (struct in6_nbrinfo *)data; struct rtentry *rt; int error = 0; - int s; switch (cmd) { case SIOCGIFINFO_IN6: @@ -1142,7 +1142,6 @@ nd6_ioctl(u_long cmd, caddr_t data, stru /* flush all the prefix advertised by routers */ struct nd_prefix *pr, *npr; - s = splsoftnet(); /* First purge the addresses referenced by a prefix. */ LIST_FOREACH_SAFE(pr, &nd_prefix, ndpr_entry, npr) { struct in6_ifaddr *ia6, *ia6_next; @@ -1170,7 +1169,6 @@ nd6_ioctl(u_long cmd, caddr_t data, stru prelist_remove(pr); } - splx(s); break; } case SIOCSRTRFLUSH_IN6: @@ -1178,12 +1176,10 @@ nd6_ioctl(u_long cmd, caddr_t data, stru /* flush all the default routers */ struct nd_defrouter *dr, *ndr; - s = splsoftnet(); defrouter_reset(); TAILQ_FOREACH_SAFE(dr, &nd_defrouter, dr_entry, ndr) defrtrlist_del(dr); defrouter_select(); - splx(s); break; } case SIOCGNBRINFO_IN6: @@ -1204,13 +1200,11 @@ nd6_ioctl(u_long cmd, caddr_t data, stru *idp = htons(ifp->if_index); } - s = splsoftnet(); rt = nd6_lookup(&nb_addr, 0, ifp, ifp->if_rdomain); if (rt == NULL || (ln = (struct llinfo_nd6 *)rt->rt_llinfo) == NULL) { error = EINVAL; rtfree(rt); - splx(s); break; } expire = ln->ln_rt->rt_expire; @@ -1224,7 +1218,6 @@ nd6_ioctl(u_long cmd, caddr_t data, stru nbi->isrouter = ln->ln_router; nbi->expire = expire; rtfree(rt); - splx(s); break; } @@ -1478,12 +1471,14 @@ fail: void nd6_slowtimo(void *ignored_arg) { - int s = splsoftnet(); struct nd_ifinfo *nd6if; struct ifnet *ifp; + int s; timeout_set(&nd6_slowtimo_ch, nd6_slowtimo, NULL); timeout_add_sec(&nd6_slowtimo_ch, ND6_SLOWTIMER_INTERVAL); + + NET_LOCK(s); TAILQ_FOREACH(ifp, &ifnet, if_list) { nd6if = ND_IFINFO(ifp); if (nd6if->basereachable && /* already initialized */ @@ -1498,7 +1493,7 @@ nd6_slowtimo(void *ignored_arg) nd6if->reachable = ND_COMPUTE_RTIME(nd6if->basereachable); } } - splx(s); + NET_UNLOCK(s); } int Index: netinet6/nd6_nbr.c === RCS file: /cvs/src/sys/netinet6/nd6_nbr.c,v retrieving revision 1.112 diff -u -p -r1.112 nd6_nbr.c --- netinet6/nd6_nbr.c 21 Dec 2016 12:30:19 - 1.112 +++ netinet6/nd6_nbr.c 21 Dec 2016 12:47:31 - @@ -825,7 +825,6 @@ nd6_na_input(struct mbuf *m, int off, in */ struct nd_defrouter *dr; struct in6_addr *in6; - int s;
Re: ND6 and splsoftnet()
On Wed, Dec 21, 2016 at 01:52:45PM +0100, Martin Pieuchot wrote: > ok? OK bluhm@, but comments inline > @@ -430,15 +430,16 @@ nd6_llinfo_timer(void *arg) > void > nd6_timer_work(void *null) > { > - int s; > struct nd_defrouter *dr, *ndr; > struct nd_prefix *pr, *npr; > struct in6_ifaddr *ia6, *nia6; > + int s; > > - s = splsoftnet(); > timeout_set(&nd6_timer_ch, nd6_timer, NULL); > timeout_add_sec(&nd6_timer_ch, nd6_prune); > > + NET_LOCK(s); > + Moving timeout_set() out of the splsoftnet() feels wrong. The whole task construct could be replaced with a timeout_set_proc(), I will make a diff. > @@ -1120,7 +1121,6 @@ nd6_ioctl(u_long cmd, caddr_t data, stru > struct in6_nbrinfo *nbi = (struct in6_nbrinfo *)data; > struct rtentry *rt; > int error = 0; > - int s; > Could you add an assert here when you remove the splsoftnet(). This makes further review easier and we see bugs during runtime. > @@ -1478,12 +1471,14 @@ fail: > void > nd6_slowtimo(void *ignored_arg) > { > - int s = splsoftnet(); > struct nd_ifinfo *nd6if; > struct ifnet *ifp; > + int s; > > timeout_set(&nd6_slowtimo_ch, nd6_slowtimo, NULL); > timeout_add_sec(&nd6_slowtimo_ch, ND6_SLOWTIMER_INTERVAL); > + > + NET_LOCK(s); Again timeout_set() without lock feels wrong. It has been done in nd6_init() already. And there it is a timeout_set_proc() now. Remove the line. > @@ -825,7 +825,6 @@ nd6_na_input(struct mbuf *m, int off, in >*/ > struct nd_defrouter *dr; > struct in6_addr *in6; > - int s; Please add an assert. > nd6_dad_starttimer(struct dadq *dp, int ticks) > { > > - timeout_set(&dp->dad_timer_ch, nd6_dad_timer, dp->dad_ifa); > + timeout_set_proc(&dp->dad_timer_ch, nd6_dad_timer, dp->dad_ifa); > timeout_add(&dp->dad_timer_ch, ticks); Are we sure that timeout_set() is not called when the timer is already running? I will have a look. You diff does not make it worse. > @@ -202,7 +202,7 @@ nd6_rs_output(struct ifnet* ifp, struct > struct nd_router_solicit *rs; > struct ip6_moptions im6o; > caddr_t mac; > - int icmp6len, maxlen, s; > + int icmp6len, maxlen; Can you put an assert here? > @@ -1592,9 +1592,8 @@ pfxlist_onlink_check(void) > if ((pr->ndpr_stateflags & NDPRF_DETACHED) != 0 && > (pr->ndpr_stateflags & NDPRF_ONLINK) != 0) { > if ((e = nd6_prefix_offlink(pr)) != 0) { > - nd6log((LOG_ERR, > - "pfxlist_onlink_check: failed to " > - "make %s/%d offlink, errno=%d\n", > + nd6log((LOG_ERR, "%s: failed to make %s/%d" > + " offlink, errno=%d\n", __func__, > inet_ntop(AF_INET6, > &pr->ndpr_prefix.sin6_addr, > addr, sizeof(addr)), > @@ -1605,9 +1604,8 @@ pfxlist_onlink_check(void) > (pr->ndpr_stateflags & NDPRF_ONLINK) == 0 && > pr->ndpr_raf_onlink) { > if ((e = nd6_prefix_onlink(pr)) != 0) { > - nd6log((LOG_ERR, > - "pfxlist_onlink_check: failed to " > - "make %s/%d offlink, errno=%d\n", > + nd6log((LOG_ERR, "%s: failed to make %s/%d" > + " offlink, errno=%d\n", __func__, > inet_ntop(AF_INET6, > &pr->ndpr_prefix.sin6_addr, > addr, sizeof(addr)), > @@ -2000,7 +1998,7 @@ in6_init_address_ltimes(struct nd_prefix Plaese keep the style that the space is before the " at the end of the line.
NET_LOCK() recursion during netboot
Network boot triggers a netlock recursion: panic rw_enter sosend <- NET_LOCK() nfs_send nfs_request nfs_lookup VOP_LOOKUP vfs_lookup namei unp_connect uipc_usrreq soconnect <- NET_LOCK() sys_connect An XXXSMP workaround seems appropriate here. OK? Index: kern/uipc_usrreq.c === RCS file: src/sys/kern/uipc_usrreq.c,v retrieving revision 1.104 diff -u -p -r1.104 uipc_usrreq.c --- kern/uipc_usrreq.c 19 Dec 2016 08:36:49 - 1.104 +++ kern/uipc_usrreq.c 21 Dec 2016 16:44:03 - @@ -143,7 +143,10 @@ uipc_usrreq(struct socket *so, int req, break; case PRU_CONNECT: + /* XXXSMP breaks atomicity */ + rw_exit_write(&netlock); error = unp_connect(so, nam, p); + rw_enter_write(&netlock); break; case PRU_CONNECT2:
Re: if attach/detach netlocks
On Tue, Dec 20, 2016 at 07:33:35PM +0100, Martin Pieuchot wrote: > On 20/12/16(Tue) 18:47, Mike Belopuhov wrote: > > On Tue, Dec 20, 2016 at 17:06 +0100, Martin Pieuchot wrote: > > > > > > You'll need to release the lock before calling ifc->ifc_create in > > > if_clone_create() and do the same dance in if_clone_destroy(). > > > > > > > Indeed. > > > > > But I think that's the way to go. If you can create/destroy > > > pseudo-interface without panicing we're good. > > > > > > > Seems to work here, but it feels wrong. > > I don't think we can do much better, let's go with that we'll refine > later. And another one. splassert: ip_output: want 1 have 0 Starting stack trace... ip_output(1,0,d09e32c1,d05129ab,d1001000) at ip_output+0x6d ip_output(d9766c00,d9804200,0,0,f5770c10) at ip_output+0x6d igmp_sendpkt(d3e9d800,d3e24640,17,2e0,d3704560) at igmp_sendpkt+0x119 igmp_leavegroup(d3e24640,d09e29de,d0bbe520,d03cdcb6,0) at igmp_leavegroup+0x65 in_delmulti(d3e24640,d3704560,c0186943,f5770e74,f5770e79) at in_delmulti+0x6a vxlan_multicast_cleanup(d3e8f000,f5770e74,770d4c,d046b999,d3e8f000) at vxlan_multicast_cleanup+0xbd vxlan_clone_destroy(d3e8f000,0,c0186943,d055d8cd,d961d7a0) at vxlan_clone_destroy+0x2a if_clone_destroy(f5770e74,0,d3e8f000,1,d3e8f000) at if_clone_destroy+0x7b ifioctl(d8e6beb0,80206979,f5770e74,d96ffcc0,d96f113c) at ifioctl+0x1ed soo_ioctl(d961ca08,80206979,f5770e74,d96ffcc0,d97b1f64) at soo_ioctl+0x21c sys_ioctl(d96ffcc0,f5770f5c,f5770f7c,0,f5770fa8) at sys_ioctl+0x19f syscall() at syscall+0x250 This time vxlan_multicast_cleanup() expects that it holds the lock. vxlan_clone_destroy() does an splnet(), but ip_output() expects to have the netlock also. bluhm
Re: ND6 and splsoftnet()
On Wed, Dec 21, 2016 at 05:21:53PM +0100, Alexander Bluhm wrote: > > @@ -430,15 +430,16 @@ nd6_llinfo_timer(void *arg) > > void > > nd6_timer_work(void *null) > > { > > - int s; > > struct nd_defrouter *dr, *ndr; > > struct nd_prefix *pr, *npr; > > struct in6_ifaddr *ia6, *nia6; > > + int s; > > > > - s = splsoftnet(); > > timeout_set(&nd6_timer_ch, nd6_timer, NULL); > > timeout_add_sec(&nd6_timer_ch, nd6_prune); > > > > + NET_LOCK(s); > > + > > Moving timeout_set() out of the splsoftnet() feels wrong. The whole > task construct could be replaced with a timeout_set_proc(), I will > make a diff. This diff replaces the nd6_timer timeout and task with a timeout proc. ok? bluhm Index: netinet6/ip6_input.c === RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_input.c,v retrieving revision 1.172 diff -u -p -r1.172 ip6_input.c --- netinet6/ip6_input.c20 Dec 2016 18:33:43 - 1.172 +++ netinet6/ip6_input.c21 Dec 2016 16:49:17 - @@ -119,7 +119,6 @@ struct niqueue ip6intrq = NIQUEUE_INITIA struct ip6stat ip6stat; -void ip6_init2(void *); int ip6_check_rh0hdr(struct mbuf *, int *); int ip6_hbhchcheck(struct mbuf *, int *, int *, int *); @@ -157,19 +156,8 @@ ip6_init(void) ip6_randomid_init(); nd6_init(); frag6_init(); - ip6_init2(NULL); mq_init(&ip6send_mq, 64, IPL_SOFTNET); -} - -void -ip6_init2(void *dummy) -{ - - /* nd6_timer_init */ - bzero(&nd6_timer_ch, sizeof(nd6_timer_ch)); - timeout_set(&nd6_timer_ch, nd6_timer, NULL); - timeout_add_sec(&nd6_timer_ch, 1); } /* Index: netinet6/nd6.c === RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/nd6.c,v retrieving revision 1.199 diff -u -p -r1.199 nd6.c --- netinet6/nd6.c 20 Dec 2016 18:33:43 - 1.199 +++ netinet6/nd6.c 21 Dec 2016 17:21:12 - @@ -93,14 +93,13 @@ struct nd_prhead nd_prefix = { 0 }; int nd6_recalc_reachtm_interval = ND6_RECALC_REACHTM_INTERVAL; void nd6_slowtimo(void *); +void nd6_timer(void *); void nd6_invalidate(struct rtentry *); struct llinfo_nd6 *nd6_free(struct rtentry *, int); void nd6_llinfo_timer(void *); struct timeout nd6_slowtimo_ch; struct timeout nd6_timer_ch; -struct task nd6_timer_task; -void nd6_timer_work(void *); int fill_drlist(void *, size_t *, size_t); int fill_prlist(void *, size_t *, size_t); @@ -122,13 +121,13 @@ nd6_init(void) /* initialization of the default router list */ TAILQ_INIT(&nd_defrouter); - task_set(&nd6_timer_task, nd6_timer_work, NULL); - nd6_init_done = 1; /* start timer */ timeout_set(&nd6_slowtimo_ch, nd6_slowtimo, NULL); timeout_add_sec(&nd6_slowtimo_ch, ND6_SLOWTIMER_INTERVAL); + timeout_set_proc(&nd6_timer_ch, nd6_timer, NULL); + timeout_add_sec(&nd6_timer_ch, nd6_prune); nd6_rs_init(); } @@ -428,7 +427,7 @@ nd6_llinfo_timer(void *arg) * ND6 timer routine to expire default route list and prefix list */ void -nd6_timer_work(void *null) +nd6_timer(void *arg) { int s; struct nd_defrouter *dr, *ndr; @@ -436,7 +435,6 @@ nd6_timer_work(void *null) struct in6_ifaddr *ia6, *nia6; s = splsoftnet(); - timeout_set(&nd6_timer_ch, nd6_timer, NULL); timeout_add_sec(&nd6_timer_ch, nd6_prune); /* expire default router list */ @@ -483,12 +481,6 @@ nd6_timer_work(void *null) } } splx(s); -} - -void -nd6_timer(void *ignored_arg) -{ - task_add(systq, &nd6_timer_task); } /* Index: netinet6/nd6.h === RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/nd6.h,v retrieving revision 1.65 diff -u -p -r1.65 nd6.h --- netinet6/nd6.h 28 Nov 2016 13:59:51 - 1.65 +++ netinet6/nd6.h 21 Dec 2016 16:48:46 - @@ -223,8 +223,6 @@ extern int nd6_debug; #define nd6log(x) do { if (nd6_debug) log x; } while (0) -extern struct timeout nd6_timer_ch; - union nd_opts { struct nd_opt_hdr *nd_opt_array[9]; struct { @@ -260,7 +258,6 @@ int nd6_options(union nd_opts *); struct rtentry *nd6_lookup(struct in6_addr *, int, struct ifnet *, u_int); void nd6_setmtu(struct ifnet *); void nd6_llinfo_settimer(struct llinfo_nd6 *, int); -void nd6_timer(void *); void nd6_purge(struct ifnet *); void nd6_nud_hint(struct rtentry *); void nd6_rtrequest(struct ifnet *, int, struct rtentry *);
Re: NET_LOCK() recursion during netboot
On Wed, Dec 21, 2016 at 04:52:32PM +, Visa Hankala wrote: > Network boot triggers a netlock recursion: > > panic > rw_enter > sosend <- NET_LOCK() > nfs_send > nfs_request > nfs_lookup > VOP_LOOKUP > vfs_lookup > namei > unp_connect > uipc_usrreq > soconnect <- NET_LOCK() > sys_connect > > An XXXSMP workaround seems appropriate here. > > OK? I was wondering wether the unp or the nfs layer is the right place for the workaround. As we have another XXXSMP in uipc_usrreq() this seems correct. OK bluhm@ > > Index: kern/uipc_usrreq.c > === > RCS file: src/sys/kern/uipc_usrreq.c,v > retrieving revision 1.104 > diff -u -p -r1.104 uipc_usrreq.c > --- kern/uipc_usrreq.c19 Dec 2016 08:36:49 - 1.104 > +++ kern/uipc_usrreq.c21 Dec 2016 16:44:03 - > @@ -143,7 +143,10 @@ uipc_usrreq(struct socket *so, int req, > break; > > case PRU_CONNECT: > + /* XXXSMP breaks atomicity */ > + rw_exit_write(&netlock); > error = unp_connect(so, nam, p); > + rw_enter_write(&netlock); > break; > > case PRU_CONNECT2:
Re: snmpd improvements
On Wed, Dec 21, 2016 at 10:40:48AM +0100, Franco Fichtner wrote: > Hi, > > Switching from net-snmp to OpenBSD's snmpd raised two > issues and I'd like to know if they make sense to address: > > A pid file is missing. Would a patch for this be accepted? As far as I know, the OpenBSD project avoids pid files for a longer time. Because of race conditions and other reasons. > The snmpd.conf can contain static values. If these values > are rewritten/changed over time by rewriting the config, > snmpd needs to be restarted. Is there a technical reason > for not supporting e.g. reload using HUP? > > I'm only looking for input, can provide patches with a bit > of help. :) The HUP feature isn't that import for the OpenSNMPd because it does not cost that much time to reload the information out of the kernel and the daemon don't keep stat from it's clients. In the case of the OpenOSPF its much more important because it take a while to restart. And during the starting time the router would not be available. Just my two cents. bye, Jan
Re: ND6 and splsoftnet()
On 21/12/16(Wed) 18:22, Alexander Bluhm wrote: > On Wed, Dec 21, 2016 at 05:21:53PM +0100, Alexander Bluhm wrote: > > > @@ -430,15 +430,16 @@ nd6_llinfo_timer(void *arg) > > > void > > > nd6_timer_work(void *null) > > > { > > > - int s; > > > struct nd_defrouter *dr, *ndr; > > > struct nd_prefix *pr, *npr; > > > struct in6_ifaddr *ia6, *nia6; > > > + int s; > > > > > > - s = splsoftnet(); > > > timeout_set(&nd6_timer_ch, nd6_timer, NULL); > > > timeout_add_sec(&nd6_timer_ch, nd6_prune); > > > > > > + NET_LOCK(s); > > > + > > > > Moving timeout_set() out of the splsoftnet() feels wrong. The whole > > task construct could be replaced with a timeout_set_proc(), I will > > make a diff. > > This diff replaces the nd6_timer timeout and task with a timeout > proc. Can we wait until the experiment is complete? Until then I'd like to keep timeout_set_proc() only for timeout converted because they need the NET_LOCK(). If we discover a flaw and need to "back out" we can simply do #define timeout_set_proc(x) timeout_set and #define NET_LOCK(s) s = splsoftnet() But if we start to convert other stuff, like the diff below does, this wont work. > > ok? > > bluhm > > Index: netinet6/ip6_input.c > === > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_input.c,v > retrieving revision 1.172 > diff -u -p -r1.172 ip6_input.c > --- netinet6/ip6_input.c 20 Dec 2016 18:33:43 - 1.172 > +++ netinet6/ip6_input.c 21 Dec 2016 16:49:17 - > @@ -119,7 +119,6 @@ struct niqueue ip6intrq = NIQUEUE_INITIA > > struct ip6stat ip6stat; > > -void ip6_init2(void *); > int ip6_check_rh0hdr(struct mbuf *, int *); > > int ip6_hbhchcheck(struct mbuf *, int *, int *, int *); > @@ -157,19 +156,8 @@ ip6_init(void) > ip6_randomid_init(); > nd6_init(); > frag6_init(); > - ip6_init2(NULL); > > mq_init(&ip6send_mq, 64, IPL_SOFTNET); > -} > - > -void > -ip6_init2(void *dummy) > -{ > - > - /* nd6_timer_init */ > - bzero(&nd6_timer_ch, sizeof(nd6_timer_ch)); > - timeout_set(&nd6_timer_ch, nd6_timer, NULL); > - timeout_add_sec(&nd6_timer_ch, 1); > } > > /* > Index: netinet6/nd6.c > === > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/nd6.c,v > retrieving revision 1.199 > diff -u -p -r1.199 nd6.c > --- netinet6/nd6.c20 Dec 2016 18:33:43 - 1.199 > +++ netinet6/nd6.c21 Dec 2016 17:21:12 - > @@ -93,14 +93,13 @@ struct nd_prhead nd_prefix = { 0 }; > int nd6_recalc_reachtm_interval = ND6_RECALC_REACHTM_INTERVAL; > > void nd6_slowtimo(void *); > +void nd6_timer(void *); > void nd6_invalidate(struct rtentry *); > struct llinfo_nd6 *nd6_free(struct rtentry *, int); > void nd6_llinfo_timer(void *); > > struct timeout nd6_slowtimo_ch; > struct timeout nd6_timer_ch; > -struct task nd6_timer_task; > -void nd6_timer_work(void *); > > int fill_drlist(void *, size_t *, size_t); > int fill_prlist(void *, size_t *, size_t); > @@ -122,13 +121,13 @@ nd6_init(void) > /* initialization of the default router list */ > TAILQ_INIT(&nd_defrouter); > > - task_set(&nd6_timer_task, nd6_timer_work, NULL); > - > nd6_init_done = 1; > > /* start timer */ > timeout_set(&nd6_slowtimo_ch, nd6_slowtimo, NULL); > timeout_add_sec(&nd6_slowtimo_ch, ND6_SLOWTIMER_INTERVAL); > + timeout_set_proc(&nd6_timer_ch, nd6_timer, NULL); > + timeout_add_sec(&nd6_timer_ch, nd6_prune); > > nd6_rs_init(); > } > @@ -428,7 +427,7 @@ nd6_llinfo_timer(void *arg) > * ND6 timer routine to expire default route list and prefix list > */ > void > -nd6_timer_work(void *null) > +nd6_timer(void *arg) > { > int s; > struct nd_defrouter *dr, *ndr; > @@ -436,7 +435,6 @@ nd6_timer_work(void *null) > struct in6_ifaddr *ia6, *nia6; > > s = splsoftnet(); > - timeout_set(&nd6_timer_ch, nd6_timer, NULL); > timeout_add_sec(&nd6_timer_ch, nd6_prune); > > /* expire default router list */ > @@ -483,12 +481,6 @@ nd6_timer_work(void *null) > } > } > splx(s); > -} > - > -void > -nd6_timer(void *ignored_arg) > -{ > - task_add(systq, &nd6_timer_task); > } > > /* > Index: netinet6/nd6.h > === > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/nd6.h,v > retrieving revision 1.65 > diff -u -p -r1.65 nd6.h > --- netinet6/nd6.h28 Nov 2016 13:59:51 - 1.65 > +++ netinet6/nd6.h21 Dec 2016 16:48:46 - > @@ -223,8 +223,6 @@ extern int nd6_debug; > > #define nd6log(x)do { if (nd6_debug) log x; } while (0) > > -extern struct timeout nd6_timer_ch; > - > union nd_opts { > struct nd_opt_hdr *nd_opt_array[9]; > struct { > @@ -260,7 +258,6 @@ int nd6_options(union nd_opts *); > struct rtentry *nd6_
Re: BFD: route get and route monitor
On 17.12.2016. 14:05, Peter Hessler wrote: > Updated output, requested by Theo. A normal get will show just the bfd > state, use "-bfd" to get all of the information. > > OK? > > $ route -n get 203.0.113.9 >route to: 203.0.113.9 > destination: 203.0.113.9 >mask: 255.255.255.255 > interface: em1 > if address: 203.0.113.1 >priority: 4 (connected) > flags: > BFD: async state up remote up > use mtuexpire >83924 0 133 > sockaddrs: > > $ route -n get 203.0.113.9 -bfd >route to: 203.0.113.9 > destination: 203.0.113.9 >mask: 255.255.255.255 > interface: em1 > if address: 203.0.113.1 >priority: 4 (connected) > flags: > BFD: async state up remote up laststate down error 0 > diag none remote neighbor-down > discr 186919089 remote 55 > uptime 05d 2h07m29s > mintx 100 minrx 100 minecho 0 multiplier 3 > use mtuexpire >83923 0 229 > sockaddrs: Hi, it seems that bfd is working with Force10 S4810 and Extreme Networks x460 switches. I can test it with cisco c6k5 if you want? Thank you. bsdtest box is connected in this way: 192.168.11.1 - bsdtest ix0 192.168.11.2 - Dell Force10 S4810 9.11(0.0) 192.168.112.1 - bsdtest ix1 192.168.112.2 - Extreme Networks X460-48t 15.5.3.4 BFD between OpenBSD - Force10 S4810 root@bsdtest:~ # route -n get 192.168.11.2 -bfd route to: 192.168.11.2 destination: 192.168.11.2 mask: 255.255.255.255 interface: ix0 if address: 192.168.11.1 priority: 3 () flags: BFD: async state up remote up laststate down error 0 diag none remote none discr 2137079205 remote 4 uptime 07m26s last state time 01s mintx 100 minrx 100 minecho 0 multiplier 3 use mtuexpire 113 0 628 sockaddrs: root@bsdtest:~ # route -n monitor sockaddrs: 192.168.11.2 192.168.11.1 got message of size 128 on Wed Dec 21 21:55:15 2016 RTM_BFD: bidirectional forwarding detection: len 128 BFD: async state up remote up S4810#sh bfd neighbors detail Session Discriminator: 4 Neighbor Discriminator: 0 Local Addr: 192.168.11.2 Local MAC Addr: 00:01:e8:8a:ea:53 Remote Addr: 192.168.11.1 Remote MAC Addr: 00:25:90:5d:ca:38 Int: TenGigabitEthernet 0/3 State: Up Configured parameters: TX: 200ms, RX: 200ms, Multiplier: 3 Neighbor parameters: TX:0ms, RX:0ms, Multiplier: 0 Actual parameters: TX: 1000ms, RX: 1000ms, Multiplier: 3 Role: Active Delete session on Down: False Client Registered: RTM Uptime: 00:06:49 Statistics: Number of packets received from neighbor: 514 Number of packets sent to neighbor: 465 Number of state changes: 2 Number of messages from IFA about port state change: 0 Number of messages communicated b/w Manager and Agent: 4 BFD between OpenBSD and Extreme root@bsdtest:~ # route -n get 192.168.112.2 -bfd route to: 192.168.112.2 destination: 192.168.112.2 mask: 255.255.255.255 interface: ix1 if address: 192.168.112.1 priority: 3 () flags: BFD: async state up remote up laststate down error 0 diag none remote none discr 3953390566 remote 2 uptime 10m17s last state time 01s mintx 100 minrx 100 minecho 0 multiplier 3 use mtuexpire 87 0 511 sockaddrs: root@bsdtest:~ # route -n monitor sockaddrs: 192.168.112.2 192.168.112.1 got message of size 128 on Wed Dec 21 21:56:30 2016 RTM_BFD: bidirectional forwarding detection: len 128 BFD: async state up remote up ExtTestFW.30 # sh bfd session detail Neighbor : 192.168.112.1 Local : 192.168.112.2 VR-Name: VR-Default Interface : obfd Session Type : Single Hop State : Up Detect Time: 3000 ms Age : 550 ms Discriminator (local/remote): 2 / 3953390566 Demand Mode (local/remote) : Off / Off Poll (local/remote) : Off / Off Tx Interval (local/remote) : 1000 / 1000 ms Rx Interval (local/remote) : 1000 / 1000 ms oper Tx Interval: 1000 ms oper Rx Interval: 1000 ms Multiplier (local/remote) : 3 / 3 Local Diag : 0 (No Diagnostic) Remote Diag : 0 (No Diagnostic) Authentication : None Clients : STATIC Uptime : 00 days 00 hours 10 minutes 29 seconds Up Count: 2 Last Valid Packet Rx: 20:52:44.173792 Last Packet Tx : 20:52:43.950240 root@bsdtest:~ # netstat -rnf inet Routing tables Internet: DestinationGatewayFlags Refs Use Mtu Prio Iface default161.53.253.225 UGS3 15 - 8 em0 224/4 127.0.0.1 URS0
Re: clang amd64 libm: declare copysign() correctly
On Wed, Dec 21, 2016 at 4:49 AM, Mark Kettenis wrote: >> Date: Wed, 21 Dec 2016 13:28:26 +0100 >> From: Christian Weisgerber >> >> libm uses copysign() and copysignf() internally, but fails to declare >> the amd64 assembly versions that way. When built with clang, this >> results in undefined references to _libm_copysign etc. >> >> Presumably gcc replaces those calls to copysign with a builtin, but >> clang doesn't. > > Looks correct to me. Hopefully guenther@ can confirm? Looks right. Might as well make the same change in libm/arch/i387/ so that they stay in sync for the symbols that they provide. ok guenther@ (Hmm, copysign(3) doesn't mention that it's in C99)
Re: BFD: route get and route monitor
Hrvoje Popovski(hrv...@srce.hr) on 2016.12.21 22:03:56 +0100: > On 17.12.2016. 14:05, Peter Hessler wrote: > > Updated output, requested by Theo. A normal get will show just the bfd > > state, use "-bfd" to get all of the information. > > > > OK? > > > > $ route -n get 203.0.113.9 > >route to: 203.0.113.9 > > destination: 203.0.113.9 > >mask: 255.255.255.255 > > interface: em1 > > if address: 203.0.113.1 > >priority: 4 (connected) > > flags: > > BFD: async state up remote up > > use mtuexpire > >83924 0 133 > > sockaddrs: > > > > $ route -n get 203.0.113.9 -bfd > >route to: 203.0.113.9 > > destination: 203.0.113.9 > >mask: 255.255.255.255 > > interface: em1 > > if address: 203.0.113.1 > >priority: 4 (connected) > > flags: > > BFD: async state up remote up laststate down error 0 > > diag none remote neighbor-down > > discr 186919089 remote 55 > > uptime 05d 2h07m29s > > mintx 100 minrx 100 minecho 0 multiplier 3 > > use mtuexpire > >83923 0 229 > > sockaddrs: > > > Hi, > > it seems that bfd is working with Force10 S4810 and Extreme Networks > x460 switches. I can test it with cisco c6k5 if you want? Hei, i'm sure phessler (who might not read this for a couple of days) is happy about any test you can do. And thanks for doing these tests! /Benno
Re: ospf6d: handle interface MTU changes
i think this is ok Jeremie Courreges-Anglas(j...@wxcvbn.org) on 2016.12.21 12:08:23 +0100: > > Hi, > > After ospfd here's a diff to make ospf6d refresh his view of an > interface's MTU at runtime. This needs a fresh kernel. > > The parent should pass the IFINFO message to its children first, and > then decide to react to a possible interface change. Like for ospfd, > the engine runs the FSM only if the interface state has actually > changed. > > ok? > > > Index: ospf6ctl/ospf6ctl.c > === > RCS file: /d/cvs/src/usr.sbin/ospf6ctl/ospf6ctl.c,v > retrieving revision 1.43 > diff -u -p -r1.43 ospf6ctl.c > --- ospf6ctl/ospf6ctl.c 5 Dec 2015 13:12:40 - 1.43 > +++ ospf6ctl/ospf6ctl.c 5 Dec 2016 22:40:53 - > @@ -409,9 +409,10 @@ show_interface_detail_msg(struct imsg *i > iface->name, print_link(iface->flags)); > printf(" Internet address %s Area %s\n", > log_in6addr(&iface->addr), inet_ntoa(iface->area)); > - printf(" Link type %s, state %s", > + printf(" Link type %s, state %s, mtu %d", > get_media_descr(get_ifms_type(iface->if_type)), > - get_linkstate(iface->if_type, iface->linkstate)); > + get_linkstate(iface->if_type, iface->linkstate), > + iface->mtu); > if (iface->linkstate != LINK_STATE_DOWN && > iface->baudrate > 0) { > printf(", "); > Index: ospf6d/kroute.c > === > RCS file: /d/cvs/src/usr.sbin/ospf6d/kroute.c,v > retrieving revision 1.48 > diff -u -p -r1.48 kroute.c > --- ospf6d/kroute.c 17 Jul 2015 20:12:38 - 1.48 > +++ ospf6d/kroute.c 20 Dec 2016 16:09:14 - > @@ -729,12 +729,6 @@ if_change(u_short ifindex, int flags, st > return; > } > > - isvalid = (iface->flags & IFF_UP) && > - LINK_STATE_IS_UP(iface->linkstate); > - > - if (wasvalid == isvalid) > - return; /* nothing changed wrt validity */ > - > /* inform engine and rde about state change if interface is used */ > if (iface->cflags & F_IFACE_CONFIGURED) { > main_imsg_compose_ospfe(IMSG_IFINFO, 0, iface, > @@ -742,6 +736,12 @@ if_change(u_short ifindex, int flags, st > main_imsg_compose_rde(IMSG_IFINFO, 0, iface, > sizeof(struct iface)); > } > + > + isvalid = (iface->flags & IFF_UP) && > + LINK_STATE_IS_UP(iface->linkstate); > + > + if (wasvalid == isvalid) > + return; /* nothing changed wrt validity */ > > /* update redistribute list */ > RB_FOREACH(kr, kroute_tree, &krt) { > Index: ospf6d/ospfe.c > === > RCS file: /d/cvs/src/usr.sbin/ospf6d/ospfe.c,v > retrieving revision 1.49 > diff -u -p -r1.49 ospfe.c > --- ospf6d/ospfe.c3 Sep 2016 10:25:36 - 1.49 > +++ ospf6d/ospfe.c21 Dec 2016 10:55:11 - > @@ -260,7 +260,7 @@ ospfe_dispatch_main(int fd, short event, > struct imsg imsg; > struct imsgev *iev = bula; > struct imsgbuf *ibuf = &iev->ibuf; > - int n, stub_changed, shut = 0; > + int n, stub_changed, shut = 0, isvalid, wasvalid; > unsigned int ifindex; > > if (event & EV_READ) { > @@ -293,11 +293,19 @@ ospfe_dispatch_main(int fd, short event, > if (iface == NULL) > fatalx("interface lost in ospfe"); > > + wasvalid = (iface->flags & IFF_UP) && > + LINK_STATE_IS_UP(iface->linkstate); > + > if_update(iface, ifp->mtu, ifp->flags, ifp->if_type, > ifp->linkstate, ifp->baudrate); > > - if ((iface->flags & IFF_UP) && > - LINK_STATE_IS_UP(iface->linkstate)) { > + isvalid = (iface->flags & IFF_UP) && > + LINK_STATE_IS_UP(iface->linkstate); > + > + if (wasvalid == isvalid) > + break; > + > + if (isvalid) { > if_fsm(iface, IF_EVT_UP); > log_warnx("interface %s up", iface->name); > } else { > > > -- > jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE >
splassert: ip_output: want 1 have 0
splassert: ip_output: want 1 have 0 Starting stack trace... ip_output() at ip_output+0x7d ipsp_process_done() at ipsp_process_done+0x2ad esp_output_cb() at esp_output_cb+0x135 taskq_thread() at taskq_thread+0x6c end trace frame: 0x0, count: 253 End of stack trace. This makes no sense to me since esp_output_cb() calls ipsp_process_done() while at splsoftnet. What am I missing?
crtbegin.o and .ctors
Our crtbegin.o has: [ 8] .ctorsPROGBITS 000248 04 00 A 0 0 4 whereas crtend.o has: [ 8] .ctorsPROGBITS 48 04 00 WA 0 0 4 So in crtbegin.o the .ctors section is read-only while in crtend.o it is writable. Our current linker concatenates those sections as intended into a writable section in the output binary. But lld, the llvm linker, creates two separate sections, which causes the output binary to crash as the code that runs constructors doesn't encounter the sentinel from crtend.o. The Solaris linker exhibits the same behaviour. Turns out that the ELF spec actually suggests that lld is right here as the section flags are different. Therefore I think we should apply the following diff. ok? Index: lib/csu/crtbegin.c === RCS file: /cvs/src/lib/csu/crtbegin.c,v retrieving revision 1.20 diff -u -p -r1.20 crtbegin.c --- lib/csu/crtbegin.c 10 Nov 2015 04:14:03 - 1.20 +++ lib/csu/crtbegin.c 21 Dec 2016 23:36:09 - @@ -84,9 +84,9 @@ __asm(".hidden __dso_handle"); long __guard_local __dso_hidden __attribute__((section(".openbsd.randomdata"))); -static const init_f __CTOR_LIST__[1] +static init_f __CTOR_LIST__[1] __attribute__((section(".ctors"))) = { (void *)-1 }; /* XXX */ -static const init_f __DTOR_LIST__[1] +static init_f __DTOR_LIST__[1] __attribute__((section(".dtors"))) = { (void *)-1 }; /* XXX */ static void__dtors(void) __used;
Re: NET_LOCK() pflow panic
Hi. On Tue, Dec 20, 2016 at 03:28:38PM +0100, Martin Pieuchot wrote: [...] > I don't have a solution for the moment and I want to be sure we know all > recursions before trying to write a fix. So here's a diff that mark the > recursions with a XXXSMP like in the NFS case. I think I have another case of this. (Note that it's a single processor VM, not SMP). So far it's reproduceable 2/2 so if you need any additional info or testing please let me know. panic: rw_enter: netlock locking against myself Stopped at Debugger+0x9: leave TIDPIDUID PRFLAGS PFLAGS CPU COMMAND *389277 87361500 0x3 00 ssh Debugger() at Debugger+0x9 panic() at panic+0xfe rw_enter() at rw_enter+0x1c1 sosend() at sosend+0x114 Full dmesg and trace below. Copyright (c) 1982, 1986, 1989, 1991, 1993 The Regents of the University of California. All rights reserved. Copyright (c) 1995-2016 OpenBSD. All rights reserved. https://www.OpenBSD.org OpenBSD 6.0-current (GENERIC) #61: Tue Dec 20 15:30:01 MST 2016 bu...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC real mem = 2130698240 (2031MB) avail mem = 2061586432 (1966MB) mpath0 at root scsibus0 at mpath0: 256 targets mainbus0 at root bios0 at mainbus0: SMBIOS rev. 2.4 @ 0xf64e0 (10 entries) bios0: vendor Bochs version "Bochs" date 01/01/2011 bios0: QEMU Standard PC (i440FX + PIIX, 1996) acpi0 at bios0: rev 0 acpi0: sleep states S3 S4 S5 acpi0: tables DSDT FACP SSDT APIC HPET acpi0: wakeup devices acpitimer0 at acpi0: 3579545 Hz, 24 bits acpimadt0 at acpi0 addr 0xfee0: PC-AT compat cpu0 at mainbus0: apid 0 (boot processor) cpu0: QEMU Virtual CPU version 2.5+, 2400.57 MHz cpu0: FPU,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,SSE3,VMX,CX16,x2APIC,HV,NXE,LONG,LAHF cpu0: 64KB 64b/line 2-way I-cache, 64KB 64b/line 2-way D-cache, 512KB 64b/line 16-way L2 cache cpu0: ITLB 255 4KB entries direct-mapped, 255 4MB entries direct-mapped cpu0: DTLB 255 4KB entries direct-mapped, 255 4MB entries direct-mapped cpu0: smt 0, core 0, package 0 mtrr: Pentium Pro MTRR support, 8 var ranges, 88 fixed ranges cpu0: apic clock running at 999MHz ioapic0 at mainbus0: apid 0 pa 0xfec0, version 11, 24 pins acpihpet0 at acpi0: 1 Hz acpiprt0 at acpi0: bus 0 (PCI0) acpicpu0 at acpi0: C1(@1 halt!) "ACPI0006" at acpi0 not configured "PNP0303" at acpi0 not configured "PNP0F13" at acpi0 not configured "PNP0700" at acpi0 not configured "PNP0501" at acpi0 not configured "PNP0A06" at acpi0 not configured "PNP0A06" at acpi0 not configured "PNP0A06" at acpi0 not configured pvbus0 at mainbus0: KVM pci0 at mainbus0 bus 0 pchb0 at pci0 dev 0 function 0 "Intel 82441FX" rev 0x02 pcib0 at pci0 dev 1 function 0 "Intel 82371SB ISA" rev 0x00 pciide0 at pci0 dev 1 function 1 "Intel 82371SB IDE" rev 0x00: DMA, channel 0 wired to compatibility, channel 1 wired to compatibility pciide0: channel 0 disabled (no drives) atapiscsi0 at pciide0 channel 1 drive 0 scsibus1 at atapiscsi0: 2 targets cd0 at scsibus1 targ 0 lun 0: ATAPI 5/cdrom removable cd0(pciide0:1:0): using PIO mode 4, DMA mode 2 piixpm0 at pci0 dev 1 function 3 "Intel 82371AB Power" rev 0x03: apic 0 int 9 iic0 at piixpm0 vga1 at pci0 dev 2 function 0 "Cirrus Logic CL-GD5446" rev 0x00 wsdisplay0 at vga1 mux 1: console (80x25, vt100 emulation) wsdisplay0: screen 1-5 added (80x25, vt100 emulation) virtio0 at pci0 dev 3 function 0 "Qumranet Virtio RNG" rev 0x00 viornd0 at virtio0 virtio0: apic 0 int 11 virtio1 at pci0 dev 4 function 0 "Qumranet Virtio Network" rev 0x00 vio0 at virtio1: address 52:54:00:f6:02:ea virtio1: msix shared virtio2 at pci0 dev 5 function 0 "Qumranet Virtio Storage" rev 0x00 vioblk0 at virtio2 scsibus2 at vioblk0: 2 targets sd0 at scsibus2 targ 0 lun 0: SCSI3 0/direct fixed sd0: 16384MB, 512 bytes/sector, 33554432 sectors virtio2: msix shared virtio3 at pci0 dev 6 function 0 "Qumranet Virtio Memory" rev 0x00 viomb0 at virtio3 virtio3: apic 0 int 10 virtio4 at pci0 dev 7 function 0 "Qumranet Virtio Storage" rev 0x00 vioblk1 at virtio4 scsibus3 at vioblk1: 2 targets sd1 at scsibus3 targ 0 lun 0: SCSI3 0/direct fixed sd1: 16384MB, 512 bytes/sector, 33554432 sectors virtio4: msix shared isa0 at pcib0 isadma0 at isa0 fdc0 at isa0 port 0x3f0/6 irq 6 drq 2 fd0 at fdc0 drive 1: density unknown com0 at isa0 port 0x3f8/8 irq 4: ns16550a, 16 byte fifo com0: console pckbc0 at isa0 port 0x60/5 irq 1 irq 12 pckbd0 at pckbc0 (kbd slot) wskbd0 at pckbd0: console keyboard, using wsdisplay0 pms0 at pckbc0 (aux slot) wsmouse0 at pms0 mux 0 pcppi0 at isa0 port 0x61 spkr0 at pcppi0 vmm0 at mainbus0: VMX/EPT vscsi0 at root scsibus4 at vscsi0: 256 targets softraid0 at root scsibus5 at softraid0: 256 targets root on sd0a (1afc9f32ece695a9.a) swap on sd0b dump on sd0b Automatic boot in progress: starting file system checks. /dev/rsd0a: file system is clean; not checking /dev/rsd1a: file system is clean; not checking setting tty flags pf enabl
Re: NET_LOCK() pr_sysctl
On Tue, Dec 20, 2016 at 05:37:20PM +, Alexander Bluhm wrote: > Obviosly a NET_LOCK() is missing in tcp_sysctl(). > > I think it is better to place the lock into net_sysctl() where all > the protocol sysctls are called via pr_sysctl. Then we don't have > to decide each case individually. As calling sysctl(2) is in the > slow path, doing fine grained locking has no benefit. Many sysctl > cases copy out a struct. Having a lock around that keeps the struct > consistent. > Holding locks across copyout/copyin is always fishy. In this particular case, what happens if the access results in a page fault and the area comes from a nfs mapped file? If network i/o is done from the same context, this should result in 'locking against myself' assertion failure. That said, I'm not exactly familiar with the area, so maybe that's a false alarm. -- Mateusz Guzik
Re: NET_LOCK() pflow panic
On Thu, Dec 22, 2016 at 11:32:26AM +1100, Darren Tucker wrote: > > I don't have a solution for the moment and I want to be sure we know all > > recursions before trying to write a fix. So here's a diff that mark the > > recursions with a XXXSMP like in the NFS case. Looks like visa@'s crash. I have already OKed his fix. bluhm > login: panic: rw_enter: netlock locking against myself > Stopped at Debugger+0x9: leave >TIDPIDUID PRFLAGS PFLAGS CPU COMMAND > *389277 87361500 0x3 00 ssh > Debugger() at Debugger+0x9 > panic() at panic+0xfe > rw_enter() at rw_enter+0x1c1 > sosend() at sosend+0x114 > nfs_send() at nfs_send+0x60 > nfs_request() at nfs_request+0x408 > nfs_lookup() at nfs_lookup+0x28c > VOP_LOOKUP() at VOP_LOOKUP+0x39 > vfs_lookup() at vfs_lookup+0x2fe > namei() at namei+0x346 > unp_connect() at unp_connect+0xd4 > uipc_usrreq() at uipc_usrreq+0x26f > soconnect() at soconnect+0xb1 > sys_connect() at sys_connect+0x1c2
Re: NET_LOCK() pflow panic
On Thu, Dec 22, 2016 at 11:41 AM, Alexander Bluhm wrote: > On Thu, Dec 22, 2016 at 11:32:26AM +1100, Darren Tucker wrote: >> > I don't have a solution for the moment and I want to be sure we know all >> > recursions before trying to write a fix. So here's a diff that mark the >> > recursions with a XXXSMP like in the NFS case. > > Looks like visa@'s crash. I have already OKed his fix. Thanks, I'll give that diff a go. (The "XXXSMP" comment mislead me since my problem was observed on a single processor machine.) -- Darren Tucker (dtucker at zip.com.au) GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860 37F4 9357 ECEF 11EA A6FA (new) Good judgement comes with experience. Unfortunately, the experience usually comes from bad judgement.
Re: Minor improvement to socket(2) man page
Philip Guenther wrote: > On Wed, Sep 21, 2016 at 6:23 PM, Michael McConville wrote: > > The current version is somewhat awkward and forgets to mention that > > errno is set. I adapted the paragraph found in most other system call > > man pages. > > There are six syscalls that return an fd on success...and their > manpages all have totally different wording for the return value. > Whee. > > accept > The call returns -1 on error. If it succeeds, it returns a non-negative > integer that is a descriptor for the accepted socket. > > fhopen > Upon successful completion, fhopen() returns the file descriptor for the > opened file, while fhstat() and fhstatfs() return 0. Otherwise, -1 is > returned and errno is set to indicate the error. > > kqueue > kqueue() creates a new kernel event queue and returns a file descriptor. > If there was an error creating the kernel event queue, a value of -1 is > returned and errno set. > > open > If successful, open() returns a non-negative integer, termed a file > descriptor. Otherwise, a value of -1 is returned and errno is set to > indicate the error. > > openat: same manpage as open but totally unmentioned in the RETURN > VALUES section > > socket > A -1 is returned if an error occurs, otherwise the return value is a > descriptor referencing the socket. > > Some consistency that doesn't sacrifice clarity would be nice... Before I do the busy-work: do the man page gurus have a preferred phrasing? I prefer fhopen(2)'s (listed above), which seems to be the most common.
Re: snmpd improvements
On 16-12-21 9:41 AM, Jan Klemkow wrote: On Wed, Dec 21, 2016 at 10:40:48AM +0100, Franco Fichtner wrote: The snmpd.conf can contain static values. If these values are rewritten/changed over time by rewriting the config, snmpd needs to be restarted. Is there a technical reason for not supporting e.g. reload using HUP? I'm only looking for input, can provide patches with a bit of help. :) The HUP feature isn't that import for the OpenSNMPd because it does not cost that much time to reload the information out of the kernel and the daemon don't keep stat from it's clients. In the case of the OpenOSPF its much more important because it take a while to restart. And during the starting time the router would not be available. As a frequent user of SNMP, and OpenBSD's snmpd in particular, I could see the HUP feature being immensely useful. Restarting the snmpd daemon resets the timeticks value to zero, which appears as a "reboot" to the outsider. However, in lieu of graceful restart to pick up static changes in snmpd.conf, I could see perhaps changing timeticks to reference the host system uptime counter instead as an alternative, then restarting it would be less of an issue on my end. However, I'm sure there are legitimate reasons for things to be the way they are now, such as portability since everyone's uptime will be somewhere different. Something that's been on my list of things to look at, should I ever find my coder hat ever again (it's been missing somewhere for quite some time now). -- http://blog.sarlok.com/ Sometimes all the left hand needs to know is where the right hand is, so it knows where to point the blame.
Re: Minor improvement to socket(2) man page
Hi, Michael McConville wrote on Wed, Dec 21, 2016 at 08:17:20PM -0500: > Before I do the busy-work: do the man page gurus have a preferred > phrasing? I prefer fhopen(2)'s (listed above), which seems to be the > most common. The preferred version is something close to what .Rv produces: schwarze@isnote $ echo ".Rv\n.Pp\n.Rv foo" | mandoc -mdoc Upon successful completion, the value 0 is returned; otherwise the value -1 is returned and the global variable errno is set to indicate the error. The foo() function returns the value 0 if successful; otherwise the value -1 is returned and the global variable errno is set to indicate the error. That gives, for a page with one function, or if all functions have the same return values, taking accept(2) as an example: Upon successful completion, a non-negative integer representing the file descriptor for the accepted socket is returned; otherwise the value -1 is returned and the global variable .Va errno is set to indicate the error. I think fhopen(3) can remain as it is, even though it differs in a few very small details. Yours, Ingo
n_time in trpt(8)
In 2014, mpi@ substituted n_time, n_long, and n_short with their equivalent u_int_* types throughout the network stack to remove the dependency on : http://marc.info/?l=openbsd-tech&m=140523875001860&w=2 As mentioned in his mail, trpt(8) is the only program in userland that uses n_time. The following diff does the final cleanup for trpt(8). ok? Index: trpt.c === RCS file: /cvs/src/usr.sbin/trpt/trpt.c,v retrieving revision 1.33 diff -u -p -r1.33 trpt.c --- trpt.c 27 Aug 2016 01:50:07 - 1.33 +++ trpt.c 1 Dec 2016 19:17:38 - @@ -73,7 +73,6 @@ #include #include -#include #include #include #include @@ -112,7 +111,7 @@ int tcp_debx; struct tcp_debug tcp_debug[TCP_NDEBUG]; static caddr_t tcp_pcbs[TCP_NDEBUG]; -static n_time ntime; +static u_int32_t ntime; static int aflag, follow, sflag, tflag; extern char *__progname;
Re: n_time in trpt(8)
On Wed, Dec 21, 2016 at 10:01:28PM -0500, Lawrence Teo wrote: > In 2014, mpi@ substituted n_time, n_long, and n_short with their equivalent > u_int_* types throughout the network stack to remove the dependency on > : > > http://marc.info/?l=openbsd-tech&m=140523875001860&w=2 > > As mentioned in his mail, trpt(8) is the only program in userland that uses > n_time. The following diff does the final cleanup for trpt(8). > > ok? OK stsp@ I believe we prefer uint32_t over u_int32_t in new code. I'm fine with either form in this case. > > > Index: trpt.c > === > RCS file: /cvs/src/usr.sbin/trpt/trpt.c,v > retrieving revision 1.33 > diff -u -p -r1.33 trpt.c > --- trpt.c27 Aug 2016 01:50:07 - 1.33 > +++ trpt.c1 Dec 2016 19:17:38 - > @@ -73,7 +73,6 @@ > #include > > #include > -#include > #include > #include > #include > @@ -112,7 +111,7 @@ int tcp_debx; > struct tcp_debug tcp_debug[TCP_NDEBUG]; > > static caddr_t tcp_pcbs[TCP_NDEBUG]; > -static n_time ntime; > +static u_int32_t ntime; > static int aflag, follow, sflag, tflag; > > extern char *__progname; >