[PATCH v2 0/3] initscript: add support for uprobes scripts

[PATCH v2 0/3] initscript: add support for uprobes scripts

[Stefan Hajnoczi]

v2:
 * Add Patch 1 with stap --save-uprobes option [Josh]
 * Switch from stap -k to stap --save-uprobes [Josh]

The initscript currently fails for user-space probing scripts on systems where
uprobes.ko is built from source by stap(1).  This is because the initscript
uses a two-phase "compile and then run" approach:

The uprobes.ko module is generated during the compile phase but not copied into
the cache directory where modules are placed for the run phase.  The staprun(8)
command fails because the script module cannot be loaded without uprobes.ko.

These patches address the issue by copying uprobes.ko into the cache directory.
If a script specifies the -u option in its initscript configuration file,
staprun(8) will receive the path to uprobes.ko.

There is no change in behavior on systems that do not build uprobes.ko.  This
has been tested on RHEL6 (builds uprobes.ko) and RHEL7 (systemtap and does not
build uprobes.ko).

Stefan Hajnoczi (3):
  stap: add --save-uprobes
  initscript: copy uprobes.ko to cache directory
  initscript: allow scripts to load uprobes

 buildrun.cxx            |  4 +++-
 cmdline.cxx             |  1 +
 cmdline.h               |  1 +
 initscript/systemtap.in | 15 +++++++++++++++
 main.cxx                |  8 ++++++++
 session.cxx             | 10 ++++++++++
 session.h               |  2 ++
 7 files changed, 40 insertions(+), 1 deletion(-)

-- 
1.9.3

[PATCH v2 1/3] stap: add --save-uprobes

[Stefan Hajnoczi]

The stap -m <name> option saves the script module.  For scripts that
rely on uprobes it may also be necessary to get the uprobes module (if
it was built by stap).

The new stap --save-uprobes option saves uprobes/uprobes.ko into the
current directory.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 buildrun.cxx |  4 +++-
 cmdline.cxx  |  1 +
 cmdline.h    |  1 +
 main.cxx     |  8 ++++++++
 session.cxx  | 10 ++++++++++
 session.h    |  2 ++
 6 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/buildrun.cxx b/buildrun.cxx
index 6f9b78b..9339b49 100644
--- a/buildrun.cxx
+++ b/buildrun.cxx
@@ -601,8 +601,10 @@ make_uprobes (systemtap_session& s)
     clog << _("uprobes rebuild exit code: ") << rc << endl;
   if (rc)
     s.set_try_server ();
-  else
+  else {
     s.uprobes_path = dir + "/uprobes.ko";
+    s.built_uprobes = true;
+  }
   return rc;
 }
 
diff --git a/cmdline.cxx b/cmdline.cxx
index 090659d..d50ca1a 100644
--- a/cmdline.cxx
+++ b/cmdline.cxx
@@ -61,5 +61,6 @@ struct option stap_long_options[] = {
   { "benchmark-sdt-threads",       required_argument, NULL, LONG_OPT_BENCHMARK_SDT_THREADS },
   { "color",                       optional_argument, NULL, LONG_OPT_COLOR_ERRS },
   { "colour",                      optional_argument, NULL, LONG_OPT_COLOR_ERRS },
+  { "save-uprobes",                no_argument,       NULL, LONG_OPT_SAVE_UPROBES },
   { NULL, 0, NULL, 0 }
 };
diff --git a/cmdline.h b/cmdline.h
index fa19d65..297096f 100644
--- a/cmdline.h
+++ b/cmdline.h
@@ -59,6 +59,7 @@ enum {
   LONG_OPT_BENCHMARK_SDT_LOOPS,
   LONG_OPT_BENCHMARK_SDT_THREADS,
   LONG_OPT_COLOR_ERRS,
+  LONG_OPT_SAVE_UPROBES,
 };
 
 // NB: when adding new options, consider very carefully whether they
diff --git a/main.cxx b/main.cxx
index 5150a30..d8b1d98 100644
--- a/main.cxx
+++ b/main.cxx
@@ -994,6 +994,14 @@ passes_0_4 (systemtap_session &s)
 	  string module_dest_path = s.module_filename();
 	  copy_file(module_src_path, module_dest_path, s.verbose > 1);
 	}
