- User Since
- Oct 29 2015, 5:25 PM (151 w, 1 d)
Sat, Sep 8
Hi. I know I'm late to the party, but I have three comments:
a) I could be wrong, but I don't think there is any guarantee this won't be called simultaneously for two different groups at the same time. (The groups could be in different VNETs, for example.) In that case, two different invocations could be working on the function's static variables at the same time. That may produce unexpected results. (Granted, it would take an unusual series of events. But, I think we've all seen highly unusual events occur.)
b) I don't think the const variable also needs to be static.
c) It seems like the rate limiter should really be per-group, so I would suggest adding the lastprint variable to the inpcblbgroup struct.
Thu, Aug 23
FYI, something didn't look right doing my tests. So, I'm going to delay committing this until I can satisfy myself that it behaves correctly. That will almost certainly mean I miss the code freeze. C'est la vie!
- Limit the ipq structure to the kernel to eliminate a buildworld failure. (And, why should we make userspace code import the sys/queue.h header for a structure they don't need anyway?)
- Address @jhb's nit.
Aug 22 2018
Aug 21 2018
Aug 18 2018
LGTM (with minor change noted in-line).
Aug 14 2018
Aug 6 2018
Jul 28 2018
Jul 27 2018
Jul 25 2018
Jul 24 2018
Never mind. It looks like this is a display issue, and you really are deleting them.
It looks like you're keeping the code by moving it to the modules directory? At this point, I think it just makes sense to delete it.
Jun 18 2018
Jun 15 2018
I've spent some time thinking about this a bit, and I have the following comments. (Some may seem contradictory, but please bear with me. :-) )
Jun 14 2018
Jun 13 2018
Address @markj's feedback by always defining the vmd_kernel_rwx_arena member of the vm_domain struct.
Jun 12 2018
Update the zone(9) manpage.
Jun 11 2018
Okay, really without the cruft this time. (Hopefully...)
I'll try to look at this review this week.
Get the correct version of sys/vm/vm_kern.c (with the troubleshooting stuff removed).
- Address @alc's concerns by adding a new arena for allocations with non-standard permissions. Import a 2MB-aligned address block at a time into the arena. Release space back to the parent arena when able. But, only do this for architectures with superpages.
- Plumb M_EXEC through malloc(9).
- Fix BPF by reverting most of rS317072.
- Add a note to the manpage that not all architectures will enforce execution permissions.
This passes my "sniff test", but it would be better to get @pkelsey to review it.
I think there are more changes needed.
Jun 8 2018
The submitter spoke to me in person at BSDCan and answered my questions.
Jun 7 2018
Incorporate review feedback from @jhb.
I added a few basic comments while I ponder the rest...
Jun 1 2018
I don't know ISA well. I'm open to switching this to be an option to always panic on any NMI, instead of picking NMI_TIMER2 for special treatment.
May 31 2018
I'm sorry it took me so long to review this.
May 24 2018
May 21 2018
Having stared at this a bit, I generally think its fine. In fact, in some aspects it is an improvement over what it replaces.
May 18 2018
By the way, measurements were taken on an Intel E5-2697A v4 (32-core Broadwell).
May 16 2018
May 13 2018
Sorry it took me a while to look at this. See comments in-line.