[PATCH] trace-cmd: Try alternate path for message cache

[PATCH] trace-cmd: Try alternate path for message cache

[Joel Fernandes]

For some systems like Android, /tmp/ does not exist. Use the /data/
directory for message cache.

With this, host guest tracing works on Android VM running on ChromeOS.

Cc: Vineeth Pillai <vineethrp@google.com>
Signed-off-by: Joel Fernandes <joel@joelfernandes.org>
---
 lib/trace-cmd/include/private/trace-cmd-private.h |  3 ++-
 lib/trace-cmd/trace-msg.c                         | 10 ++++++++--
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
index 6934376..492ad9c 100644
--- a/lib/trace-cmd/include/private/trace-cmd-private.h
+++ b/lib/trace-cmd/include/private/trace-cmd-private.h
@@ -377,7 +377,8 @@ enum tracecmd_msg_flags {
 };
 
 /* for both client and server */
-#define MSG_CACHE_FILE "/tmp/trace_msg_cacheXXXXXX"
+#define MSG_CACHE_FILE  "/tmp/trace_msg_cacheXXXXXX"
+#define MSG_CACHE_FILE2 "/data/trace_msg_cacheXXXXXX"
 struct tracecmd_msg_handle {
 	int			fd;
 	short			cpu_count;
diff --git a/lib/trace-cmd/trace-msg.c b/lib/trace-cmd/trace-msg.c
index 03b853e..91dac77 100644
--- a/lib/trace-cmd/trace-msg.c
+++ b/lib/trace-cmd/trace-msg.c
@@ -595,8 +595,14 @@ int tracecmd_msg_handle_cache(struct tracecmd_msg_handle *msg_handle)
 	if (msg_handle->cfd < 0) {
 		strcpy(msg_handle->cfile, MSG_CACHE_FILE);
 		msg_handle->cfd = mkstemp(msg_handle->cfile);
-		if (msg_handle->cfd < 0)
-			return -1;
+		if (msg_handle->cfd < 0) {
+			/* Try an alternate path. */
+			strcpy(msg_handle->cfile, MSG_CACHE_FILE2);
+			msg_handle->cfd = mkstemp(msg_handle->cfile);
+			if (msg_handle->cfd < 0) {
+				return -1;
+			}
+		}
 		unlink(msg_handle->cfile);
 	}
 	msg_handle->cache = true;
-- 
2.35.1.1021.g381101b075-goog

Re: [PATCH] trace-cmd: Try alternate path for message cache

[Steven Rostedt]

On Tue, 29 Mar 2022 19:18:01 +0000
Joel Fernandes <joel@joelfernandes.org> wrote:

> For some systems like Android, /tmp/ does not exist. Use the /data/
> directory for message cache.
> 
> With this, host guest tracing works on Android VM running on ChromeOS.
> 
> Cc: Vineeth Pillai <vineethrp@google.com>
> Signed-off-by: Joel Fernandes <joel@joelfernandes.org>
> ---
>  lib/trace-cmd/include/private/trace-cmd-private.h |  3 ++-
>  lib/trace-cmd/trace-msg.c                         | 10 ++++++++--
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
> index 6934376..492ad9c 100644
> --- a/lib/trace-cmd/include/private/trace-cmd-private.h
> +++ b/lib/trace-cmd/include/private/trace-cmd-private.h
> @@ -377,7 +377,8 @@ enum tracecmd_msg_flags {
>  };
>  
>  /* for both client and server */
> -#define MSG_CACHE_FILE "/tmp/trace_msg_cacheXXXXXX"
> +#define MSG_CACHE_FILE  "/tmp/trace_msg_cacheXXXXXX"
> +#define MSG_CACHE_FILE2 "/data/trace_msg_cacheXXXXXX"

Hmm,

I think I rather have an environment variable that can override the
default, and not just hardcode in paths that might work.

export TRACECMD_TEMPDIR="/data"

-- Steve

Re: [PATCH] trace-cmd: Try alternate path for message cache

[Steven Rostedt]

On Fri, 1 Apr 2022 15:50:10 -0400
Joel Fernandes <joel@joelfernandes.org> wrote:

> export TRACECMD_TEMPDIR="/data"
> 
> That’s fair. What about using memfd for this, do you feel that’s
> reasonable? I have not yet measured how big this file gets but if it’s
> small enough that might work too.

Is this a separate question? That is, do you mean using the above
environment variable *and* then use memfd?

I believe that the cache is used for passing the compressed data from the
guest to the host. I don't think it will be more than one compressed chunk.

But Tzvetomir would know better.

-- Steve

Re: [PATCH] trace-cmd: Try alternate path for message cache

[Joel Fernandes]

On Fri, Apr 01, 2022 at 07:06:29PM -0400, Steven Rostedt wrote:
> On Fri, 1 Apr 2022 15:50:10 -0400
> Joel Fernandes <joel@joelfernandes.org> wrote:
> 
> > export TRACECMD_TEMPDIR="/data"
> > 
> > That’s fair. What about using memfd for this, do you feel that’s
> > reasonable? I have not yet measured how big this file gets but if it’s
> > small enough that might work too.
> 
> Is this a separate question? That is, do you mean using the above
> environment variable *and* then use memfd?
> 
> I believe that the cache is used for passing the compressed data from the
> guest to the host. I don't think it will be more than one compressed chunk.
> 
> But Tzvetomir would know better.

Hey Steve,
No its the same question. Instead of temp file, I was proposing in-memory
file using memfd_create(2), that way no hassle as long as the file is not too
huge.

Also looks like my patch is incomplete anyway, I need to change
tracecmd_msg_handle_cache() as well.

I'll try to write up a patch with memfd unless you guys disagree.

Thanks,

- Joel

Re: [PATCH] trace-cmd: Try alternate path for message cache

[Joel Fernandes]

On Sun, Apr 03, 2022 at 02:56:12PM +0000, Joel Fernandes wrote:
> On Fri, Apr 01, 2022 at 07:06:29PM -0400, Steven Rostedt wrote:
> > On Fri, 1 Apr 2022 15:50:10 -0400
> > Joel Fernandes <joel@joelfernandes.org> wrote:
> > 
> > > export TRACECMD_TEMPDIR="/data"
> > > 
> > > That’s fair. What about using memfd for this, do you feel that’s
> > > reasonable? I have not yet measured how big this file gets but if it’s
> > > small enough that might work too.
> > 
> > Is this a separate question? That is, do you mean using the above
> > environment variable *and* then use memfd?
> > 
> > I believe that the cache is used for passing the compressed data from the
> > guest to the host. I don't think it will be more than one compressed chunk.
> > 
> > But Tzvetomir would know better.
> 
> Hey Steve,
> No its the same question. Instead of temp file, I was proposing in-memory
> file using memfd_create(2), that way no hassle as long as the file is not too
> huge.
> 
> Also looks like my patch is incomplete anyway, I need to change
> tracecmd_msg_handle_cache() as well.
> 
> I'll try to write up a patch with memfd unless you guys disagree.

Something like the following seems to work, and the file grows to only 4KB.
Note with memfd, the file name does not have to be unique and the fd entry
in the process denotes the file's uniqueness.

I'll roll it into a patch, let me know if you disagree:

---8<---

diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
index 6934376..3aee139 100644
--- a/lib/trace-cmd/include/private/trace-cmd-private.h
+++ b/lib/trace-cmd/include/private/trace-cmd-private.h
@@ -377,7 +377,6 @@ enum tracecmd_msg_flags {
 };
 
 /* for both client and server */
-#define MSG_CACHE_FILE "/tmp/trace_msg_cacheXXXXXX"
 struct tracecmd_msg_handle {
 	int			fd;
 	short			cpu_count;
@@ -386,7 +385,6 @@ struct tracecmd_msg_handle {
 	bool			done;
 	bool			cache;
 	int			cfd;
-	char			cfile[sizeof(MSG_CACHE_FILE)];
 };
 
 struct tracecmd_tsync_protos {
diff --git a/lib/trace-cmd/trace-msg.c b/lib/trace-cmd/trace-msg.c
index 03b853e..1472f20 100644
--- a/lib/trace-cmd/trace-msg.c
+++ b/lib/trace-cmd/trace-msg.c
@@ -593,11 +593,9 @@ tracecmd_msg_handle_alloc(int fd, unsigned long flags)
 int tracecmd_msg_handle_cache(struct tracecmd_msg_handle *msg_handle)
 {
 	if (msg_handle->cfd < 0) {
-		strcpy(msg_handle->cfile, MSG_CACHE_FILE);
-		msg_handle->cfd = mkstemp(msg_handle->cfile);
+		msg_handle->cfd = memfd_create("trace_msg_cache", 0);
 		if (msg_handle->cfd < 0)
 			return -1;
-		unlink(msg_handle->cfile);
 	}
 	msg_handle->cache = true;
 	return 0;

Re: [PATCH] trace-cmd: Try alternate path for message cache

[Steven Rostedt]

On Sun, 3 Apr 2022 15:24:33 +0000
Joel Fernandes <joel@joelfernandes.org> wrote:

> Something like the following seems to work, and the file grows to only 4KB.
> Note with memfd, the file name does not have to be unique and the fd entry
> in the process denotes the file's uniqueness.
> 
> I'll roll it into a patch, let me know if you disagree:

I'm OK with this, but I would really like a "Reviewed-by" from Tzvetomir.

Go ahead and send a real patch.

Thanks,

-- Steve

> 
> ---8<---
> 
> diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
> index 6934376..3aee139 100644
> --- a/lib/trace-cmd/include/private/trace-cmd-private.h
> +++ b/lib/trace-cmd/include/private/trace-cmd-private.h
> @@ -377,7 +377,6 @@ enum tracecmd_msg_flags {
>  };
>  
>  /* for both client and server */
> -#define MSG_CACHE_FILE "/tmp/trace_msg_cacheXXXXXX"
>  struct tracecmd_msg_handle {
>  	int			fd;
>  	short			cpu_count;
> @@ -386,7 +385,6 @@ struct tracecmd_msg_handle {
>  	bool			done;
>  	bool			cache;
>  	int			cfd;
> -	char			cfile[sizeof(MSG_CACHE_FILE)];
>  };
>  
>  struct tracecmd_tsync_protos {
> diff --git a/lib/trace-cmd/trace-msg.c b/lib/trace-cmd/trace-msg.c
> index 03b853e..1472f20 100644
> --- a/lib/trace-cmd/trace-msg.c
> +++ b/lib/trace-cmd/trace-msg.c
> @@ -593,11 +593,9 @@ tracecmd_msg_handle_alloc(int fd, unsigned long flags)
>  int tracecmd_msg_handle_cache(struct tracecmd_msg_handle *msg_handle)
>  {
>  	if (msg_handle->cfd < 0) {
> -		strcpy(msg_handle->cfile, MSG_CACHE_FILE);
> -		msg_handle->cfd = mkstemp(msg_handle->cfile);
> +		msg_handle->cfd = memfd_create("trace_msg_cache", 0);
>  		if (msg_handle->cfd < 0)
>  			return -1;
> -		unlink(msg_handle->cfile);
>  	}
>  	msg_handle->cache = true;
>  	return 0;

Re: [PATCH] trace-cmd: Try alternate path for message cache

[Tzvetomir Stoyanov]

On Sun, Apr 3, 2022 at 5:56 PM Joel Fernandes <joel@joelfernandes.org> wrote:
>
> On Fri, Apr 01, 2022 at 07:06:29PM -0400, Steven Rostedt wrote:
> > On Fri, 1 Apr 2022 15:50:10 -0400
> > Joel Fernandes <joel@joelfernandes.org> wrote:
> >
> > > export TRACECMD_TEMPDIR="/data"
> > >
> > > That’s fair. What about using memfd for this, do you feel that’s
> > > reasonable? I have not yet measured how big this file gets but if it’s
> > > small enough that might work too.
> >
> > Is this a separate question? That is, do you mean using the above
> > environment variable *and* then use memfd?
> >
> > I believe that the cache is used for passing the compressed data from the
> > guest to the host. I don't think it will be more than one compressed chunk.
> >
> > But Tzvetomir would know better.
>
> Hey Steve,
> No its the same question. Instead of temp file, I was proposing in-memory
> file using memfd_create(2), that way no hassle as long as the file is not too
> huge.
>

Hi  Joel,
That cache file is used for constructing the trace meta-data on the
guest, before sending it to the host. Usually it is compressed, but it
could be uncompressed in some cases (depending on the configuration) -
and in that case it can grow up to a few megabytes. Using memfd is ok
in most cases, but I'm wondering in the worst case - these few
megabytes could be a problem, especially if the guest runs with a
minimum amount of memory.

> Also looks like my patch is incomplete anyway, I need to change
> tracecmd_msg_handle_cache() as well.
>
> I'll try to write up a patch with memfd unless you guys disagree.
>
> Thanks,
>
> - Joel
>


-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center

Re: [PATCH] trace-cmd: Try alternate path for message cache

[Tzvetomir Stoyanov]

On Mon, Apr 4, 2022 at 7:41 AM Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:
>
> On Sun, Apr 3, 2022 at 5:56 PM Joel Fernandes <joel@joelfernandes.org> wrote:
> >
> > On Fri, Apr 01, 2022 at 07:06:29PM -0400, Steven Rostedt wrote:
> > > On Fri, 1 Apr 2022 15:50:10 -0400
> > > Joel Fernandes <joel@joelfernandes.org> wrote:
> > >
> > > > export TRACECMD_TEMPDIR="/data"
> > > >
> > > > That’s fair. What about using memfd for this, do you feel that’s
> > > > reasonable? I have not yet measured how big this file gets but if it’s
> > > > small enough that might work too.
> > >
> > > Is this a separate question? That is, do you mean using the above
> > > environment variable *and* then use memfd?
> > >
> > > I believe that the cache is used for passing the compressed data from the
> > > guest to the host. I don't think it will be more than one compressed chunk.
> > >
> > > But Tzvetomir would know better.
> >
> > Hey Steve,
> > No its the same question. Instead of temp file, I was proposing in-memory
> > file using memfd_create(2), that way no hassle as long as the file is not too
> > huge.
> >
>
> Hi  Joel,
> That cache file is used for constructing the trace meta-data on the
> guest, before sending it to the host. Usually it is compressed, but it
> could be uncompressed in some cases (depending on the configuration) -
> and in that case it can grow up to a few megabytes. Using memfd is ok
> in most cases, but I'm wondering in the worst case - these few
> megabytes could be a problem, especially if the guest runs with a
> minimum amount of memory.
>

Can you check that file size on your Android setup with that command,
it will force to not use compression on the guest trace file:
   trace-cmd <host trace options> -A <guest> <guest trace options>
--file-version 7 --compression none

Thanks!

> > Also looks like my patch is incomplete anyway, I need to change
> > tracecmd_msg_handle_cache() as well.
> >
> > I'll try to write up a patch with memfd unless you guys disagree.
> >
> > Thanks,
> >
> > - Joel
> >
>
>
> --
> Tzvetomir (Ceco) Stoyanov
> VMware Open Source Technology Center



-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center

Re: [PATCH] trace-cmd: Try alternate path for message cache

[Joel Fernandes]

On Mon, Apr 4, 2022 at 1:03 AM Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:
>
> On Mon, Apr 4, 2022 at 7:41 AM Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:
> >
> > On Sun, Apr 3, 2022 at 5:56 PM Joel Fernandes <joel@joelfernandes.org> wrote:
> > >
> > > On Fri, Apr 01, 2022 at 07:06:29PM -0400, Steven Rostedt wrote:
> > > > On Fri, 1 Apr 2022 15:50:10 -0400
> > > > Joel Fernandes <joel@joelfernandes.org> wrote:
> > > >
> > > > > export TRACECMD_TEMPDIR="/data"
> > > > >
> > > > > That’s fair. What about using memfd for this, do you feel that’s
> > > > > reasonable? I have not yet measured how big this file gets but if it’s
> > > > > small enough that might work too.
> > > >
> > > > Is this a separate question? That is, do you mean using the above
> > > > environment variable *and* then use memfd?
> > > >
> > > > I believe that the cache is used for passing the compressed data from the
> > > > guest to the host. I don't think it will be more than one compressed chunk.
> > > >
> > > > But Tzvetomir would know better.
> > >
> > > Hey Steve,
> > > No its the same question. Instead of temp file, I was proposing in-memory
> > > file using memfd_create(2), that way no hassle as long as the file is not too
> > > huge.
> > >
> >
> > Hi  Joel,
> > That cache file is used for constructing the trace meta-data on the
> > guest, before sending it to the host. Usually it is compressed, but it
> > could be uncompressed in some cases (depending on the configuration) -
> > and in that case it can grow up to a few megabytes. Using memfd is ok
> > in most cases, but I'm wondering in the worst case - these few
> > megabytes could be a problem, especially if the guest runs with a
> > minimum amount of memory.
> >
>
> Can you check that file size on your Android setup with that command,
> it will force to not use compression on the guest trace file:
>    trace-cmd <host trace options> -A <guest> <guest trace options>
> --file-version 7 --compression none

The file grows to 5.3MB with this. Is this really the common case
though? If not, I would still prefer memfd tbh. Is that Ok with you?

Thanks,

- Joel

Re: [PATCH] trace-cmd: Try alternate path for message cache

[Tzvetomir Stoyanov]

On Mon, Apr 4, 2022 at 4:21 PM Joel Fernandes <joel@joelfernandes.org> wrote:
>
> On Mon, Apr 4, 2022 at 1:03 AM Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:
> >
> > On Mon, Apr 4, 2022 at 7:41 AM Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:
> > >
> > > On Sun, Apr 3, 2022 at 5:56 PM Joel Fernandes <joel@joelfernandes.org> wrote:
> > > >
> > > > On Fri, Apr 01, 2022 at 07:06:29PM -0400, Steven Rostedt wrote:
> > > > > On Fri, 1 Apr 2022 15:50:10 -0400
> > > > > Joel Fernandes <joel@joelfernandes.org> wrote:
> > > > >
> > > > > > export TRACECMD_TEMPDIR="/data"
> > > > > >
> > > > > > That’s fair. What about using memfd for this, do you feel that’s
> > > > > > reasonable? I have not yet measured how big this file gets but if it’s
> > > > > > small enough that might work too.
> > > > >
> > > > > Is this a separate question? That is, do you mean using the above
> > > > > environment variable *and* then use memfd?
> > > > >
> > > > > I believe that the cache is used for passing the compressed data from the
> > > > > guest to the host. I don't think it will be more than one compressed chunk.
> > > > >
> > > > > But Tzvetomir would know better.
> > > >
> > > > Hey Steve,
> > > > No its the same question. Instead of temp file, I was proposing in-memory
> > > > file using memfd_create(2), that way no hassle as long as the file is not too
> > > > huge.
> > > >
> > >
> > > Hi  Joel,
> > > That cache file is used for constructing the trace meta-data on the
> > > guest, before sending it to the host. Usually it is compressed, but it
> > > could be uncompressed in some cases (depending on the configuration) -
> > > and in that case it can grow up to a few megabytes. Using memfd is ok
> > > in most cases, but I'm wondering in the worst case - these few
> > > megabytes could be a problem, especially if the guest runs with a
> > > minimum amount of memory.
> > >
> >
> > Can you check that file size on your Android setup with that command,
> > it will force to not use compression on the guest trace file:
> >    trace-cmd <host trace options> -A <guest> <guest trace options>
> > --file-version 7 --compression none
>
> The file grows to 5.3MB with this. Is this really the common case
> though? If not, I would still prefer memfd tbh. Is that Ok with you?
>

There are two cases that could hit this:
 1. Using a " --compression none" flag on the guest file. We could
disable that flag and force trace file v7 always to use compression. I
cannot imagine a use case for uncompressed trace file, maybe only for
debug purposes ?
 2. If, for some reason, there are no supported compression libraries
on the guest. Trace-cmd supports libz and libzstd, I believe both are
widely available. Theoretically it could be possible to have some
minimalistic VM image without any of these libraries.
What are the advantages of using memfd instead of temp file, is it
only for performance ? Usually collecting trace metadata is not
performance critical, as it is done before starting the trace.
I think that the best solution is to use memfd in case the guest file
is compressed and switch back to a temp file in case of a
not-compressed file.


> Thanks,
>
> - Joel



-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center

Re: [PATCH] trace-cmd: Try alternate path for message cache

[Joel Fernandes]

Hi Tzvetomir,

On Mon, Apr 4, 2022 at 9:48 AM Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:
>
> On Mon, Apr 4, 2022 at 4:21 PM Joel Fernandes <joel@joelfernandes.org> wrote:
[..]
> > > > >
> > > > > On Fri, Apr 01, 2022 at 07:06:29PM -0400, Steven Rostedt wrote:
> > > > > > On Fri, 1 Apr 2022 15:50:10 -0400
> > > > > > Joel Fernandes <joel@joelfernandes.org> wrote:
> > > > > >
> > > > > > > export TRACECMD_TEMPDIR="/data"
> > > > > > >
> > > > > > > That’s fair. What about using memfd for this, do you feel that’s
> > > > > > > reasonable? I have not yet measured how big this file gets but if it’s
> > > > > > > small enough that might work too.
> > > > > >
> > > > > > Is this a separate question? That is, do you mean using the above
> > > > > > environment variable *and* then use memfd?
> > > > > >
> > > > > > I believe that the cache is used for passing the compressed data from the
> > > > > > guest to the host. I don't think it will be more than one compressed chunk.
> > > > > >
> > > > > > But Tzvetomir would know better.
> > > > >
> > > > > Hey Steve,
> > > > > No its the same question. Instead of temp file, I was proposing in-memory
> > > > > file using memfd_create(2), that way no hassle as long as the file is not too
> > > > > huge.
> > > > >
> > > >
> > > > Hi  Joel,
> > > > That cache file is used for constructing the trace meta-data on the
> > > > guest, before sending it to the host. Usually it is compressed, but it
> > > > could be uncompressed in some cases (depending on the configuration) -
> > > > and in that case it can grow up to a few megabytes. Using memfd is ok
> > > > in most cases, but I'm wondering in the worst case - these few
> > > > megabytes could be a problem, especially if the guest runs with a
> > > > minimum amount of memory.
> > > >
> > >
> > > Can you check that file size on your Android setup with that command,
> > > it will force to not use compression on the guest trace file:
> > >    trace-cmd <host trace options> -A <guest> <guest trace options>
> > > --file-version 7 --compression none
> >
> > The file grows to 5.3MB with this. Is this really the common case
> > though? If not, I would still prefer memfd tbh. Is that Ok with you?
> >
>
> There are two cases that could hit this:
>  1. Using a " --compression none" flag on the guest file. We could
> disable that flag and force trace file v7 always to use compression. I
> cannot imagine a use case for uncompressed trace file, maybe only for
> debug purposes ?
>  2. If, for some reason, there are no supported compression libraries
> on the guest. Trace-cmd supports libz and libzstd, I believe both are
> widely available. Theoretically it could be possible to have some
> minimalistic VM image without any of these libraries.
> What are the advantages of using memfd instead of temp file, is it
> only for performance ? Usually collecting trace metadata is not
> performance critical, as it is done before starting the trace.
> I think that the best solution is to use memfd in case the guest file
> is compressed and switch back to a temp file in case of a
> not-compressed file.

Thanks for the background. The only reason for my use of memfd is to
do away with absolute file paths and temp directories, while fixing
the problem and keeping the code simple.

Steve, any opinions on this?

Thanks,

Joel

Re: [PATCH] trace-cmd: Try alternate path for message cache

[Steven Rostedt]

On Mon, 4 Apr 2022 16:48:11 +0300
Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:

> > > > Hi  Joel,
> > > > That cache file is used for constructing the trace meta-data on the
> > > > guest, before sending it to the host. Usually it is compressed, but it
> > > > could be uncompressed in some cases (depending on the configuration) -
> > > > and in that case it can grow up to a few megabytes. Using memfd is ok
> > > > in most cases, but I'm wondering in the worst case - these few
> > > > megabytes could be a problem, especially if the guest runs with a
> > > > minimum amount of memory.
> > > >  
> > >
> > > Can you check that file size on your Android setup with that command,
> > > it will force to not use compression on the guest trace file:
> > >    trace-cmd <host trace options> -A <guest> <guest trace options>
> > > --file-version 7 --compression none  
> >
> > The file grows to 5.3MB with this. Is this really the common case
> > though? If not, I would still prefer memfd tbh. Is that Ok with you?

This is meta data right? Which means everything in here is in kernel memory
anyway. kallsyms, events, etc. I do not believe that this will be an issue
even uncompressed.


> >  
> 
> There are two cases that could hit this:
>  1. Using a " --compression none" flag on the guest file. We could
> disable that flag and force trace file v7 always to use compression. I
> cannot imagine a use case for uncompressed trace file, maybe only for
> debug purposes ?

Please no. I do have some machines that do not have zlib installed. I do
not want to make it a requirement to have zlib for this use case. If we do
not have memory, we could fall back to mktemp.

>  2. If, for some reason, there are no supported compression libraries
> on the guest. Trace-cmd supports libz and libzstd, I believe both are
> widely available. Theoretically it could be possible to have some
> minimalistic VM image without any of these libraries.

Not theoretical ;-)

> What are the advantages of using memfd instead of temp file, is it
> only for performance ? Usually collecting trace metadata is not
> performance critical, as it is done before starting the trace.
> I think that the best solution is to use memfd in case the guest file
> is compressed and switch back to a temp file in case of a
> not-compressed file.

Yeah, we could use memfd if compressed and temp if not. Luckily, this isn't
something exposed across the API so we should be able to experiment with
various implementations, and if people complain about one (like Joel did
for Android), we can try something else.

-- Steve

Re: [PATCH] trace-cmd: Try alternate path for message cache

[Tzvetomir Stoyanov]

On Mon, Apr 4, 2022 at 5:35 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Mon, 4 Apr 2022 16:48:11 +0300
> Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:
>
> > > > > Hi  Joel,
> > > > > That cache file is used for constructing the trace meta-data on the
> > > > > guest, before sending it to the host. Usually it is compressed, but it
> > > > > could be uncompressed in some cases (depending on the configuration) -
> > > > > and in that case it can grow up to a few megabytes. Using memfd is ok
> > > > > in most cases, but I'm wondering in the worst case - these few
> > > > > megabytes could be a problem, especially if the guest runs with a
> > > > > minimum amount of memory.
> > > > >
> > > >
> > > > Can you check that file size on your Android setup with that command,
> > > > it will force to not use compression on the guest trace file:
> > > >    trace-cmd <host trace options> -A <guest> <guest trace options>
> > > > --file-version 7 --compression none
> > >
> > > The file grows to 5.3MB with this. Is this really the common case
> > > though? If not, I would still prefer memfd tbh. Is that Ok with you?
>
> This is meta data right? Which means everything in here is in kernel memory
> anyway. kallsyms, events, etc. I do not believe that this will be an issue
> even uncompressed.
>
>
> > >
> >
> > There are two cases that could hit this:
> >  1. Using a " --compression none" flag on the guest file. We could
> > disable that flag and force trace file v7 always to use compression. I
> > cannot imagine a use case for uncompressed trace file, maybe only for
> > debug purposes ?
>
> Please no. I do have some machines that do not have zlib installed. I do
> not want to make it a requirement to have zlib for this use case. If we do
> not have memory, we could fall back to mktemp.

Hi Steven,
I'm wondering how you run the latest trace-cmd on those machines ? As
these libraries are checked at compile time, this is a compile time
dependency - so the trace-cmd compiled with zlib support should not be
able to run there ? That's why I think   " --compression none" could
be used only for debugging.
We could implement loading compression libraries dynamically to avoid
these compile time dependencyies, or add a compile flag to disable
compression support ?

>
> >  2. If, for some reason, there are no supported compression libraries
> > on the guest. Trace-cmd supports libz and libzstd, I believe both are
> > widely available. Theoretically it could be possible to have some
> > minimalistic VM image without any of these libraries.
>
> Not theoretical ;-)
>
> > What are the advantages of using memfd instead of temp file, is it
> > only for performance ? Usually collecting trace metadata is not
> > performance critical, as it is done before starting the trace.
> > I think that the best solution is to use memfd in case the guest file
> > is compressed and switch back to a temp file in case of a
> > not-compressed file.
>
> Yeah, we could use memfd if compressed and temp if not. Luckily, this isn't
> something exposed across the API so we should be able to experiment with
> various implementations, and if people complain about one (like Joel did
> for Android), we can try something else.
>
> -- Steve



-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center

Re: [PATCH] trace-cmd: Try alternate path for message cache

[Steven Rostedt]

On Mon, 4 Apr 2022 17:48:24 +0300
Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:

> On Mon, Apr 4, 2022 at 5:35 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On Mon, 4 Apr 2022 16:48:11 +0300
> > Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:
> >  
> > > > > > Hi  Joel,
> > > > > > That cache file is used for constructing the trace meta-data on the
> > > > > > guest, before sending it to the host. Usually it is compressed, but it
> > > > > > could be uncompressed in some cases (depending on the configuration) -
> > > > > > and in that case it can grow up to a few megabytes. Using memfd is ok
> > > > > > in most cases, but I'm wondering in the worst case - these few
> > > > > > megabytes could be a problem, especially if the guest runs with a
> > > > > > minimum amount of memory.
> > > > > >  
> > > > >
> > > > > Can you check that file size on your Android setup with that command,
> > > > > it will force to not use compression on the guest trace file:
> > > > >    trace-cmd <host trace options> -A <guest> <guest trace options>
> > > > > --file-version 7 --compression none  
> > > >
> > > > The file grows to 5.3MB with this. Is this really the common case
> > > > though? If not, I would still prefer memfd tbh. Is that Ok with you?  
> >
> > This is meta data right? Which means everything in here is in kernel memory
> > anyway. kallsyms, events, etc. I do not believe that this will be an issue
> > even uncompressed.
> >
> >  
> > > >  
> > >
> > > There are two cases that could hit this:
> > >  1. Using a " --compression none" flag on the guest file. We could
> > > disable that flag and force trace file v7 always to use compression. I
> > > cannot imagine a use case for uncompressed trace file, maybe only for
> > > debug purposes ?  
> >
> > Please no. I do have some machines that do not have zlib installed. I do
> > not want to make it a requirement to have zlib for this use case. If we do
> > not have memory, we could fall back to mktemp.  
> 
> Hi Steven,
> I'm wondering how you run the latest trace-cmd on those machines ? As
> these libraries are checked at compile time, this is a compile time
> dependency - so the trace-cmd compiled with zlib support should not be

It was compiled on the machine it ran on. I have to find which machine it
was. I believe it was one of the gentoo or arch VMs that have very little
installed. Bare minimum (you have to compile everything that is on it). But
the trace-cmd I built did not have any compression support, which put it to
--compression none by default. Without any compression libraries, it can
still build.

> able to run there ? That's why I think   " --compression none" could
> be used only for debugging.
> We could implement loading compression libraries dynamically to avoid
> these compile time dependencyies, or add a compile flag to disable
> compression support ?

That is not needed. Do we really need to have compression always?

-- Steve

Re: [PATCH] trace-cmd: Try alternate path for message cache

[Tzvetomir Stoyanov]

On Mon, Apr 4, 2022 at 6:04 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Mon, 4 Apr 2022 17:48:24 +0300
> Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:
>
> > On Mon, Apr 4, 2022 at 5:35 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> > >
> > > On Mon, 4 Apr 2022 16:48:11 +0300
> > > Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:
> > >
> > > > > > > Hi  Joel,
> > > > > > > That cache file is used for constructing the trace meta-data on the
> > > > > > > guest, before sending it to the host. Usually it is compressed, but it
> > > > > > > could be uncompressed in some cases (depending on the configuration) -
> > > > > > > and in that case it can grow up to a few megabytes. Using memfd is ok
> > > > > > > in most cases, but I'm wondering in the worst case - these few
> > > > > > > megabytes could be a problem, especially if the guest runs with a
> > > > > > > minimum amount of memory.
> > > > > > >
> > > > > >
> > > > > > Can you check that file size on your Android setup with that command,
> > > > > > it will force to not use compression on the guest trace file:
> > > > > >    trace-cmd <host trace options> -A <guest> <guest trace options>
> > > > > > --file-version 7 --compression none
> > > > >
> > > > > The file grows to 5.3MB with this. Is this really the common case
> > > > > though? If not, I would still prefer memfd tbh. Is that Ok with you?
> > >
> > > This is meta data right? Which means everything in here is in kernel memory
> > > anyway. kallsyms, events, etc. I do not believe that this will be an issue
> > > even uncompressed.
> > >
> > >
> > > > >
> > > >
> > > > There are two cases that could hit this:
> > > >  1. Using a " --compression none" flag on the guest file. We could
> > > > disable that flag and force trace file v7 always to use compression. I
> > > > cannot imagine a use case for uncompressed trace file, maybe only for
> > > > debug purposes ?
> > >
> > > Please no. I do have some machines that do not have zlib installed. I do
> > > not want to make it a requirement to have zlib for this use case. If we do
> > > not have memory, we could fall back to mktemp.
> >
> > Hi Steven,
> > I'm wondering how you run the latest trace-cmd on those machines ? As
> > these libraries are checked at compile time, this is a compile time
> > dependency - so the trace-cmd compiled with zlib support should not be
>
> It was compiled on the machine it ran on. I have to find which machine it
> was. I believe it was one of the gentoo or arch VMs that have very little
> installed. Bare minimum (you have to compile everything that is on it). But
> the trace-cmd I built did not have any compression support, which put it to
> --compression none by default. Without any compression libraries, it can
> still build.
>
> > able to run there ? That's why I think   " --compression none" could
> > be used only for debugging.
> > We could implement loading compression libraries dynamically to avoid
> > these compile time dependencyies, or add a compile flag to disable
> > compression support ?
>
> That is not needed. Do we really need to have compression always?
>

The current approach to use the best available compression algorithm
by default is good enough, but personally I prefer the logic with
dynamically loading of these libraries. That way we can use the same
binary, which will simplify the trace-cmd packaging. And the first PoC
implementation of compression support was that way :)

> -- Steve



-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center

Re: [PATCH] trace-cmd: Try alternate path for message cache

[Steven Rostedt]

On Mon, 4 Apr 2022 18:15:42 +0300
Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:

> The current approach to use the best available compression algorithm
> by default is good enough, but personally I prefer the logic with
> dynamically loading of these libraries. That way we can use the same
> binary, which will simplify the trace-cmd packaging. And the first PoC
> implementation of compression support was that way :)

I disagree. How does it make it easier for packaging? Then you have the
nightmare of having different compression algorithms on different machines
depending on what is installed at run time. Because we pass around
trace.dat files, and now you may not be able to read it if the other
machine doesn't support it. I already hit this, and it is annoying.

If the compressions are compile time dependent, then they will likely be
added in the distro packaging. Which is what we want. That way, when people
are not compiling their own trace-cmd, then all the trace-cmd of the same
version they have will be able to create and read the trace.dat files that
are passed around.

If you make it a run time dependency, it will be a nightmare to keep it
straight for the average user. And I don't want to deal with bug reports
saying "I ran trace-cmd v3.1.3 on one machine, and trace-cmd v3.1.3 on
another machine can't read the file".

Sure, if you compile it yourself, it may not be supported (which is what I
hit), but at least we guarantee that it will be supported across machines
using the same distro if we make it compile time dependent.

-- Steve

Re: [PATCH] trace-cmd: Try alternate path for message cache

[Steven Rostedt]

On Mon, 4 Apr 2022 11:27:58 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> Sure, if you compile it yourself, it may not be supported (which is what I
> hit), but at least we guarantee that it will be supported across machines
> using the same distro if we make it compile time dependent.

Not to mention, I have use cases where trace-cmd needs to be installed in
an environment with no dynamic libraries (nor using dlopen). Everything
will be built statically. trace-cmd report will not be used, but trace-cmd
agent will be.

Remember, trace-cmd is to be embedded friendly.

-- Steve

Re: [PATCH] trace-cmd: Try alternate path for message cache

[Joel Fernandes]

On Mon, Apr 4, 2022 at 11:32 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Mon, 4 Apr 2022 11:27:58 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > Sure, if you compile it yourself, it may not be supported (which is what I
> > hit), but at least we guarantee that it will be supported across machines
> > using the same distro if we make it compile time dependent.
>
> Not to mention, I have use cases where trace-cmd needs to be installed in
> an environment with no dynamic libraries (nor using dlopen). Everything
> will be built statically. trace-cmd report will not be used, but trace-cmd
> agent will be.
>
> Remember, trace-cmd is to be embedded friendly.

Agreed, so should I send a patch next to do memfd unless
not-compressed? Is there an easy way to do not-compressed? Feel free
to rewrite my patch as well if doing this is trivial, I'm actually OOO
today for jury duty.

Thanks,

- Joel

Re: [PATCH] trace-cmd: Try alternate path for message cache

[Joel Fernandes]

On Mon, Apr 4, 2022 at 11:40 AM Joel Fernandes <joel@joelfernandes.org> wrote:
>
> On Mon, Apr 4, 2022 at 11:32 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On Mon, 4 Apr 2022 11:27:58 -0400
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > > Sure, if you compile it yourself, it may not be supported (which is what I
> > > hit), but at least we guarantee that it will be supported across machines
> > > using the same distro if we make it compile time dependent.
> >
> > Not to mention, I have use cases where trace-cmd needs to be installed in
> > an environment with no dynamic libraries (nor using dlopen). Everything
> > will be built statically. trace-cmd report will not be used, but trace-cmd
> > agent will be.
> >
> > Remember, trace-cmd is to be embedded friendly.
>
> Agreed, so should I send a patch next to do memfd unless
> not-compressed? Is there an easy way to do not-compressed?

Sorry I meant "is there an easy way to detect, in this code path, that
we are running in not compressed mode" ? I guess I could just check
the argv but I haven't looked closely yet.

Thanks,

 - Joel

Re: [PATCH] trace-cmd: Try alternate path for message cache

[Steven Rostedt]

On Mon, 4 Apr 2022 11:41:58 -0400
Joel Fernandes <joel@joelfernandes.org> wrote:

> > Agreed, so should I send a patch next to do memfd unless
> > not-compressed? Is there an easy way to do not-compressed?  
> 
> Sorry I meant "is there an easy way to detect, in this code path, that
> we are running in not compressed mode" ? I guess I could just check
> the argv but I haven't looked closely yet.

From that function not yet. We could add a flag.

I think we could just take this patch and then I could add a way to put
back the mktemp in compression = none mode.

I rather have it broken up into multiple patches anyway.

-- Steve

Re: [PATCH] trace-cmd: Try alternate path for message cache

[Joel Fernandes]

On Mon, Apr 4, 2022 at 11:47 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Mon, 4 Apr 2022 11:41:58 -0400
> Joel Fernandes <joel@joelfernandes.org> wrote:
>
> > > Agreed, so should I send a patch next to do memfd unless
> > > not-compressed? Is there an easy way to do not-compressed?
> >
> > Sorry I meant "is there an easy way to detect, in this code path, that
> > we are running in not compressed mode" ? I guess I could just check
> > the argv but I haven't looked closely yet.
>
> From that function not yet. We could add a flag.
>
> I think we could just take this patch and then I could add a way to put
> back the mktemp in compression = none mode.
>
> I rather have it broken up into multiple patches anyway.

That sounds good to me, thanks.

 - Joel