+
+      // Copy uprobes module to the current directory.
+      if (s.save_uprobes && s.built_uprobes && !pending_interrupts)
+        {
+          rc = create_dir("uprobes");
+          if (! rc)
+            copy_file(s.uprobes_path, "uprobes/uprobes.ko", s.verbose > 1);
+        }
     }
 
   PROBE1(stap, pass4__end, &s);
diff --git a/session.cxx b/session.cxx
index 56198ef..201ae6c 100644
--- a/session.cxx
+++ b/session.cxx
@@ -134,6 +134,7 @@ systemtap_session::systemtap_session ():
   output_file = ""; // -o FILE
   tmpdir_opt_set = false;
   save_module = false;
+  save_uprobes = false;
   modname_given = false;
   keep_tmpdir = false;
   cmd = "";
@@ -145,6 +146,7 @@ systemtap_session::systemtap_session ():
   need_uprobes = false;
   need_unwind = false;
   need_symbols = false;
+  built_uprobes = false;
   uprobes_path = "";
   load_only = false;
   skip_badvars = false;
@@ -315,6 +317,7 @@ systemtap_session::systemtap_session (const systemtap_session& other,
   output_file = other.output_file; // XXX how should multiple remotes work?
   tmpdir_opt_set = false;
   save_module = other.save_module;
+  save_uprobes = other.save_uprobes;
   modname_given = other.modname_given;
   keep_tmpdir = other.keep_tmpdir;
   cmd = other.cmd;
@@ -326,6 +329,7 @@ systemtap_session::systemtap_session (const systemtap_session& other,
   need_uprobes = false;
   need_unwind = false;
   need_symbols = false;
+  built_uprobes = false;
   uprobes_path = "";
   load_only = other.load_only;
   skip_badvars = other.skip_badvars;
@@ -637,6 +641,8 @@ systemtap_session::usage (int exitcode)
     "              relative to the sysroot.\n"
     "   --suppress-time-limits\n"
     "              disable -DSTP_OVERLOAD, -DMAXACTION, and -DMAXTRYACTION limits\n"
+    "   --save-uprobes\n"
+    "              save uprobes.ko to current directory if it is built from source\n"
     , compatible.c_str()) << endl
   ;
 
@@ -1390,6 +1396,10 @@ systemtap_session::parse_cmdline (int argc, char * const argv [])
                     strcmp(getenv("TERM") ?: "notdumb", "dumb"));
           break;
 
+        case LONG_OPT_SAVE_UPROBES:
+          save_uprobes = true;
+          break;
+
 	case '?':
 	  // Invalid/unrecognized option given or argument required, but
 	  // not given. In both cases getopt_long() will have printed the
diff --git a/session.h b/session.h
index e95a26d..70f7909 100644
--- a/session.h
+++ b/session.h
@@ -197,6 +197,7 @@ public:
   unsigned verbose;
   bool timing;
   bool save_module;
+  bool save_uprobes;
   bool modname_given;
   bool keep_tmpdir;
   bool guru_mode;
@@ -210,6 +211,7 @@ public:
   bool need_uprobes;
   bool need_unwind;
   bool need_symbols;
+  bool built_uprobes;
   std::string uprobes_path;
   std::string uprobes_hash;
   bool load_only; // flight recorder mode
-- 
1.9.3

[PATCH v2 3/3] initscript: allow scripts to load uprobes

[Stefan Hajnoczi]

Scripts that rely on userspace probing must include the -u option in
their conf file:

  # cat /etc/systemtap/conf.d/example.conf
  example_OPT=-u

