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

[Eugene Loh]

On 6/9/21 10:23 AM, Kris Van Hees wrote:

> 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.

Perhaps the effort to introduce new options is minimal, but defining new 
options in a meaningful way perhaps is not.  When we are so far behind 
on feature parity, I'm reluctant to imagine and implement new features.  
The user interface should not change capriciously, and there is a cost 
associated with maintaining features, especially if the underlying 
implementation changes.  E.g., what if we get a bpf_get_stackdepth() and 
this whole issue goes away?  We'd have a vestigial option.

This option would presumably make sense only for stackdepth.

Why should stackdepth and stack() size match?  Well, unfettered, the 
semantics of stackdepth are that it should be the stack() size.  But if 
you place restrictions -- like stack(n) -- then stack(n) size can be 
smaller.  So, if someone wanted to bound stackdepthframes with a novel 
option, we could imagine stackdepth being smaller... again, just as 
historically one could restrict stack(n).  You say of course stackdepth 
and stack() size would not match if you restrict stack().  Just as 
naturally, you would not expect them to match if you restrict stackdepth.

Or another option is not to allocate the stackdepth buffer at all if 
that built-in variable isn't being used.

Bottom line is that introducing such a new option at this point does not 
make sense to me.

>
>>          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?