Page MenuHomeFreeBSD

domagoj.stolfa_gmail.com (Domagoj Stolfa)
User

Projects

User Details

User Since
Sep 8 2016, 3:21 PM (148 w, 5 d)

Recent Activity

Aug 27 2018

domagoj.stolfa_gmail.com added a comment to D16921: Attempt to support multiple tracepoints with the same address..

I think it would be nicer to instead keep all tracepoints for a given address on a linked list, with a single hash table entry for all of them. fbt_invop() would invoke dtrace_probe() for each probe associated with the tracepoint, which should be fine so long as there aren't many. (With ifuncs there will be 2.)

Perhaps we could even bunch them into a bounded array (or a vector-like implementation) and check it at fbt load-time in order to avoid indirection? A linked list is unlikely to be too painful given all the membars and indirection happening in dynamic variable implementation -- but nonetheless would degrade performance :-).

I think it's acceptable for the time being since, as I pointed out, there are at most 2 probes for a given tracepoint currently. The FBT hash table is itself cache-unfriendly, and I'd rather fix the performance problems there holistically. Here's the distribution of hash chain lengths on a system with a fairly stripped-down kernel:

5395 0
9755 1
8748 2
5244 3
2369 4
 876 5
 280 6
  73 7
  24 8
   3 9
   1 10
Aug 27 2018, 6:28 PM
domagoj.stolfa_gmail.com added a comment to D16921: Attempt to support multiple tracepoints with the same address..

I think it would be nicer to instead keep all tracepoints for a given address on a linked list, with a single hash table entry for all of them. fbt_invop() would invoke dtrace_probe() for each probe associated with the tracepoint, which should be fine so long as there aren't many. (With ifuncs there will be 2.)

Aug 27 2018, 5:15 PM
domagoj.stolfa_gmail.com added a comment to D16921: Attempt to support multiple tracepoints with the same address..

Why "additional"? The problem exists regardless since fbt_invop() returns after the first matching tracepoint. The problem was invisible before ifuncs since before that all tracepoints had distinct addresses. This patch just hides the problem in the common case.

Aug 27 2018, 4:38 PM
domagoj.stolfa_gmail.com added a comment to D16921: Attempt to support multiple tracepoints with the same address..

I'm a little worried that this may cause additional confusion if someone expects multiple probes to fire at that point. I wonder if a better workaround would be to just add a warning message whenever DTrace is called, or perhaps when someone calls dtrace -l, in the man page or something along the lines until we fix? Is there a reason we can't fix this in 13 and MFC the changes?

Aug 27 2018, 4:25 PM

May 8 2018

domagoj.stolfa_gmail.com updated the diff for D15359: DTrace aarch64: Avoid calling unwind_frame() in the probe context.

Preemptively remove a redundant check for INKERNEL(fp).

May 8 2018, 9:23 PM · arm64, DTrace
domagoj.stolfa_gmail.com added a comment to D15359: DTrace aarch64: Avoid calling unwind_frame() in the probe context.

Why not just exclude it in fbt_provide_module_function? I'd prefer we keep the unwinding code together, maybe with a comment that it may be called from such a context.

May 8 2018, 9:22 PM · arm64, DTrace
domagoj.stolfa_gmail.com added a comment to D15359: DTrace aarch64: Avoid calling unwind_frame() in the probe context.

however, in the future a patch that enabled instrumentation of inlined functions could accidentally break DTrace on aarch64.

Was the problem found while working on such a patch? :)

May 8 2018, 8:55 PM · arm64, DTrace
domagoj.stolfa_gmail.com updated the summary of D15359: DTrace aarch64: Avoid calling unwind_frame() in the probe context.
May 8 2018, 8:51 PM · arm64, DTrace
domagoj.stolfa_gmail.com created D15359: DTrace aarch64: Avoid calling unwind_frame() in the probe context.
May 8 2018, 8:37 PM · arm64, DTrace

Apr 30 2018

