Re: [ksh] [patch] Make "$@" POSIX-compliant with empty IFS
On Fri, Mar 04, 2016 at 11:29:38AM +0100, Dmitrij D. Czarkoff wrote: > Martijn Dekker said: > > So this patch makes quoted "$@" act according to the standard even when > > IFS is empty. Quoted "$*" is unchanged. For the unspecified (not > > standardised) cases of unquoted $@ and $*, this patch makes ksh act like > > AT&T ksh93, bash, zsh and (d)ash, which seems safest from a > > compatibility point of view. > > This makes sense to me, and changes seem reasonable. I've re-generated > diff against -current. Anyone willing to OK it? I think this is ok. However, since this patch is rather subtle, it would be nice if someone more experienced could look this over as well. Note that this appears to have been in the snapshots for a while. > @@ -420,6 +446,7 @@ expand(char *cp, /* input word */ > if (f&DOBLANK) > doblank++; > st = st->prev; > + word = quote || (!*x.str) ? IFS_WORD : > IFS_IWS; This line would need some wrapping. While I'm here: the parens are a bit strange. Since the precedence here is not completely obvious anyway, I think they are more confusing than helpful. Why not word = (quote || !*x.str) ? IFS_WORD : IFS_IWS; or even if (quote || !*x.str) word = IFS_WORD; else word = IFS_IWS;
Re: Scheduler hack for multi-threaded processes
> > Even if one day we fix the "browsers problem" in the thread > > library, I think this diff will remain necessary to handle > > processes calling sched_yield() in a loop, i.e. processes that are > > spinning. > > > Complete and utter hack: penalize processes or threads on > #system calls/time period. Syscalls are more expensive than > normal execution because of locking which blocks other > processes. Reset the counter when process transitions from > inactive->active. Yeah. punish processes when they call the code most likely to be efficient (that is -- kernel code). Thanks for the advice, but you don't have a clue.
[patch] www errata59.html - validation pass id rNNN - worthy?
Tue, 15 Mar 2016 07:10:12 -0600 (MDT) Theo de Raadt
> CVSROOT: /cvs
> Module name: www
> Changes by: dera...@cvs.openbsd.org 2016/03/15 07:10:12
>
> Modified files:
> . : errata57.html errata58.html errata59.html
>
> Log message:
> release new errata
A very insignificant patch proposal, and a question on applying
the same minor change to other two errata pages in maintenance.
Please see 2 validation reports for the errata59.html page here:
[http://validator.w3.org/check?uri=http://www.openbsd.org/errata59.html]
[http://www.htmlhelp.org/cgi-bin/validate.cgi?url=http://www.openbsd.org/errata59.html]
Similar to the previous patch proposal for faq/current.html:
[http://marc.info/?l=openbsd-tech&m=145865833130449&w=4]
While errata pages are much higher importance, copy to Theo de Raadt.
Below is a single patch [1] to change the id to start with a letter
"r" (errata, revision, record), instead of a digit like this sample:
This allows the document to validate correct, and if you approve, will
follow similar pattern change for the relevant errata5{7,8}.html too.
Please advise if this then should be followed with a patch for the old
historic errata pages, although I would not insist on it frankly.
[1] Here is the proposed patch, hope this applies without mangling:
$ diff -u errata59.html{-1.3,}
--- errata59.html-1.3 Thu Mar 24 00:22:33 2016
+++ errata59.html Thu Mar 24 00:27:34 2016
@@ -83,7 +83,7 @@
-
+
001: SECURITY FIX: March 10, 2016
All architectures
http://www.openssh.com/txt/x11fwd.adv";>
@@ -96,7 +96,7 @@
A source code patch exists which remedies this problem.
-
+
002: SECURITY FIX: March 16, 2016
All architectures
Insufficient checks in IPv6 socket binding and UDP IPv6 option
@@ -107,7 +107,7 @@
A source code patch exists which remedies this problem.
-
+
003: RELIABILITY FIX: March 16,
2016
All architectures
Incorrect path processing in pledge_namei() could result in unexpected
Re: Scheduler hack for multi-threaded processes
On 03/23/2016 18:58, Alexandre Ratchov wrote: On Wed, Mar 23, 2016 at 09:35:50PM +0100, Mark Kettenis wrote: This doesn't only change the sched_yield() behaviour, but also modifies how in-kernel yield() calls behave for threaded processes. That is probably not right. So here is a diff that keeps yield() the same and adds the code in the sched_yield(2) implementation instead. It also changes the logic that picks the priority to walk the complete threads listand pick the highest priotity of all the threads. That should be enough to make sure the calling thread is scheduled behind all other threads from the same process. That does require us to grab the kernel lock though. So this removes NOLOCK from sched_yield(2). I don't think that is a big issue. I used to think that this is a hack, but now i think this is necessary. Even if one day we fix the "browsers problem" in the thread library, I think this diff will remain necessary to handle processes calling sched_yield() in a loop, i.e. processes that are spinning. Complete and utter hack: penalize processes or threads on #system calls/time period. Syscalls are more expensive than normal execution because of locking which blocks other processes. Reset the counter when process transitions from inactive->active. I know that some systems have implemented this type of input to scheduling according to use of intangible resources. Geoff Steckel
Re: Scheduler hack for multi-threaded processes
On Wed, Mar 23, 2016 at 09:35:50PM +0100, Mark Kettenis wrote: > > > > This doesn't only change the sched_yield() behaviour, but also > > modifies how in-kernel yield() calls behave for threaded processes. > > That is probably not right. > > So here is a diff that keeps yield() the same and adds the code in the > sched_yield(2) implementation instead. It also changes the logic that > picks the priority to walk the complete threads listand pick the > highest priotity of all the threads. That should be enough to make > sure the calling thread is scheduled behind all other threads from the > same process. That does require us to grab the kernel lock though. > So this removes NOLOCK from sched_yield(2). I don't think that is a > big issue. I used to think that this is a hack, but now i think this is necessary. Even if one day we fix the "browsers problem" in the thread library, I think this diff will remain necessary to handle processes calling sched_yield() in a loop, i.e. processes that are spinning.
Re: Scheduler hack for multi-threaded processes
On Wed, Mar 23, 2016 at 06:18:59PM -0400, Ted Unangst wrote: > Mark Kettenis wrote: > > So here is a diff that keeps yield() the same and adds the code in the > > sched_yield(2) implementation instead. It also changes the logic that > > picks the priority to walk the complete threads listand pick the > > highest priotity of all the threads. That should be enough to make > > sure the calling thread is scheduled behind all other threads from the > > same process. That does require us to grab the kernel lock though. > > So this removes NOLOCK from sched_yield(2). I don't think that is a > > big issue. > > this does sound a little better. in theory we should be gifting priority to > the thread with the lock, but if we don't know who... more of this lock > business needs to go kernel side i suspect. still, maybe we can do the > opposite > and raise other threads to this thread's priority? AFAICS, raised priority is for interactive (aka io-bound) threads. Processes spinning (here by calling sched_yield()) desserve lower priority, as do any cpu-bound thread.
Re: Scheduler hack for multi-threaded processes
Mark Kettenis wrote: > So here is a diff that keeps yield() the same and adds the code in the > sched_yield(2) implementation instead. It also changes the logic that > picks the priority to walk the complete threads listand pick the > highest priotity of all the threads. That should be enough to make > sure the calling thread is scheduled behind all other threads from the > same process. That does require us to grab the kernel lock though. > So this removes NOLOCK from sched_yield(2). I don't think that is a > big issue. this does sound a little better. in theory we should be gifting priority to the thread with the lock, but if we don't know who... more of this lock business needs to go kernel side i suspect. still, maybe we can do the opposite and raise other threads to this thread's priority?
Re: Scheduler hack for multi-threaded processes
On 23/03/16(Wed) 21:35, Mark Kettenis wrote:
> > Date: Mon, 21 Mar 2016 16:51:16 +0100 (CET)
> > From: Mark Kettenis
> >
> > > Date: Sat, 19 Mar 2016 13:53:07 +0100
> > > From: Martin Pieuchot
> > >
> > > Applications using multiple threads often call sched_yield(2) to
> > > indicate that one of the threads cannot make any progress because
> > > it is waiting for a resource held by another one.
> > >
> > > One example of this scenario is the _spinlock() implementation of
> > > our librthread. But if you look on https://codesearch.debian.net
> > > you can find much more use cases, notably MySQL, PostgreSQL, JDK,
> > > libreoffice, etc.
> > >
> > > Now the problem with our current scheduler is that the priority of
> > > a thread decreases when it is the "curproc" of a CPU. So the threads
> > > that don't run and sched_yield(2) end up having a higher priority than
> > > the thread holding the resource. Which means that it's really hard for
> > > such multi-threaded applications to make progress, resulting in a lot of
> > > IPIs numbers.
> > > That'd also explain why if you have a more CPUs, let's say 4 instead
> > > of 2, your application will more likely make some progress and you'll
> > > see less sluttering/freezing.
> > >
> > > So what the diff below does is that it penalizes the threads from
> > > multi-threaded applications such that progress can be made. It is
> > > inspired from the recent scheduler work done by Michal Mazurek on
> > > tech@.
> > >
> > > I experimented with various values for "p_priority" and this one is
> > > the one that generates fewer # IPIs when watching a HD video on firefox.
> > > Because yes, with this diff, now I can.
> > >
> > > I'd like to know if dereferencing ``p_p'' is safe without holding the
> > > KERNEL_LOCK.
> > >
> > > I'm also interested in hearing from more people using multi-threaded
> > > applications.
> >
> > This doesn't only change the sched_yield() behaviour, but also
> > modifies how in-kernel yield() calls behave for threaded processes.
> > That is probably not right.
>
> So here is a diff that keeps yield() the same and adds the code in the
> sched_yield(2) implementation instead.
I'm not a big fan of code duplication, what about checking for
p_usrpri >= PUSER instead?
> It also changes the logic that
> picks the priority to walk the complete threads listand pick the
> highest priotity of all the threads. That should be enough to make
> sure the calling thread is scheduled behind all other threads from the
> same process. That does require us to grab the kernel lock though.
> So this removes NOLOCK from sched_yield(2). I don't think that is a
> big issue.
>
> Still improves video playback in firefox on my x1.
This is another kind of hack :) It's certainly less intrusive but
I'm still not sure if we want that. Reducing the priority of a
thread to the worst priority of its siblings might still not be
enough.
Now I'd like to know how many times sched_yield() really triggers a
context switch for threaded programs with this version of the diff.
Without any diff I observed that only 20 to 25% of the sched_yield()
calls made by firefox result in a different thread being chosen by
mi_switch(). Somethings tell me that per-CPU runqueues are to blame
here.
> Index: syscalls.master
> ===
> RCS file: /cvs/src/sys/kern/syscalls.master,v
> retrieving revision 1.167
> diff -u -p -r1.167 syscalls.master
> --- syscalls.master 21 Mar 2016 22:41:29 - 1.167
> +++ syscalls.master 23 Mar 2016 20:33:45 -
> @@ -514,7 +514,7 @@
> #else
> 297 UNIMPL
> #endif
> -298 STD NOLOCK { int sys_sched_yield(void); }
> +298 STD { int sys_sched_yield(void); }
> 299 STD NOLOCK { pid_t sys_getthrid(void); }
> 300 OBSOL t32___thrsleep
> 301 STD { int sys___thrwakeup(const volatile void *ident, \
> Index: kern_synch.c
> ===
> RCS file: /cvs/src/sys/kern/kern_synch.c,v
> retrieving revision 1.129
> diff -u -p -r1.129 kern_synch.c
> --- kern_synch.c 9 Mar 2016 13:38:50 - 1.129
> +++ kern_synch.c 23 Mar 2016 20:33:45 -
> @@ -1,4 +1,4 @@
> -/* $OpenBSD: kern_synch.c,v 1.129 2016/03/09 13:38:50 mpi Exp $*/
> +/* $openbsd: kern_synch.c,v 1.129 2016/03/09 13:38:50 mpi Exp $*/
> /* $NetBSD: kern_synch.c,v 1.37 1996/04/22 01:38:37 christos Exp $ */
>
> /*
> @@ -432,7 +432,24 @@ wakeup(const volatile void *chan)
> int
> sys_sched_yield(struct proc *p, void *v, register_t *retval)
> {
> - yield();
> + struct proc *q;
> + int s;
> +
> + SCHED_LOCK(s);
> + /*
> + * If one of the threads of a multi-threaded process called
> + * sched_yield(2), drop its priority to ensure its siblings
> + * can make some progress.
> + */
> + p->p_priority = p->p_usrpri;
> +
Re: Scheduler hack for multi-threaded processes
> Date: Mon, 21 Mar 2016 16:51:16 +0100 (CET)
> From: Mark Kettenis
>
> > Date: Sat, 19 Mar 2016 13:53:07 +0100
> > From: Martin Pieuchot
> >
> > Applications using multiple threads often call sched_yield(2) to
> > indicate that one of the threads cannot make any progress because
> > it is waiting for a resource held by another one.
> >
> > One example of this scenario is the _spinlock() implementation of
> > our librthread. But if you look on https://codesearch.debian.net
> > you can find much more use cases, notably MySQL, PostgreSQL, JDK,
> > libreoffice, etc.
> >
> > Now the problem with our current scheduler is that the priority of
> > a thread decreases when it is the "curproc" of a CPU. So the threads
> > that don't run and sched_yield(2) end up having a higher priority than
> > the thread holding the resource. Which means that it's really hard for
> > such multi-threaded applications to make progress, resulting in a lot of
> > IPIs numbers.
> > That'd also explain why if you have a more CPUs, let's say 4 instead
> > of 2, your application will more likely make some progress and you'll
> > see less sluttering/freezing.
> >
> > So what the diff below does is that it penalizes the threads from
> > multi-threaded applications such that progress can be made. It is
> > inspired from the recent scheduler work done by Michal Mazurek on
> > tech@.
> >
> > I experimented with various values for "p_priority" and this one is
> > the one that generates fewer # IPIs when watching a HD video on firefox.
> > Because yes, with this diff, now I can.
> >
> > I'd like to know if dereferencing ``p_p'' is safe without holding the
> > KERNEL_LOCK.
> >
> > I'm also interested in hearing from more people using multi-threaded
> > applications.
>
> This doesn't only change the sched_yield() behaviour, but also
> modifies how in-kernel yield() calls behave for threaded processes.
> That is probably not right.
So here is a diff that keeps yield() the same and adds the code in the
sched_yield(2) implementation instead. It also changes the logic that
picks the priority to walk the complete threads listand pick the
highest priotity of all the threads. That should be enough to make
sure the calling thread is scheduled behind all other threads from the
same process. That does require us to grab the kernel lock though.
So this removes NOLOCK from sched_yield(2). I don't think that is a
big issue.
Still improves video playback in firefox on my x1.
Index: syscalls.master
===
RCS file: /cvs/src/sys/kern/syscalls.master,v
retrieving revision 1.167
diff -u -p -r1.167 syscalls.master
--- syscalls.master 21 Mar 2016 22:41:29 - 1.167
+++ syscalls.master 23 Mar 2016 20:33:45 -
@@ -514,7 +514,7 @@
#else
297UNIMPL
#endif
-298STD NOLOCK { int sys_sched_yield(void); }
+298STD { int sys_sched_yield(void); }
299STD NOLOCK { pid_t sys_getthrid(void); }
300OBSOL t32___thrsleep
301STD { int sys___thrwakeup(const volatile void *ident, \
Index: kern_synch.c
===
RCS file: /cvs/src/sys/kern/kern_synch.c,v
retrieving revision 1.129
diff -u -p -r1.129 kern_synch.c
--- kern_synch.c9 Mar 2016 13:38:50 - 1.129
+++ kern_synch.c23 Mar 2016 20:33:45 -
@@ -1,4 +1,4 @@
-/* $OpenBSD: kern_synch.c,v 1.129 2016/03/09 13:38:50 mpi Exp $*/
+/* $openbsd: kern_synch.c,v 1.129 2016/03/09 13:38:50 mpi Exp $*/
/* $NetBSD: kern_synch.c,v 1.37 1996/04/22 01:38:37 christos Exp $ */
/*
@@ -432,7 +432,24 @@ wakeup(const volatile void *chan)
int
sys_sched_yield(struct proc *p, void *v, register_t *retval)
{
- yield();
+ struct proc *q;
+ int s;
+
+ SCHED_LOCK(s);
+ /*
+* If one of the threads of a multi-threaded process called
+* sched_yield(2), drop its priority to ensure its siblings
+* can make some progress.
+*/
+ p->p_priority = p->p_usrpri;
+ TAILQ_FOREACH(q, &p->p_p->ps_threads, p_thr_link)
+ p->p_priority = max(p->p_priority, q->p_priority);
+ p->p_stat = SRUN;
+ setrunqueue(p);
+ p->p_ru.ru_nvcsw++;
+ mi_switch();
+ SCHED_UNLOCK(s);
+
return (0);
}
Re: sdhc: don't always compile the pci attachment driver
> Date: Fri, 4 Mar 2016 22:32:19 +0100 > From: Patrick Wildt > > Hi, > > if you attach sdhc, you automatically compile the pci attachment driver, > even if there's no "sdhc* at pci?" in the config. This might fail on > architectures that don't have pci compiled in. > > If I'm not completely mistaken, this can be averted by making the file > depend on sdhc_pci. Tested to still compile correctly on amd64. ok kettenis@ > Patrick > > diff --git sys/dev/pci/files.pci sys/dev/pci/files.pci > index 0614e56..c5d0494 100644 > --- sys/dev/pci/files.pci > +++ sys/dev/pci/files.pci > @@ -766,7 +766,7 @@ file dev/pci/nviic.c nviic > > # SD Host Controller > attach sdhc at pci with sdhc_pci > -file dev/pci/sdhc_pci.c sdhc > +file dev/pci/sdhc_pci.c sdhc_pci > > # AMD NPT Family 0Fh Processors, Function 3 -- Miscellaneous Control > device kate > >
use fast lookup in in6_pcbconnect()
The current use of in_pcblookup() in in6_pcbconnect() is suboptimal :
all of the addresses and ports are defined, we are only interested in
exact matches, and its v4 cousin in_pcbconnect() already uses
in_pcbhashlookup().
Ok ?
Index: sys/netinet6/in6_pcb.c
===
RCS file: /cvs/src/sys/netinet6/in6_pcb.c,v
retrieving revision 1.89
diff -u -p -r1.89 in6_pcb.c
--- sys/netinet6/in6_pcb.c 23 Mar 2016 15:50:36 - 1.89
+++ sys/netinet6/in6_pcb.c 23 Mar 2016 17:09:11 -
@@ -304,9 +304,9 @@ in6_pcbconnect(struct inpcb *inp, struct
inp->inp_ipv6.ip6_hlim = (u_int8_t)in6_selecthlim(inp);
- if (in_pcblookup(inp->inp_table, &sin6->sin6_addr, sin6->sin6_port,
+ if (in6_pcbhashlookup(inp->inp_table, &sin6->sin6_addr, sin6->sin6_port,
IN6_IS_ADDR_UNSPECIFIED(&inp->inp_laddr6) ? in6a : &inp->inp_laddr6,
- inp->inp_lport, INPLOOKUP_IPV6, inp->inp_rtableid)) {
+ inp->inp_lport, inp->inp_rtableid)) {
return (EADDRINUSE);
}
uvm: remove unused(?) amap_extend
amap_extend is called when merging two adjacent areas of virtual address
space. However, merging is done only for kernel
virtual address space. It's not done for user space:
uvm/uvm_map.c:1359
/*
* Try to merge entry.
*
* Userland allocations are kept separated most of the time.
* Forego the effort of merging what most of the time can't be merged
* and only try the merge if it concerns a kernel entry.
*/
if ((flags & UVM_FLAG_NOMERGE) == 0 &&
(map->flags & VM_MAP_ISVMSPACE) == 0)
uvm_mapent_tryjoin(map, entry, &dead);
As far as I can tell, kernel vm_map_entries do not use amaps. So
amap_extend should never be called. Can we remove it?
Index: uvm/uvm_amap.c
===
RCS file: /cvs/src/sys/uvm/uvm_amap.c,v
retrieving revision 1.62
diff -u -p -r1.62 uvm_amap.c
--- uvm/uvm_amap.c 16 Mar 2016 16:53:43 - 1.62
+++ uvm/uvm_amap.c 23 Mar 2016 17:03:53 -
@@ -279,174 +279,6 @@ amap_free(struct vm_amap *amap)
}
/*
- * amap_extend: extend the size of an amap (if needed)
- *
- * => called from uvm_map when we want to extend an amap to cover
- *a new mapping (rather than allocate a new one)
- * => to safely extend an amap it should have a reference count of
- *one (thus it can't be shared)
- * => XXXCDC: support padding at this level?
- */
-int
-amap_extend(struct vm_map_entry *entry, vsize_t addsize)
-{
- struct vm_amap *amap = entry->aref.ar_amap;
- int slotoff = entry->aref.ar_pageoff;
- int slotmapped, slotadd, slotneed, slotalloc;
-#ifdef UVM_AMAP_PPREF
- int *newppref, *oldppref;
-#endif
- u_int *newsl, *newbck, *oldsl, *oldbck;
- struct vm_anon **newover, **oldover;
- int slotadded;
-
- /*
-* first, determine how many slots we need in the amap. don't
-* forget that ar_pageoff could be non-zero: this means that
-* there are some unused slots before us in the amap.
-*/
- AMAP_B2SLOT(slotmapped, entry->end - entry->start); /* slots mapped */
- AMAP_B2SLOT(slotadd, addsize); /* slots to add */
- slotneed = slotoff + slotmapped + slotadd;
-
- /*
-* case 1: we already have enough slots in the map and thus
-* only need to bump the reference counts on the slots we are
-* adding.
-*/
- if (amap->am_nslot >= slotneed) {
-#ifdef UVM_AMAP_PPREF
- if (amap->am_ppref && amap->am_ppref != PPREF_NONE) {
- amap_pp_adjref(amap, slotoff + slotmapped, slotadd, 1);
- }
-#endif
- return (0);
- }
-
- /*
-* case 2: we pre-allocated slots for use and we just need to
-* bump nslot up to take account for these slots.
-*/
- if (amap->am_maxslot >= slotneed) {
-#ifdef UVM_AMAP_PPREF
- if (amap->am_ppref && amap->am_ppref != PPREF_NONE) {
- if ((slotoff + slotmapped) < amap->am_nslot)
- amap_pp_adjref(amap, slotoff + slotmapped,
- (amap->am_nslot - (slotoff + slotmapped)),
- 1);
- pp_setreflen(amap->am_ppref, amap->am_nslot, 1,
- slotneed - amap->am_nslot);
- }
-#endif
- amap->am_nslot = slotneed;
- /*
-* no need to zero am_anon since that was done at
-* alloc time and we never shrink an allocation.
-*/
- return (0);
- }
-
- /*
-* case 3: we need to malloc a new amap and copy all the amap
-* data over from old amap to the new one.
-*
-* XXXCDC: could we take advantage of a kernel realloc()?
-*/
- if (slotneed >= UVM_AMAP_LARGE)
- return E2BIG;
-
- if (slotneed > UVM_AMAP_CHUNK)
- slotalloc = malloc_roundup(slotneed * MALLOC_SLOT_UNIT) /
- MALLOC_SLOT_UNIT;
- else
- slotalloc = slotneed;
-
-#ifdef UVM_AMAP_PPREF
- newppref = NULL;
- if (amap->am_ppref && amap->am_ppref != PPREF_NONE) {
- newppref = mallocarray(slotalloc, sizeof(int), M_UVMAMAP,
- M_WAITOK | M_CANFAIL);
- if (newppref == NULL) {
- /* give up if malloc fails */
- free(amap->am_ppref, M_UVMAMAP, 0);
- amap->am_ppref = PPREF_NONE;
- }
- }
-#endif
- if (slotneed > UVM_AMAP_CHUNK)
- newsl = malloc(slotalloc * MALLOC_SLOT_UNIT, M_UVMAMAP,
- M_WAITOK | M_CANFAIL);
- else
- newsl = pool_get(&uvm_amap_slot_pools[slotalloc - 1],
- PR_WAITOK | PR_LIMITFAIL);
- if (newsl == NULL) {
-#ifdef UVM_AMAP_PPREF
Re: Fix overflow check in sys/kern/kern_time.c
On Wed, 23 Mar 2016 10:58:42 -0400, Michael McConville wrote: > I'm not sure whether avoiding incrementing here is an ideal move, but > this diff definitely works toward a local optimum. Namely, that error > check is technically meaningless because signed overflow is undefined. > > ok? Or would people prefer a solution that's robust to changing > *curpps's type? Is there any reason for the counters to be signed? In general I don't think it makes much sense for counters to be signed. - todd
Re: Fix overflow check in sys/kern/kern_time.c
Mark Kettenis wrote: > > I'm not sure whether avoiding incrementing here is an ideal move, but > > this diff definitely works toward a local optimum. Namely, that error > > check is technically meaningless because signed overflow is undefined. > > Within the kernel, signed overflow actually is well-defined. We don't > run on hypethetical one's complement machines (or machines that use > some other weird scheme to encode signed integers) and we pass flags > to the compiler that make it do the right thing. > > Yes, the fact that the C standard doesn't want to make assumptions > about the encoding of signed inttegers is the real reason why the > standard leaves integer overflow undefined. Unfortunately that has > lead the language lwayer types in charge of compilers to implement > misguided optimizations that break stuff. I agree that it's unfortunate that signed overflow is undefined. I think ignoring that fact is a long-term problem, though. Presumably, we're eventually going to switch compilers, and that could silently break things. In particular, the flags used for saner signed overflow behavior are often poorly maintained or broken. For example, -ftrapv is completely broken (no effect) in both the base and port GCC. If we decide to depend on signed overflow, we restrict ourselves to compilers implementing our non-standard dialect of C, and we rely on these somewhat uncommonly used flags. > > ok? Or would people prefer a solution that's robust to changing > > *curpps's type? > > I you absolutely want to "fix" this issue, I'd prefer that you just > fixed the check and left the comment in place. Then at least the code > continues to document the corner case. So simply: > > > - if (*curpps + 1 > *curpps) > > + if (*curpps != INT_MAX) > > Of course that breaks if you start out with a negative number... Can you explain? I don't understand. > So I think I'd prefer if you simply left this alone.
Re: Fix overflow check in sys/kern/kern_time.c
> Date: Wed, 23 Mar 2016 10:58:42 -0400 > From: Michael McConville > > I'm not sure whether avoiding incrementing here is an ideal move, but > this diff definitely works toward a local optimum. Namely, that error > check is technically meaningless because signed overflow is undefined. Within the kernel, signed overflow actually is well-defined. We don't run on hypethetical one's complement machines (or machines that use some other weird scheme to encode signed integers) and we pass flags to the compiler that make it do the right thing. Yes, the fact that the C standard doesn't want to make assumptions about the encoding of signed inttegers is the real reason why the standard leaves integer overflow undefined. Unfortunately that has lead the language lwayer types in charge of compilers to implement misguided optimizations that break stuff. > ok? Or would people prefer a solution that's robust to changing > *curpps's type? I you absolutely want to "fix" this issue, I'd prefer that you just fixed the check and left the comment in place. Then at least the code continues to document the corner case. So simply: > - if (*curpps + 1 > *curpps) > + if (*curpps != INT_MAX) Of course that breaks if you start out with a negative number... So I think I'd prefer if you simply left this alone. Cheers, Mark > Index: sys/kern/kern_time.c > === > RCS file: /cvs/src/sys/kern/kern_time.c,v > retrieving revision 1.96 > diff -u -p -r1.96 kern_time.c > --- sys/kern/kern_time.c 5 Dec 2015 10:11:53 - 1.96 > +++ sys/kern/kern_time.c 10 Feb 2016 05:25:35 - > @@ -765,20 +765,8 @@ ppsratecheck(struct timeval *lasttime, i > else > rv = 0; > > -#if 1 /*DIAGNOSTIC?*/ > - /* be careful about wrap-around */ > - if (*curpps + 1 > *curpps) > - *curpps = *curpps + 1; > -#else > - /* > - * assume that there's not too many calls to this function. > - * not sure if the assumption holds, as it depends on *caller's* > - * behavior, not the behavior of this function. > - * IMHO it is wrong to make assumption on the caller's behavior, > - * so the above #if is #if 1, not #ifdef DIAGNOSTIC. > - */ > - *curpps = *curpps + 1; > -#endif > + if (*curpps != INT_MAX) > + (*curpps)++; > > return (rv); > } > >
Fix overflow check in sys/kern/kern_time.c
I'm not sure whether avoiding incrementing here is an ideal move, but this diff definitely works toward a local optimum. Namely, that error check is technically meaningless because signed overflow is undefined. ok? Or would people prefer a solution that's robust to changing *curpps's type? Index: sys/kern/kern_time.c === RCS file: /cvs/src/sys/kern/kern_time.c,v retrieving revision 1.96 diff -u -p -r1.96 kern_time.c --- sys/kern/kern_time.c5 Dec 2015 10:11:53 - 1.96 +++ sys/kern/kern_time.c10 Feb 2016 05:25:35 - @@ -765,20 +765,8 @@ ppsratecheck(struct timeval *lasttime, i else rv = 0; -#if 1 /*DIAGNOSTIC?*/ - /* be careful about wrap-around */ - if (*curpps + 1 > *curpps) - *curpps = *curpps + 1; -#else - /* -* assume that there's not too many calls to this function. -* not sure if the assumption holds, as it depends on *caller's* -* behavior, not the behavior of this function. -* IMHO it is wrong to make assumption on the caller's behavior, -* so the above #if is #if 1, not #ifdef DIAGNOSTIC. -*/ - *curpps = *curpps + 1; -#endif + if (*curpps != INT_MAX) + (*curpps)++; return (rv); }
Re: Proxy ARP and published entry
On Mon, Mar 21, 2016 at 02:29:46PM +0100, Martin Pieuchot wrote:
> When the caller of arplookup() asked for a proxy'd ARP entry, make
> sure the entry returned by rtalloc(9) is indeed "published".
>
> This is currently always true for ARP entries added with arp(8) but
> it is not the case if you add your own entry with the 33rd bit set
> but without setting RTF_ANNOUNCE.
>
> It is also not the case with ART.
>
> Since the 33rd bit trick is an implementation detail of the current
> routing table, which I'm doing differently with ART, let's check for
> the correct flag.
>
> ok?
OK bluhm@
>
> Index: netinet/if_ether.c
> ===
> RCS file: /cvs/src/sys/netinet/if_ether.c,v
> retrieving revision 1.202
> diff -u -p -r1.202 if_ether.c
> --- netinet/if_ether.c7 Mar 2016 11:00:36 - 1.202
> +++ netinet/if_ether.c21 Mar 2016 13:22:33 -
> @@ -696,6 +696,12 @@ arplookup(u_int32_t addr, int create, in
> rtfree(rt);
> return (NULL);
> }
> +
> + if (proxy && !ISSET(rt->rt_flags, RTF_ANNOUNCE)) {
> + rtfree(rt);
> + return (NULL);
> + }
> +
> return (rt);
> }
>
Re: www.openbsd.org/cgi-bin/man.cgi
Wed, 23 Mar 2016 10:07:10 + skin...@britvault.co.uk (Craig Skinner) > On 2016-03-22 Tue 22:49 PM |, Bob Beck wrote: > > > > A few years back, Ingo moved it to the new mandoc based man.cgi, and > > now we've actually moved this to a dedicated place - "man.openbsd.org" > > Superb. What's next? You could fix your bookmarks and search URLs for online manuals, i.e. [[http://man.openbsd.org/][OpenBSD manual pages]] [search_url="om OpenBSD manuals http://man.openbsd.org/cgi-bin/man.cgi?query=%s";] Details for the search URL construct are in the "URI interface" section: man.cgi — CGI program to search and display manual pages [[http://man.openbsd.org/mandoc/man8/man.cgi.8][OpenBSD man.cgi]] Please test the web system at man.openbsd.org along normal local usage for reference. If you come up with a smart idea and/or find something worth contributing, please do so. For local use, you could obtain the OpenBSD release from the usual places: [[http://www.openbsd.org/][OpenBSD]] [[http://www.openbsd.org/orders.html][OpenBSD ordering]] After install according to your preferences please read the afterboot(8) manual page in the terminal. $ man afterboot Same page is available now online from the new man.openbsd.org server: afterboot - things to check after the first complete boot [[http://man.openbsd.org/OpenBSD-current/man8/afterboot.8][om afterboot]] To display and author manual pages please see here: mandoc - format and display UNIX manuals [[http://man.openbsd.org/OpenBSD-current/man1/mandoc.1][om mandoc]] mdoc - semantic markup language for formatting manual pages [[http://man.openbsd.org/OpenBSD-current/man7/mdoc.7][om mdoc]]
Re: www.openbsd.org/cgi-bin/man.cgi
On 2016-03-22 Tue 22:49 PM |, Bob Beck wrote: > > A few years back, Ingo moved it to the new mandoc based man.cgi, and > now we've actually moved this to a dedicated place - "man.openbsd.org" > Superb. What's next? $ ssh gu...@man.openbsd.org Welcome guest user to OpenBSD's online manual library. The only command available is 'man'. (For help; type 'man man[ENTER]'.) $
multi-pool malloc wip diff
Hi,
first diff that seems to work. Tested on amd64 and compile tested on
sparc64.
It is alo available at http://www.drijf.net/openbsd/malloc
Form the README:
The diff should be applied while in /usr/src/lib, it will patch
both librthreads as as well as libc.
THIS IS WORK IN PROGRESS. It contains multiple things that should
be improved. To name a few things:
- Curently fixed at 4 pools with a fixed thread -> pool mapping.
- All pools are always initialized, even for single threaded programs, where
only one pool is used.
- Especially realloc gets quite a bit uglier.
- I'm pondering storing the thread -> pool mapping in the thread
struct instead of computing it each time from the tcb address.
-Otto
Index: libc/include/thread_private.h
===
RCS file: /cvs/src/lib/libc/include/thread_private.h,v
retrieving revision 1.26
diff -u -p -r1.26 thread_private.h
--- libc/include/thread_private.h 7 Apr 2015 01:27:07 - 1.26
+++ libc/include/thread_private.h 21 Mar 2016 16:34:52 -
@@ -17,6 +17,8 @@
*/
extern int __isthreaded;
+#define _MALLOC_MUTEXES 4
+
/*
* Weak symbols are used in libc so that the thread library can
* efficiently wrap libc functions.
@@ -136,16 +138,16 @@ extern void *__THREAD_NAME(serv_mutex);
/*
* malloc lock/unlock prototypes and definitions
*/
-void _thread_malloc_lock(void);
-void _thread_malloc_unlock(void);
+void _thread_malloc_lock(int);
+void _thread_malloc_unlock(int);
-#define _MALLOC_LOCK() do {\
+#define _MALLOC_LOCK(n)do {
\
if (__isthreaded) \
- _thread_malloc_lock(); \
+ _thread_malloc_lock(n); \
} while (0)
-#define _MALLOC_UNLOCK() do {\
+#define _MALLOC_UNLOCK(n) do {\
if (__isthreaded) \
- _thread_malloc_unlock();\
+ _thread_malloc_unlock(n);\
} while (0)
void _thread_atexit_lock(void);
Index: libc/stdlib/malloc.c
===
RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.185
diff -u -p -r1.185 malloc.c
--- libc/stdlib/malloc.c17 Mar 2016 17:55:33 - 1.185
+++ libc/stdlib/malloc.c21 Mar 2016 16:34:52 -
@@ -1,6 +1,6 @@
/* $OpenBSD: malloc.c,v 1.185 2016/03/17 17:55:33 mmcc Exp $ */
/*
- * Copyright (c) 2008, 2010, 2011 Otto Moerbeek
+ * Copyright (c) 2008, 2010, 2011, 2016 Otto Moerbeek
* Copyright (c) 2012 Matthew Dempsky
* Copyright (c) 2008 Damien Miller
* Copyright (c) 2000 Poul-Henning Kamp
@@ -43,6 +43,7 @@
#endif
#include "thread_private.h"
+#include
#if defined(__sparc__) && !defined(__sparcv9__)
#define MALLOC_PAGESHIFT (13U)
@@ -95,10 +96,10 @@
#define _MALLOC_LEAVE(d) do { if (__isthreaded) { \
(d)->active--; \
- _MALLOC_UNLOCK(); } \
+ _MALLOC_UNLOCK(d->mutex); } \
} while (0)
#define _MALLOC_ENTER(d) do { if (__isthreaded) { \
- _MALLOC_LOCK(); \
+ _MALLOC_LOCK(d->mutex); \
(d)->active++; } \
} while (0)
@@ -129,6 +130,7 @@ struct dir_info {
void *delayed_chunks[MALLOC_DELAYED_CHUNK_MASK + 1];
size_t rbytesused; /* random bytes used */
char *func; /* current function */
+ int mutex;
u_char rbytes[32]; /* random bytes */
u_short chunk_start;
#ifdef MALLOC_STATS
@@ -178,7 +180,7 @@ struct chunk_info {
};
struct malloc_readonly {
- struct dir_info *malloc_pool; /* Main bookkeeping information */
+ struct dir_info *malloc_pool[_MALLOC_MUTEXES]; /* Main bookkeeping
information */
int malloc_freenow; /* Free quickly - disable chunk rnd */
int malloc_freeunmap; /* mprotect free pages PROT_NONE? */
int malloc_hint;/* call madvice on free pages? */
@@ -202,14 +204,13 @@ static union {
u_char _pad[MALLOC_PAGESIZE];
} malloc_readonly __attribute__((aligned(MALLOC_PAGESIZE)));
#define mopts malloc_readonly.mopts
-#define getpool() mopts.malloc_pool
char *malloc_options;/* compile-time options */
static u_char getrbyte(struct dir_info *d);
#ifdef MALLOC_STATS
-void malloc_dump(int);
+void malloc_dump(int, struct dir_info *);
PROTO_NORMAL(malloc_dump);
static void malloc_exit(void);
#define CALLER __builtin_return_address(0)
@@ -240,6 +241,18 @@ hash(void *p)
return sum;
}
+static inline
+struct dir_info *getpo