The uprobe kernel module will be loaded when the script runs.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 initscript/systemtap.in | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/initscript/systemtap.in b/initscript/systemtap.in
index ebd28e4..d61d480 100755
--- a/initscript/systemtap.in
+++ b/initscript/systemtap.in
@@ -422,6 +422,12 @@ get_run_opts () { # normalized_opts
   show=0
   for o in $opts; do
     case $o in
+    -u)
+      # Use cached uprobes module, if available
+      if [ -f "$CACHE_PATH/uprobes/uprobes.ko" ]; then
+        echo -n "-u$CACHE_PATH/uprobes/uprobes.ko "
+      fi
+      ;;
     -c|-x|-s|-o|-S)
       [ $o == '-s' ] && o='-b'
       [ $o == '-o' ] && mode='-D'
-- 
1.9.3

[PATCH v2 2/3] initscript: copy uprobes.ko to cache directory

[Stefan Hajnoczi]

If the uprobes.ko module was built then it will be needed at staprun
time.  Copy the module into the cache directory alongside compiled
scripts.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 initscript/systemtap.in | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/initscript/systemtap.in b/initscript/systemtap.in
index 2d770e4..ebd28e4 100755
--- a/initscript/systemtap.in
+++ b/initscript/systemtap.in
@@ -403,6 +403,8 @@ get_compile_opts () { # opts
        skip=1 ;;
     -h|-V|-k|-F)
       ;;
+    -u)
+      echo -n "--save-uprobes " ;;
     *)
       echo -n $o" " ;;
     esac
@@ -492,6 +494,13 @@ compile_script () { # script checkcache
   logex $STAP -m "$1" -p4 -r $KRELEASE $opts "$f"
   ret=$?
   if [ $ret -eq 0 ]; then
+    if [ -f "uprobes/uprobes.ko" ]; then
+      logex mkdir -p "$CACHE_PATH/uprobes"
+      logex mv "uprobes/uprobes.ko" "$CACHE_PATH/uprobes/"
+      ret=$?
+    fi
+  fi
+  if [ $ret -eq 0 ]; then
     $UNAME -a > "$1.opts"
     echo $opts >> "$1.opts"
     logex mv "$1.ko" "$1.opts" "$CACHE_PATH/"
-- 
1.9.3

Re: [PATCH v2 0/3] initscript: add support for uprobes scripts

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 6 bytes --]

Ping?

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2 0/3] initscript: add support for uprobes scripts

[Jonathan Lebon]

Hi Stefan,

I'm wondering, could we do without the user needing to
provide the -u option at all and make it completely
transparent instead?

I.e. the initscript could always use the new
--save-uprobes switch, and remember whether uprobes.ko was
built or not so that at runtime, it appropriately appends
-u to the staprun command.

This also works around overriding the '-u' option in the
config file, which could previously be used to enable
unoptimized mode.

The only question is how/where this setting would be
remembered. One way I can think of right now is to simply
touch a blank file in the CACHE_PATH's uprobes directory for
each script that requires it, but maybe there's a more
elegant method.

Re: [PATCH v2 0/3] initscript: add support for uprobes scripts

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 832 bytes --]

On Tue, Sep 02, 2014 at 11:03:26AM -0400, Jonathan Lebon wrote:
> I'm wondering, could we do without the user needing to
> provide the -u option at all and make it completely
> transparent instead?

That's a nice idea.

> The only question is how/where this setting would be
> remembered. One way I can think of right now is to simply
> touch a blank file in the CACHE_PATH's uprobes directory for
> each script that requires it, but maybe there's a more
> elegant method.

The $CACHE_PATH/$SCRIPT.opts file seems like the logical place to save
it.  Currently the format is:

$(uname -a)
stap compile opts

We can add a new line extra options (in a backwards-compatible fashion
to avoid breaking existing cache directories).  If it contains
--load-uprobes then staprun will be invoked with
-u$CACHE_PATH/uprobes/uprobes.ko.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2 0/3] initscript: add support for uprobes scripts

[Jonathan Lebon]

Hi Stefan,

I've pushed the original series for now (v2).
Thanks for the contribution! :)

I've added a follow-up commit to make use of --save-uprobes rather
than -u so that we don't block the stap -u option while remaining
compatible with the idea we mentioned.

BTW, while testing, I've noticed that --save-uprobes only has an
effect if uprobes.ko was not already built and cached in a previous
run. Have you encountered that as well?

