Re: [PATCH] rawfbt: new provider

[Kris Van Hees <kris.van.hees@oracle.com>]

On Mon, Dec 09, 2024 at 04:29:19PM +0000, Nick Alcock wrote:
> On 9 Dec 2024, Alan Maguire stated:
> 
> > On 06/12/2024 16:33, Kris Van Hees wrote:
> >> On Fri, Dec 06, 2024 at 10:35:50AM +0000, Alan Maguire wrote:
> >>> On 05/12/2024 19:42, Kris Van Hees wrote:
> >>>> This provider provides access to all kprobe-based probes that are
> >>>> available on the system.  This includes any compiler-generated
> >>>> optimized variants of functions, named <func>.<suffix>.
> >>>>
> >>>
> >>> This is great! Having a rawfbt allows users to still trace cases
> >>> where fprobe can't currently handle.
> >>>
> >>> On the .suffix; I get that this is intended to be a raw provider, but
> >>> would it be better for stability to expose these functions as "func"
> >>> rather than "func.suffix"? It becomes difficult to write portable
> >>> scripts when suffixes are included, because wildcarding
> >>>
> >>> rawfbt::func*:entry
> >>>
> >>> ...to catch both suffixed and non-suffixed variants may end up tracing
> >>> func, func.suffix but also func2 etc.
> >> 
> >> The trouble is that exposing <func>.<suffix> as <func> will mean that you
> >> automatically end up trying to probe *any* <func>.<suffix> whenever you
> >> place a probe on <func> using rawfbt, which is not necessarily what you may
> >> want.  And there would not be any way to avoid that.
> >> 
> >> But by not collapsing them, you have the ability to catch them all anyway,
> >> if you want, by specifying <func>, <func>.* instead of <func>.
> >
> > I tried that with the rawfbt code but got:
> >
> > # dtrace -n 'rawfbt::xfs_cleanup_inode.*:entry {}'
> > dtrace: description 'rawfbt::xfs_cleanup_inode.*:entry ' matched 1 probe
> > ^C
> >
> > This matches xfs_cleanup_inode.isra.0, looks good so far. However:
> >
> > # dtrace -n
> > 'rawfbt::xfs_cleanup_inode:entry,rawfbt::xfs_cleanup_inode.*:entry {}'
> > dtrace: invalid probe specifier
> > rawfbt::xfs_cfs_cleanup_inode:entry,rawfbt::xfs_cleanup_inode.*:entry
> > {}: probe description rawfbt::xfs_cfs_cleanup_inode:entry does not match
> > any probes
> >
> > So the absence of the non-suffixed function (which we included for
> > portability) triggers the failure. So looks like it will be necessary to
> > add -Z:
> >
> > # /sbin/dtrace -Zn
> > 'rawfbt::xfs_cfs_cleanup_inode:entry,rawfbt::xfs_cleanup_inode.*:entry {}'
> > dtrace: description
> > 'rawfbt::xfs_cfs_cleanup_inode:entry,rawfbt::xfs_cleanup_inode.*:entry '
> > matched 1 probe
> >
> > So I guess as long as we document this clearly for folks wanting to to
> > use rawfbt to write portable scripts, it should be okay.
> 
> Agreed. Documented behaviour, trivial workaround, maybe can be improved
> later -- maybe we shouldn't get a 'no probes' error unless *every probe*
> in a probe list is unmatched? There are good reasons for both... maybe
> we should add "foo & bar" and "foo | bar" and parens for precedence in
> later releases, but this is huge overdesign :P

This is indeed proper behaviour.  If the probe does not exist, the tracing
should not start and an error should be reported.

Since these probes are made available by the kernel based on the kernel build
(i.e. the functions that it determines can be probed using kprobe), they are
not at all a stable interface and therefore portability of scripts is not at
all supported.  Often that can be resolved with using -Z indeed (or the
equivalent pragma that can be included in the script).

I could *perhaps* see some value in someday being able to specify that certain
probes are required and others are optional (in a script), possibly using a
more refined pragma that assigns such status to particular probe specifications
but that is certainly only a vague notion of potential future feature :)