[DTrace-devel] [PATCH 31/38] Fix dt_pebs_init() call

[Eugene Loh]

On 8/28/24 17:16, Kris Van Hees wrote:

> On Wed, Aug 28, 2024 at 04:57:43PM -0400, Eugene Loh wrote:
>> On 8/26/24 12:20, Kris Van Hees wrote:
>>> On Mon, Aug 26, 2024 at 11:42:25AM -0400, Eugene Loh wrote:
>>>> On 8/26/24 10:30, Kris Van Hees wrote:
>>>>> This patch goes against the intent of the code, as far as I can see.  The
>>>>> point of performing the size check prior to increasing the buffer size was
>>>>> to enforce the limit on the size that was explicit set by the user.  Then,
>>>>> if that size was sufficient, then DTrace is at liberty to increase that
>>>>> size to a whole multiple of the page size.
>>>>>
>>>>> But if I were to insist that a buffer can only be 24 bytes, and 24 bytes are
>>>>> not enough to store a single trace record, DTrace should report an error
>>>>> rather than increasing my bufsize setting on its own.
>>>> The point is that DTrace *does* increase my bufsize setting on its own.  Why
>>>> should DTrace complain about my setting when it's going to ignore my setting
>>>> anyhow?  What value does the pre-existing behavior offer to DTrace users?
>>>> Are you saying that DTrace should not resize?  That it should honor the
>>>> user's bufsize?  No more resizing (to page size or whatever it wants)?
>>> The main point of this code was to retain the bevhaviour that DTrace provided,
>> As far as adhering to legacy behavior... quite simply, we do not. We do not
>> set the "principal buffer size" to bufsize.  Further, any legacy resizing
>> behavior is, if I understand correctly, to go smaller than the user
>> requested;  we do the opposite.  Looking to the legacy behavior for
>> precedent is pointless.  The situation is that we are unlike legacy behavior
>> in fundamental respects, opposite in another respect (automatic resizing),
>> but consistent in some impractical and useless respect.  That consistency
>> strikes me as small consolation.
> You can look at this from different angles, yes.
>
>> There is no documentation to change;  the documentation is already wrong.
>> This patch was intended as a step to making the behavior something that was
>> describable.  Clearly, trying to hold on to a legacy buffering model has
>> produced a byzantine situation in the current port, which treats buffering
>> very differently.
> Yes, but if you want to go that route we ought to go all the way, and not some
> interim solution (see below).
>
>> Perhaps we simply disagree on those points, and this patch is DOA. But the
>> patch points out other problems as well.  E.g., the min computation wasn't
>> even right and error checking was wrong.
> Let's do this as two patches:
>   - one that ensures that the computation is updated to the current
>     implementation changes and is correct
>   - one that provides for increasing the bufsize to be the smallest multiple of
>     pagesize to fit the largest trace record needed for the tracing session
>
> I don't think that checking the rounded up bufsize against the minimum size
> makes any sense if you are going to increase the specified bufsize anyway.
> Might as well increase it to fit the minimum size altogether.

I'd like to understand this proposal.  Are you saying:

*)  Determine the min size (correctly).

*)  Round that up per perf_event requirements (2^n + 1 pages).

*)  Ignore the user's bufsize setting.

I'm fine with that (given that we do buffering so unlike the legacy 
implementation and will not be honoring the user's bufsize value 
anyhow), but I just wanted to be explicit about ignoring the user's bufsize.

>>> which is that if a user explicitly requests a bufsize that is less than the
>>> minimum required for the given trace programs, an error is reported.  That is
>>> a mater of being consistent (even if it has no practical application that I
>>> can see).  Tests from the existing version failed because this was not being
>>> done right.
>>>
>>> As far as functionality, as long as the bufsize is large enough to hold the
>>> largest trace record, DTrace can (and needs to) ensure that the buffer size
>>> is a whole multiple of the page size because that is required by the perf
>>> ring buffer imlementation.
>>>
>>> So, DTrace only increases the buffer size to the required multiple of page size
>>> if the user supplied bufsize is at least large enough to hold the largest trace
>>> record (or if no bufsize was explicitly specified).
>>>
>>> We could opt to change the behaviour of the bufsize opton to always round up to
>>> a whole multiple of page size, but that is a deviation from the original
>>> implementation and thus is something that would need to be documented, etc...
>>>
>>> Aside from the computation of the minimum required size needing updating when
>>> patches (e.g. the USDT changes) modify the layout and size of the trace records,
>>> I do not think that the existing code is wrong.
>>>
>>> I am certainly open to a feature change to make bufsize always be rounded up as
>>> needed, but that is not a bug fix and I think that ought to be its own patch
>>> (along with documentation changes etc).  And it might need some more discussion
>>> because it brings up the question why we wouldn't just simply always increase
>>> bufsize to be the smallest multiple of the page size to accomodate the largest
>>> trace record.
>>>
>>>>> On Thu, Jun 27, 2024 at 01:38:57AM -0400, eugene.loh at oracle.com wrote:
>>>>>> From: Eugene Loh <eugene.loh at oracle.com>
>>>>>>
>>>>>> The function had a few issues, mainly with its comments,
>>>>>> ranging from typos to incorrect descriptions of behavior.
>>>>>>
>>>>>> The function has only one caller.  The enforcement of a
>>>>>> size minimum was problematic in a number of respects:
>>>>>> - it was missing 4 bytes
>>>>>> - it was enforcing the minimum before the size was
>>>>>>        increased by the caller
>>>>>> - it was checking dt_pebs_init()==-1,
>>>>>>        missing errors with other negative return values
>>>>>>
>>>>>> So change the function to accept a minimum and change its
>>>>>> return values.