domagoj.stolfa_gmail.com accepted D15221: Disable connectat/bindat with AT_FDCWD parameter in capabilities mode.

Had a quick skim, but LGTM.

Apr 30 2018, 4:05 PM · capsicum

Apr 21 2018

domagoj.stolfa_gmail.com added inline comments to D15150: Split procfs_attr into multiple functions.
Apr 21 2018, 7:54 PM

Apr 12 2018

domagoj.stolfa_gmail.com added a comment to D14656: Add SDT probes to the VMX VMexit interface.

Is this good to land? Thanks!

Apr 12 2018, 4:31 PM · DTrace, bhyve

Apr 6 2018

domagoj.stolfa_gmail.com updated the diff for D14863: DTrace: Add an invariant to check for recursion in dtrace_probe().

Address the other comments by @markj and fix a misleading word in the dtrace_probe_exit() comment.

Apr 6 2018, 5:11 PM
domagoj.stolfa_gmail.com removed 1 blocking reviewer(s) for D14863: DTrace: Add an invariant to check for recursion in dtrace_probe(): gnn.
Apr 6 2018, 4:51 PM
domagoj.stolfa_gmail.com updated the diff for D14863: DTrace: Add an invariant to check for recursion in dtrace_probe().

Address comments by @markj.

Apr 6 2018, 4:50 PM

Apr 4 2018

domagoj.stolfa_gmail.com added inline comments to D9988: Add support for a copyoutmbuf() subroutine for DTrace..
Apr 4 2018, 6:02 PM
domagoj.stolfa_gmail.com added inline comments to D9988: Add support for a copyoutmbuf() subroutine for DTrace..
Apr 4 2018, 4:32 PM

Apr 3 2018

domagoj.stolfa_gmail.com added a comment to D14863: DTrace: Add an invariant to check for recursion in dtrace_probe().

Is there anything else I can do for this to land?

Apr 3 2018, 9:55 PM

Mar 28 2018

domagoj.stolfa_gmail.com added a comment to D14656: Add SDT probes to the VMX VMexit interface.

Should I make any more changes for this to land? As an aside note: I've tried to make the interface as simple as possible to use with the args[0-2] being consistent.

Mar 28 2018, 3:11 PM · DTrace, bhyve

Mar 27 2018

domagoj.stolfa_gmail.com abandoned D14862: Make INVARIANTS enable DEBUG in DTrace.
In D14862#312653, @avg wrote:

FWIW, rS253996 is what I did to translate INVARIANTS to DEBUG for ZFS.
Seems like the commit even touched some dtrace modules.

Mar 27 2018, 5:04 PM
domagoj.stolfa_gmail.com updated the diff for D14862: Make INVARIANTS enable DEBUG in DTrace.

Update the diff based on comments from @markj.

Mar 27 2018, 4:49 PM
domagoj.stolfa_gmail.com added inline comments to D14862: Make INVARIANTS enable DEBUG in DTrace.
Mar 27 2018, 4:44 PM

Mar 26 2018

domagoj.stolfa_gmail.com updated the summary of D14863: DTrace: Add an invariant to check for recursion in dtrace_probe().
Mar 26 2018, 11:25 PM
domagoj.stolfa_gmail.com created D14863: DTrace: Add an invariant to check for recursion in dtrace_probe().
Mar 26 2018, 11:23 PM
domagoj.stolfa_gmail.com created D14862: Make INVARIANTS enable DEBUG in DTrace.
Mar 26 2018, 10:33 PM

Mar 11 2018

domagoj.stolfa_gmail.com created D14656: Add SDT probes to the VMX VMexit interface.
Mar 11 2018, 6:21 PM · DTrace, bhyve

Jan 12 2018

domagoj.stolfa_gmail.com updated the diff for D13877: DTrace: Add jailname/jid builtins.

Address the comments by markj@ and add tests in safety/ for jailname and jid.

