| Message ID | 20220329191801.429691-1-joel@joelfernandes.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | trace-cmd: Try alternate path for message cache | expand |
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
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
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
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;
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;
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 >
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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;
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(-)