- User Since
- Jul 28 2014, 7:43 PM (139 w, 3 d)
Wed, Mar 29
We are probably now at a commit candidate, if various reviewers wouldn't mind checking that they are happy with how the patch has ended up looking?
Restore higher stability level for DTrace probes, as otherwise the
DTrace command-line tool will reject use of the probes in its default
Address a number of reviewer comments from @jon, @markj for dtaudit.
Thanks for these reviews; will make various changes and update the patch soon.
Tue, Mar 28
Further updates to constant use in less(1) using a more recent LLVM.
Mon, Mar 27
Minor update: remove unneeded #include that snuck in.
Sun, Mar 26
FYI, I have now committed a man page for DPCPU(9) in r316003. It includes some (safe?) synchronisation patterns in its example code.
This is correct: you must make sure that you continue to access state on the CPU for which you acquired a mutex -- e.g., by caching a pointer to the per-CPU state you are accessing, in case migration takes place.
But that is racey. Preemption can in theory occur straight after I have verified that it hasn't. Looks like I need to use critical regions for now. I can live with that if you can?
Thu, Mar 23
Just a few quick comments:
Wed, Mar 15
How does this play out with non-native ABIs (e.g., the Linux emulator) -- I thought SYS_fork (etc) were ABI-specific system-call numbers?
Mon, Mar 13
It would be more tempting to add the systrace_probe_func invocation at the end of fork_return() where the similar KTRACE probe fires (for similar reasons). Take a look at the call to ktrsysret(SYS_fork, 0, 0); for details.
Sat, Mar 4
This seems like a sensible general change. I'm quite surprised it wasn't this way already (.. and sort of misremembered that it was -- IPSEC should always have been using the netisr...).
Feb 7 2017
I like the idea, and encourage you to proceed, but be aware that struct tcpcb is part of the user-visible ABI for monitoring tools (sigh). Someone should restock our supplies of padding someday.
Jan 28 2017
Jan 9 2017
Adding "-R" support is a good idea.
Jan 6 2017
I'm not sure if consumers of m_pulldown() make assumptions about writability or not. The man page doesn't mention that they should (or not) but this is more of an empirical question. As I recall, m_pulldown() is particularly popular in IPv6, so tagging Bjoern to perhaps take a look at this and see what he thinks.
Dec 7 2016
Nov 30 2016
Nov 22 2016
Nov 5 2016
Oct 20 2016
Mentor approval granted. (NB: not a technical review, but existing technical reviews here look good to go!)
Oct 7 2016
Oct 5 2016
Overall I like this approach, but there's an important experimental question as to whether this enables all the use cases we care about -- and, more generally, whether there are visible failure modes that might surprise application programmers. We also need to think quite hard to convince ourselves this maintains safe operation. Getting Jon Anderson, Ben Laurie, and David Drysdale to review the approach would be very useful.
Oct 3 2016
Oct 1 2016
Sep 30 2016
In general, this seems like a good idea. A bit of wordsmithing does help, and reviewing an updated commit candidate before it goes into the tree wouldn't hurt if you can tolerate another RTT with reviewers :-).
Sep 23 2016
I'm fine with exposing the hostname here -- the goal of Capsicum has always been to be pragmatic about getting software running where it doesn't violate isolation properties. You could argue that this is an information leak and/or might cause problems for deterministic replay-style applications of Capsicum -- but I'd rather we had more code working in a sandboxing. :-)
Sep 18 2016
High-level comments rather than a detailed code review:
Sep 13 2016
Should we also be ditching M_COPY() and/or switching it to M_COPYM() for consistency..?
Aug 29 2016
The comments could have to do with the au_qctrl structure, which uses "int", whereas the au_qctrl64 type uses "uint64_t". You can see the code handling the older structure a bit below this point, which likely has to do with compatibility with older Solaris/XNU versions rather than FreeBSD per se.
Aug 20 2016
Jul 28 2016
Looks good to me!
Jul 15 2016
I'd suggest avoiding any style changes in the initial copy of code to the new locations, so diffs can more easily be checked, and changes can be more easily merged. Apply style/comment/etc changes in a separate commit.
Jul 11 2016
I think using panic() here would be preferable to KASSERT().
Jul 10 2016
Jul 5 2016
Jun 23 2016
This change seems sensible to me, but I believe John has worked with this code most recently, so I've added him as a reviewer as well.
Jun 13 2016
May 20 2016
May 4 2016
This seems like a good idea, but fixing the style bugs (e.g., local variable definitions should be at the start of the function before any executable code) is necessary.
Apr 30 2016
Mar 13 2016
Mar 12 2016
CheriBSD change for reference: https://github.com/CTSRD-CHERI/cheribsd/commit/7ed51f1f4ff2ea4c7ba1cebc101d9dd6e26f3844
Mar 11 2016
Mar 6 2016
Jan 18 2016
Dec 30 2015
Seems generally sensible. Better documenting the locking protocol would be useful -- why various locks are required, and what the implications of the unlock flag are (they are more broad than suggested by the name). Addition of a flags argument that is conflated with a local flags field is confusing.
Oct 23 2015
On net.inet.tcp.experimental.initcwnd10: if it's not shipped in a release, we can remove it as a bit of interface stability in 11-CURRENT. If it's not a tunable, then it will generate a warning on boot if set in sysctl.conf after kernel support is removed, as the sysctl won't be present. If it's also a tunable, there won't be a warning, and it will silently fail to operate. So, hopefully it's not in a release? If it is, we probably need something in the release notes about its removal, etc.
Oct 14 2015
Just to quickly comment on sysctl naming: I don't like putting things like "nonstandard" or "experimental" in sysctl names, because what is standardised or experimental frequently changes. Sysctl names are effectively ABIs, since the names get put in loader.conf, sysctl.conf, etc, and if you change the sysctl name in the kernel, you may break those configuration lines. For sysctl.conf, you at least get an error, but if a loader tunable changes names, it just silently stops working. There are other ways to document whether something is standardised -- e.g., source-code commands and in the man page -- that are probably better. We already have a TCP man page documenting a number of sysctls (I think?) and really the information should just be there, with suitable caveats. So I'd vote (if I were asked to vote) for just naming the sysctl the most descriptive thing.
Oct 8 2015
Just to quickly chime in with a few high-level points:
Oct 6 2015
Should be fine for SDT, but I've not tried FBT, and as you point out, stack layout may differ a bit. I will try to give that a test run in isolation from the unupstreamed dtrace_invop_jump_addr change. We really do need to upstream the latter -- is that in your court?
Oct 4 2015
Updated diff from svn contains further context; no functional change.
Updated patch generated against Subversion, and contains more context. No functional change.