Jan 12 2018, 5:15 PM · DTrace
domagoj.stolfa_gmail.com updated the diff for D13877: DTrace: Add jailname/jid builtins.

Update the diff to add more context. No actual code was changed.

Jan 12 2018, 2:01 PM · DTrace
domagoj.stolfa_gmail.com added inline comments to D13877: DTrace: Add jailname/jid builtins.
Jan 12 2018, 1:13 PM · DTrace
domagoj.stolfa_gmail.com updated the summary of D13877: DTrace: Add jailname/jid builtins.
Jan 12 2018, 1:11 PM · DTrace
domagoj.stolfa_gmail.com added a comment to D13877: DTrace: Add jailname/jid builtins.

Tracing on a per-jail basis using a predicate was already possible (example below):

someprobe /curthread->td_proc->p_ucred->cr_prison->pr_id == $1/
{
        /* some actions */
}

That's true, however, this exposes a stable interface that people can use without following pointers in the D script and instead simply calling into a builtin variable. I'd imagine this is the same reason why you would want to have execname, errno, uid, gid and others, as they can also be accessible via other, more primitive builtins.

Don't get me wrong, I like the analog to Solaris's zonename and thus think jailname and jid are good additions.
I just wanted to point out that curthread already gives us access to far more details than the few given at the top-level.
Programs like "dwatch" (reviews.freebsd.org/D10006) make it possible to access not-only curthread details but the details of the parent, grandparent, and ancestor process(es), and thus the ability to trace a jail is neither novel nor unique.
If you're ever looking for inspiration on other top-level variables that might be nice-to-haves without having to crawl curthread->td_proc (as dwatch does), give dwatch a trial run.

Jan 12 2018, 1:07 PM · DTrace

Jan 11 2018

domagoj.stolfa_gmail.com added a comment to D13877: DTrace: Add jailname/jid builtins.

Tracing on a per-jail basis using a predicate was already possible (example below):

someprobe /curthread->td_proc->p_ucred->cr_prison->pr_id == $1/
{
        /* some actions */
}
Jan 11 2018, 11:56 PM · DTrace
domagoj.stolfa_gmail.com updated the summary of D13877: DTrace: Add jailname/jid builtins.
Jan 11 2018, 10:50 PM · DTrace
domagoj.stolfa_gmail.com created D13877: DTrace: Add jailname/jid builtins.
Jan 11 2018, 10:48 PM · DTrace

Jun 18 2017

domagoj.stolfa_gmail.com updated the diff for D8100: Bhyve hypercall implementation.

Update the diff to reflect changes in HEAD with bhyve identification. This diff no longer requires it.

Jun 18 2017, 10:05 PM

Jun 10 2017

domagoj.stolfa_gmail.com created D11138: UUID API extension to allow for comparison.
Jun 10 2017, 5:43 PM

Mar 3 2017

domagoj.stolfa_gmail.com updated the diff for D8100: Bhyve hypercall implementation.

Update the diff to address the compilation errors due to the way clang 4.0 handles -Wstrict-prototypes.

Mar 3 2017, 7:00 PM

Feb 2 2017

domagoj.stolfa_gmail.com added a member for DTrace: domagoj.stolfa_gmail.com.
Feb 2 2017, 5:58 PM

Dec 19 2016

domagoj.stolfa_gmail.com added inline comments to D8100: Bhyve hypercall implementation.
Dec 19 2016, 1:17 PM
domagoj.stolfa_gmail.com updated the diff for D8100: Bhyve hypercall implementation.

This diff changes the following:

  • Remove DTrace stuff as much as possible, create a prototype instead
  • Remove the unnecessary static NULL assignments in the hc_dispatcher
  • Reiterate the instructions on some parts of the code
Dec 19 2016, 1:14 PM

Dec 3 2016

domagoj.stolfa_gmail.com added a comment to D8100: Bhyve hypercall implementation.

Point out an issue regarding mi_startup.

