[DTrace-devel] [PATCH 05/12] Add support for the stack() action

[Kris Van Hees]

On Wed, Jun 09, 2021 at 02:14:15AM -0400, Eugene Loh wrote:
> On 6/9/21 1:21 AM, Eugene Loh wrote:
> 
>     On 6/9/21 12:43 AM, Kris Van Hees wrote:
> 
>          Fri, May 28, 2021 at 02:35:09PM -0400, eugene.loh at oracle.com wrote:
> 
>             From: Eugene Loh <eugene.loh at oracle.com>
> 
>             Implement the stack() action using the bpf_get_stack() helper
>             function.  This implementation most closely resembles the legacy
>             DTrace implementation.  Someday it may make sense to switch over
>             to the bpf_get_stackid() instead, which would allow consolidation
>             of like stacks.
> 
>             The max stack size can be controlled by the kernel parameter
>             perf_event_max_stack, and we would like to allow users to increase
>             that limit.  We will eventually need to know this limit in various
>             places.  So, add a dtp->dt_maxframes value.
> 
>         It seems that the max frames setting should perhaps be an option, just
>         like various other tunables?
> 
> 
>     I can imagine that, but that would be a new option and I would see it being
>     a separate patch.  If you really want it in this patch, just let me know.
> 
> 
>         I could see a potential use for setting the
>         maxframes value to be less than the system max (e.g. to avoid wasting space
>         in buffers because I know I won't need the max anyway).
> 
> 
> I wanted to add another comment here.  The only case where a lower value would
> be of interest is for [u]stackdepth support, where we need to set aside some
> room for a stack that doesn't interest us.  (We only care about the return
> value of bpf_get_stack(), but for the stackdepth to be reflected in that return
> value we need the stack buffer to be large enough.)  For stack([n]) calls, a
> lower maxframes value does us no good.  So, really, the option you are
> proposing should be named in some way that specifies that it's related to
> stackdepth.  Then, we would unnecessarily be imposing the same limit on stack
> ().  Or, we don't need to impose the stackdepth limit on stack(), but then we
> still need to know the kernel limit, which was what I was trying to do.
> 
> How about we leave this "option" for a future patch once we have a better sense
> of what's worth doing.
> 
> [Not sure that's clear.  The point is that the only "wasting space" is just for
> stackdepth.  There is no such issue for stack().  For stack(), the only issue
> is whether stack(n) is requesting more than the kernel will permit.]

Well, yes, as you say there are two aspects to this: impact on stackdepth and
impact on stack([n]).  But they are somewhat linked in the sense that if I
retrieve a stack with a large enough n value (say, identical to the max system
limit) then stackdepth should reflect the size of the stack reported by
stack().  If I request less frames, then of course they will not match.

But this also means that any maximum on frames to retrieve should be applied
to both.  You definitely do not want to limit stackdepth to a number of frames
that is less than what stack() can obtain.

Then the question becomes whether there is value in being able to override the
system limit for stack traces.  I do believe there is because that limit could
be set to a value that impacts DTrace negatively (say, someone needs it to be
really high - well beyond anything we need).  Being able to set our own max
frames limit helps in that case, and then it ought to apply to both stack()
and stackdepth for consistency of results.

The effort to make it an option is minimal.  And yes, it probably should be its
own patch, coming before this one in the series so that this and following
patches can make use of it.

>         So, the default
>         would be the perf_event_max_stack value, but it would be something the user
>         can override with a pragma or -x option.  Then you an access the value just
>         like any other option, and you do not need to add another member to the
>         dtrace_hdl_t struct.
> 
>         Since there is a stackframes option, perhaps use maxstackframes for this?
> 

> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel