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

[Kris Van Hees]

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 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.
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel