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

[Eugene Loh <eugene.loh@oracle.com>]

So do you have a proposal for what the new semantics for bufsize should 
be?  (And, why not simply ignore bufsize?)

On 8/29/24 22:26, Kris Van Hees wrote:
> On Thu, Aug 29, 2024 at 08:54:46PM -0400, Eugene Loh via DTrace-devel wrote:
>> 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).
> Correct.
>
>> *)  Round that up per perf_event requirements (2^n + 1 pages).
> No, round up to nearest 2^n.  The extra page is allocated automatically by
> dt_peb_open().
>
>> *)  Ignore the user's bufsize setting.
> Well, no.  The bufsize setting is not ignored.  We just change the semantics
> a little (in view of the new implementation) to be a mechanism to allow the
> user to tune the buffer size, but within certain limitations.
>
>> 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.
> We do not ignore the bufsize as such.  But the extent to which the bugsize
> can be tuned is limited by the implementation.  The bufsizw setting cannot be
> less than the minimum requirement (and you prefer to not complain about that
> and instead just increase the buffer size).  And its value must be a 2^n
> multiple of the page size, so it will be rounded up to the nearest acceptable
> value.  But that is therefore still controlled by the user's bufsize setting.
>
>>>>> 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@oracle.com wrote:
>>>>>>>> From: Eugene Loh <eugene.loh@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.
>> _______________________________________________
>> DTrace-devel mailing list
>> DTrace-devel@oss.oracle.com
>> https://oss.oracle.com/mailman/listinfo/dtrace-devel