We may need to tweak the buildrun.cxx logic a bit if that's the
case.

Re: [PATCH v2 0/3] initscript: add support for uprobes scripts

[Josh Stone]

On 09/03/2014 04:01 PM, Jonathan Lebon wrote:
> BTW, while testing, I've noticed that --save-uprobes only has an
> effect if uprobes.ko was not already built and cached in a previous
> run. Have you encountered that as well?
> 
> We may need to tweak the buildrun.cxx logic a bit if that's the
> case.

I'd guess that get_cached_uprobes() ought to set s.built_uprobes when it
assigns s.uprobes_path, just as make_uprobes() does.

Actually, I think s.built_uprobes may not be needed at all, just check
s.uprobes_path.empty().

[PATCH] Remove systemtap_session::built_uprobes

[Josh Stone]

For the purpose of save_uprobes, it doesn't actually matter whether
uprobes.ko was just built or was pulled from the cache.  If we have the
uprobes_path at all, go ahead and save it.
---
 buildrun.cxx | 4 +---
 main.cxx     | 2 +-
 session.cxx  | 2 --
 session.h    | 1 -
 4 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/buildrun.cxx b/buildrun.cxx
index 67217c3459e5..5d3ab582c058 100644
--- a/buildrun.cxx
+++ b/buildrun.cxx
@@ -602,10 +602,8 @@ make_uprobes (systemtap_session& s)
     clog << _("uprobes rebuild exit code: ") << rc << endl;
   if (rc)
     s.set_try_server ();
-  else {
+  else
     s.uprobes_path = dir + "/uprobes.ko";
-    s.built_uprobes = true;
-  }
   return rc;
 }
 
diff --git a/main.cxx b/main.cxx
index af4be20aa6bf..f88701cba1b8 100644
--- a/main.cxx
+++ b/main.cxx
@@ -990,7 +990,7 @@ passes_0_4 (systemtap_session &s)
 	}
 
       // Copy uprobes module to the current directory.
-      if (s.save_uprobes && s.built_uprobes && !pending_interrupts)
+      if (s.save_uprobes && !s.uprobes_path.empty() && !pending_interrupts)
         {
           rc = create_dir("uprobes");
           if (! rc)
diff --git a/session.cxx b/session.cxx
index e2a518ba0538..ee4e81b72e31 100644
--- a/session.cxx
+++ b/session.cxx
@@ -145,7 +145,6 @@ systemtap_session::systemtap_session ():
   need_uprobes = false;
   need_unwind = false;
   need_symbols = false;
-  built_uprobes = false;
   uprobes_path = "";
   load_only = false;
   skip_badvars = false;
@@ -327,7 +326,6 @@ systemtap_session::systemtap_session (const systemtap_session& other,
   need_uprobes = false;
   need_unwind = false;
   need_symbols = false;
-  built_uprobes = false;
   uprobes_path = "";
   load_only = other.load_only;
   skip_badvars = other.skip_badvars;
diff --git a/session.h b/session.h
index e92fff167e88..63cacc997632 100644
--- a/session.h
+++ b/session.h
@@ -209,7 +209,6 @@ public:
   bool need_uprobes;
   bool need_unwind;
   bool need_symbols;
-  bool built_uprobes;
   std::string uprobes_path;
   std::string uprobes_hash;
   bool load_only; // flight recorder mode
-- 
1.9.3

Re: [PATCH] Remove systemtap_session::built_uprobes

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 452 bytes --]

On Wed, Sep 03, 2014 at 04:44:30PM -0700, Josh Stone wrote:
> For the purpose of save_uprobes, it doesn't actually matter whether
> uprobes.ko was just built or was pulled from the cache.  If we have the
> uprobes_path at all, go ahead and save it.
> ---
>  buildrun.cxx | 4 +---
>  main.cxx     | 2 +-
>  session.cxx  | 2 --
>  session.h    | 1 -
>  4 files changed, 2 insertions(+), 7 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] Remove systemtap_session::built_uprobes

[Jonathan Lebon]

Hey Josh,

This patch fixes the issue mentioned in the other thread for me.