Dec 3 2016, 12:50 PM
domagoj.stolfa_gmail.com updated the diff for D8100: Bhyve hypercall implementation.

Address some style(9) issues, fix a comment.

Dec 3 2016, 12:40 AM

Nov 27 2016

domagoj.stolfa_gmail.com updated the diff for D8100: Bhyve hypercall implementation.

License bhyve_hypercall.S under the FreeBSD preferred license.

Nov 27 2016, 6:17 PM
domagoj.stolfa_gmail.com updated the diff for D8100: Bhyve hypercall implementation.

Remove the unused include in bhyve_hypercall.h

Nov 27 2016, 4:52 PM
domagoj.stolfa_gmail.com updated the diff for D8100: Bhyve hypercall implementation.

Address the points discussed at MeetBSD.

Nov 27 2016, 4:12 PM

Oct 13 2016

domagoj.stolfa_gmail.com updated the test plan for D8100: Bhyve hypercall implementation.
Oct 13 2016, 11:55 PM
domagoj.stolfa_gmail.com updated the diff for D8100: Bhyve hypercall implementation.

Addresses a late noticed typo in the macro HC_CPUID_ID which in fact should be HV_CPUID_ID.

Oct 13 2016, 11:49 PM
domagoj.stolfa_gmail.com updated the diff for D8100: Bhyve hypercall implementation.

This patch addresses a couple of issues:

  • Allow global enabling and disabling of hypercalls. The hypercall exists are by default disabled
  • Add the necessary dispatchers and handlers for a hypervisor emulation layer
  • Clean up the bhyve_hypercall.h using macros and revert to the calling convention that uses the stack
  • Add a tunable which allows setting of the hypervisor mode
  • Unimplemented hypercalls now cause an #UD fault
  • CPUID_4000_0001 gives information in EAX whether hypercalls are enabled or not
Oct 13 2016, 11:41 PM

Oct 3 2016

domagoj.stolfa_gmail.com added inline comments to D8100: Bhyve hypercall implementation.
Oct 3 2016, 4:50 PM
domagoj.stolfa_gmail.com updated the diff for D8100: Bhyve hypercall implementation.

This patch should address some of the concerns brought on by Peter.

Oct 3 2016, 4:47 PM

Oct 1 2016

domagoj.stolfa_gmail.com added a comment to D8100: Bhyve hypercall implementation.

Thanks for this work - it will be very useful.
Some general high-level comments:

  • There are a number of guest users of hypercalls: KVM, Xen, Hyper-V. Since bhyve may emulate some/all of these hypercalls, it's probably worthwhile looking at the call sequences they use to try and co-exist. For example, it may even be useful at some future point to have DTrace support in those hypervisors.
  • In a similar vein, 'hypercall.h' should probably be 'bhyve_hypercall.h'
  • the hypercall exit should be disabled by default unless a tunable is set. It's an attack point that should only be enabled through hypervisor configuration. While there isn't currently a generic way to set per-guest configuration, there will be at some point, at which time this feature could be shifted over to that instead of having a global enable/disable.
  • in addition, a future work item, and not really part of this review, is to have a CPUID space carved out for bhyve that will provide a way for the guest to determine if the hypercall facility is available. See the KVM docs for an example of how this is done.
Oct 1 2016, 2:57 PM

Sep 30 2016

domagoj.stolfa_gmail.com added a comment to D8100: Bhyve hypercall implementation.

Thanks for the review Peter. I'll address these issues as soon as possible.

Sep 30 2016, 8:53 PM
domagoj.stolfa_gmail.com added inline comments to D8100: Bhyve hypercall implementation.
Sep 30 2016, 8:42 PM
domagoj.stolfa_gmail.com added a comment to D8100: Bhyve hypercall implementation.

Added some explanations.

Sep 30 2016, 7:57 PM
domagoj.stolfa_gmail.com retitled D8100: Bhyve hypercall implementation from to Bhyve hypercall implementation.
Sep 30 2016, 7:54 PM