Re: [lttng-dev] [LTTng-modules PATCH] The msg part of the printk:console event was 1 character too long
From: Mathieu Desnoyers [mathieu.desnoy...@efficios.com] Sent: Monday, July 01, 2013 00:20 To: Yannick Brosseau; Gabbasov, Andrew Cc: lttng-dev@lists.lttng.org Subject: Re: [lttng-dev] [LTTng-modules PATCH] The msg part of the printk:console event was 1 character too long * Yannick Brosseau (yannick.bross...@gmail.com) wrote: On 2013-06-28 15:43, Mathieu Desnoyers wrote: * Yannick Brosseau (yannick.bross...@gmail.com) wrote: Signed-off-by: Yannick Brosseau yannick.bross...@gmail.com --- instrumentation/events/lttng-module/printk.h |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/events/lttng-module/printk.h b/instrumentation/events/lttng-module/printk.h index 4c744f9..f4b6028 100644 --- a/instrumentation/events/lttng-module/printk.h +++ b/instrumentation/events/lttng-module/printk.h @@ -19,7 +19,7 @@ TRACE_EVENT_CONDITION(console, TP_STRUCT__entry( __dynamic_array_text(char, msg, - min_t(unsigned, end - start, MSG_TRACE_MAX_LEN) + 1) this was taken from the Linux kernel mainline instrumentation, no ? Has this been fixed upstream ? If so, can you cite the commit in the changelog ? It's not changed upstream. From what I see, ftrace does the right thing, but not LTTng. Will check perf and see if I can reproduce the problem directly with the upstream kernel. in 3.9.8, I see: TP_STRUCT__entry( __dynamic_array(char, msg, end - start + 1) ), (upstream) which has the +1. What effect of the off-by-one are you observing exactly ? Are you sure your fix is the right fix ? I wonder if modifying the TP_fast_assign() code would not be better. I'm not sure why #if (LINUX_VERSION_CODE = KERNEL_VERSION(3,5,0)) has a different code from mainline. CCing Andrew Gabbasov who contributed this instrumentation. Thanks, Mathieu Hi Mathieu, Yannick, Actually I also don't see why having +1 is wrong. Trailing zero byte needs it in the message length. Removing it in entry and not changing TP_fast_assign (that explicitly adds a zero byte) looks doubtful. I join to the question what the exact problem is, since my testing (at least on version 3.5) didn't show any problem. As for the different implementation code: starting version 3.5 the kernel no longer use circular buffer for storing printk messages, and handling the message in 2 pieces what it crosses the boundary is no longer needed. Moreover, log_buf_len becomes not necessarily the power of 2, and previous implementation (based on what was in the kernel) just becomes incorrect. So, the newer and simpler implementation was introduced for later version. It also looks like this instrumentation needs to be further updated for version 3.10, since the whole tracepoint (even the parameters set) was changed there, becoming even more simple. Thanks. Best regards, Andrew ___ lttng-dev mailing list lttng-dev@lists.lttng.org http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
Re: [lttng-dev] LTTng: global_dirty_limit symbol lookup failed
From: Gabbasov, Andrew Sent: Monday, February 18, 2013 14:14 To: Mathieu Desnoyers Cc: lttng-dev@lists.lttng.org Subject: RE: LTTng: global_dirty_limit symbol lookup failed Hi Andrew, Commit: commit b87700e318c27267890cbd6fb5e50b687279131b Author: Andrew Gabbasov andrew_gabba...@mentor.com Date: Mon Dec 10 11:14:52 2012 -0500 Add new kernel probes instrumentation Add kernel probes for btrfs, compaction, ext4, printk, random, rcu, regmap, rpm, sunrpc, workqueue, writeback. Signed-off-by: Andrew Gabbasov andrew_gabba...@mentor.com Signed-off-by: Mathieu Desnoyers mathieu.desnoy...@efficios.com has 2 issues: 1) it implements a wrapper around global_dirty_limit() in probes/. All wrappers should be in /wrapper/, without exception, otherwise the code will quickly become unmaintainable (we need to know what wrappers we can eventually remove if we remove support for older kernels in the future, and we need to be aware of those wrappers for complete testing coverage). I have no problem with having a probes/Makefile using a relative path (e.g. ../wrapper/somefile.o) as input if needed. 2) It fills my dmesg log output with: LTTng: global_dirty_limit symbol lookup failed. messages. .config follows. Can you look into those issues ? Thanks, Mathieu Hi Mathieu, Sorry for a late response. I'm sending a separate e-mail with the patch to fix these issues. Here are some details: 1) I moved the wrapper to a separate file wrapper/writeback.h. Since it is supposed to be used in writeback probe only, I think, using static variable will be harmless. 2) Since global_dirty_limit is not a function, but a global variable, its address is available to kallsyms lookup only if CONFIG_KALLSYMS_ALL is set. In order not to introduce wrapper related checks of this config into tracepoint definition include file, I decided not to exclude separate tracepoints in case of this config absence, but to omit the whole probe from building. So, writeback probe will be available only if CONFIG_KALLSYMS_ALL is set. 3) Additional issue that I noticed: in case of CONFIG_THUMB2_KERNEL setting for ARM, kallsyms_lookup_funcptr modifies the address found for the symbol. In order to avoid it for data addresses, I introduced a new kallsyms_lookup_dataptr function, that does not do any address modifications. And this new function is used for global_dirty_limit wrapper. Thanks. Best regards, Andrew Hi Mathieu, Any thoughts on these changes? Do you need anything else regarding it (the patch itself was sent separately on Feb 18, 2013, with a subject [PATCH lttng-modules] Clean up using global_dirty_limit wrapper for writeback probe). Thanks. Best regards, Andrew ___ lttng-dev mailing list lttng-dev@lists.lttng.org http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
Re: [lttng-dev] LTTng: global_dirty_limit symbol lookup failed
Hi Andrew, Commit: commit b87700e318c27267890cbd6fb5e50b687279131b Author: Andrew Gabbasov andrew_gabba...@mentor.com Date: Mon Dec 10 11:14:52 2012 -0500 Add new kernel probes instrumentation Add kernel probes for btrfs, compaction, ext4, printk, random, rcu, regmap, rpm, sunrpc, workqueue, writeback. Signed-off-by: Andrew Gabbasov andrew_gabba...@mentor.com Signed-off-by: Mathieu Desnoyers mathieu.desnoy...@efficios.com has 2 issues: 1) it implements a wrapper around global_dirty_limit() in probes/. All wrappers should be in /wrapper/, without exception, otherwise the code will quickly become unmaintainable (we need to know what wrappers we can eventually remove if we remove support for older kernels in the future, and we need to be aware of those wrappers for complete testing coverage). I have no problem with having a probes/Makefile using a relative path (e.g. ../wrapper/somefile.o) as input if needed. 2) It fills my dmesg log output with: LTTng: global_dirty_limit symbol lookup failed. messages. .config follows. Can you look into those issues ? Thanks, Mathieu Hi Mathieu, Sorry for a late response. I'm sending a separate e-mail with the patch to fix these issues. Here are some details: 1) I moved the wrapper to a separate file wrapper/writeback.h. Since it is supposed to be used in writeback probe only, I think, using static variable will be harmless. 2) Since global_dirty_limit is not a function, but a global variable, its address is available to kallsyms lookup only if CONFIG_KALLSYMS_ALL is set. In order not to introduce wrapper related checks of this config into tracepoint definition include file, I decided not to exclude separate tracepoints in case of this config absence, but to omit the whole probe from building. So, writeback probe will be available only if CONFIG_KALLSYMS_ALL is set. 3) Additional issue that I noticed: in case of CONFIG_THUMB2_KERNEL setting for ARM, kallsyms_lookup_funcptr modifies the address found for the symbol. In order to avoid it for data addresses, I introduced a new kallsyms_lookup_dataptr function, that does not do any address modifications. And this new function is used for global_dirty_limit wrapper. Thanks. Best regards, Andrew ___ lttng-dev mailing list lttng-dev@lists.lttng.org http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
Re: [lttng-dev] [PATCH lttng-modules] wrapper/perf.h: Fix kernel version condition
Hi Mathieu,
I was looking at stable versions when setting all those versions ifdef's.
Stable 2.6.36.x has old parameter (pid_t), while 2.6.37.x already has new
parameter
(struct task_struct *). That's why the checking statement with = should have
2.6.37.
For example, see
http://git.kernel.org/?p=linux/kernel/git/stable/linux-stable.git;a=blob;f=include/linux/perf_event.h;h=1d42c6ecd00a3d0b885c84f36e0c38dca976eee4;hb=refs/heads/linux-2.6.36.y
line 871. This is the file in latest 2.6.36.y version, that has old pid_t
parameter
in perf_event_create_kernel_counter() function, so this change was introduced
later than in 2.6.36.
Thanks.
Best regards,
Andrew
From: Mathieu Desnoyers [mathieu.desnoy...@efficios.com]
Sent: Monday, December 10, 2012 20:05
To: Gabbasov, Andrew
Cc: lttng-dev@lists.lttng.org
Subject: Re: [lttng-dev] [PATCH lttng-modules] wrapper/perf.h: Fix kernel
version condition
* Andrew Gabbasov (andrew_gabba...@mentor.com) wrote:
The pid_t parameter of function perf_event_create_kernel_counter
was changed to task_struct pointer starting from 2.6.37.x.
Did you check if 2.6.36.x stable kernels also introduced this change ?
This might explain why in my own testing I did put 2.6.36 as kernel
version cutoff.
Thanks,
Mathieu
Signed-off-by: Andrew Gabbasov andrew_gabba...@mentor.com
---
wrapper/perf.h |9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/wrapper/perf.h b/wrapper/perf.h
index 5dfa84b..5de205c 100644
--- a/wrapper/perf.h
+++ b/wrapper/perf.h
@@ -34,7 +34,7 @@ wrapper_perf_event_create_kernel_counter(struct
perf_event_attr *attr,
}
#else /* defined(CONFIG_PERF_EVENTS) (LINUX_VERSION_CODE =
KERNEL_VERSION(3,0,99)) */
-#if (LINUX_VERSION_CODE = KERNEL_VERSION(2,6,36))
+#if (LINUX_VERSION_CODE = KERNEL_VERSION(2,6,37))
static inline struct perf_event *
wrapper_perf_event_create_kernel_counter(struct perf_event_attr *attr,
int cpu,
@@ -43,7 +43,7 @@ wrapper_perf_event_create_kernel_counter(struct
perf_event_attr *attr,
{
return perf_event_create_kernel_counter(attr, cpu, task, callback);
}
-#else /* (LINUX_VERSION_CODE = KERNEL_VERSION(2,6,36)) */
+#else /* (LINUX_VERSION_CODE = KERNEL_VERSION(2,6,37)) */
static inline struct perf_event *
wrapper_perf_event_create_kernel_counter(struct perf_event_attr *attr,
int cpu,
@@ -60,8 +60,11 @@ wrapper_perf_event_create_kernel_counter(struct
perf_event_attr *attr,
return perf_event_create_kernel_counter(attr, cpu, pid, callback);
}
+#endif /* (LINUX_VERSION_CODE = KERNEL_VERSION(2,6,37)) */
+
+#if (LINUX_VERSION_CODE KERNEL_VERSION(2,6,36))
#define local64_read(l) atomic64_read(l)
-#endif /* (LINUX_VERSION_CODE = KERNEL_VERSION(2,6,36)) */
+#endif
#endif /* defined(CONFIG_PERF_EVENTS) (LINUX_VERSION_CODE =
KERNEL_VERSION(3,0,99)) */
--
1.7.10.4
___
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
--
Mathieu Desnoyers
Operating System Efficiency RD Consultant
EfficiOS Inc.
http://www.efficios.com
___
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
Re: [lttng-dev] [PATCH lttng-modules] Update kernel probes to more detailed match to kernel versions
Hi Mathieu, By the way, we're planning to go for a 2.1 release in a matter of days. At that point, I'll have to decide if we keep the recent instrumentation commits introduced after the start of the -rc cycle in this release (if mature enough), or if we should branch before those for the 2.1 release if they need more testing (and keep these instrumentation extensions for 2.2). Any thoughts on this ? I believe, it would be desirable to make more thorough testing of new instrumentations. On our side the testing was not too extensive, and was done for ARM platform only. Although the probes are mostly replicating the kernel's tracepoints, it would be reasonable to keep the commits in a branch for some time so that more people in the community could make some testing. The other team here at Mentor Graphics, that originally created this set of tracepoints instrumentation, submitted them to the list quite long ago. Also, they actually did an extensive testing (much more than I was aware of), including other platforms (e.g. PowerPC). May be it's not too late to still include at least the original implementation of tracepoints (may be with some later fixes) to upcoming 2.1 release? Having new supported fixed kernel tracepoints could be a good accomplishment for the release. Thanks. Best regards, Andrew Gabbasov ___ lttng-dev mailing list lttng-dev@lists.lttng.org http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
Re: [lttng-dev] [PATCH lttng-modules] Update kernel probes to more detailed match to kernel versions
Hi Mathieu, * Gabbasov, Andrew (andrew_gabba...@mentor.com) wrote: Hi Mathieu, * Andrew Gabbasov (andrew_gabba...@mentor.com) wrote: We have added some more ifdef's to kernel probes to more closely match the kernel tracepoints for different kernel versions. The changes cover kernel versions from 2.6.38 up to 3.7. We did not track the earlier versions, and they are filtered out in probes Makefile. ext3 probe was modified to use #include ../fs/ext3/ext3.h instead of copying this include from kernel source to lttng-modules source. Using such includes (there will be more similar in other probes) imposes a restriction to use the full kernel source for building lttng-modules rather than having just kernel include directory, but it looks like full source is required anyway, at least with respect to Makefiles structure. May be it is worth mentioning somewhere in the documentation. The code was verified to compile with latest versions in all stable branches from 2.6.38.x to 3.6.x and 3.7-rc7. I get this build failure on 3.5 with this patch: CC [M] /home/compudj/work/lttng-modules/probes/lttng-probe-ext3.o /home/compudj/work/lttng-modules/probes/lttng-probe-ext3.c:30:29: fatal error: ../fs/ext3/ext3.h: No such file or directory compilation terminated. can you look into it and submit a revised patch ? You must be using kernel headers for building lttng-modules (linux-headers package or something like that), right? This is what I was writing about: since ext3.h is not available in kernel's include directory, and in order to avoid duplicating this file in lttng-modules sources, we have to use that ../ based reference, and this requires having full kernel source for building. The same issue will later be with btrfs and ext4 probes: their necessary include files are also not available in kernel headers. Trying to duplicate these header files to lttng-modules source seems to be not a good idea, especially if we need to add many ifdef's to them to track different kernel versions. We can try to insert some checks to the Makefile and skip building those probes if we do not have full kernel sources (actually if we do not have necessary include files). Or to declare having full kernel source as a requirement for building lttng-modules. What do you think? The user experience should be that if lttng-modules is built against linux-headers packages, the build should work fine, even if it means some extra features are disabled. And we don't want local copies of kernel headers, because then we could trigger runtime bugs if the headers don't match between the kernel version and local version. OK, then I'm inserting the checks to Makefile, that build such probes if the full kernel source is available and to skip them if not. Also, although the README file states that we support only 3.6.38+, we happen to also have support for kernels down to 2.6.32 with the patches in the linux-patches/ directory applied to the appropriate kernel versions. So it would be good that the instrumentation compiles for those older kernels too. I promise we won't go further back than 2.6.32 though. We'll try to add the ifdef's for earlier version down to 2.6.32 to the probes, although the additional amount of conditions makes the code overcomplicated ;-) Indeed, but I never said supporting various kernels was easy ;) I don't care immensely if older kernels have less features: in some cases, it can be just OK to disable an entire tracepoint probe for older kernels. I just want the build to work fine for those. Understood, updated probes (as well as some additional ones) will be sent soon. By the way, we're planning to go for a 2.1 release in a matter of days. At that point, I'll have to decide if we keep the recent instrumentation commits introduced after the start of the -rc cycle in this release (if mature enough), or if we should branch before those for the 2.1 release if they need more testing (and keep these instrumentation extensions for 2.2). Any thoughts on this ? I believe, it would be desirable to make more thorough testing of new instrumentations. On our side the testing was not too extensive, and was done for ARM platform only. Although the probes are mostly replicating the kernel's tracepoints, it would be reasonable to keep the commits in a branch for some time so that more people in the community could make some testing. Also, a side note: the lock probe seems to be not working. It causes kernel hang when trying to perform 'lttng start' at least on arm platform). We suspect that it may be caused by some kind of locks recursion or deadlocks (since tracing engine use locks too and they may be tried to trace again). There is no solution or even
Re: [lttng-dev] [PATCH lttng-modules] Update kernel probes to more detailed match to kernel versions
Hi Mathieu, * Gabbasov, Andrew (andrew_gabba...@mentor.com) wrote: Hi Mathieu, * Andrew Gabbasov (andrew_gabba...@mentor.com) wrote: We have added some more ifdef's to kernel probes to more closely match the kernel tracepoints for different kernel versions. The changes cover kernel versions from 2.6.38 up to 3.7. We did not track the earlier versions, and they are filtered out in probes Makefile. ext3 probe was modified to use #include ../fs/ext3/ext3.h instead of copying this include from kernel source to lttng-modules source. Using such includes (there will be more similar in other probes) imposes a restriction to use the full kernel source for building lttng-modules rather than having just kernel include directory, but it looks like full source is required anyway, at least with respect to Makefiles structure. May be it is worth mentioning somewhere in the documentation. The code was verified to compile with latest versions in all stable branches from 2.6.38.x to 3.6.x and 3.7-rc7. I get this build failure on 3.5 with this patch: CC [M] /home/compudj/work/lttng-modules/probes/lttng-probe-ext3.o /home/compudj/work/lttng-modules/probes/lttng-probe-ext3.c:30:29: fatal error: ../fs/ext3/ext3.h: No such file or directory compilation terminated. can you look into it and submit a revised patch ? You must be using kernel headers for building lttng-modules (linux-headers package or something like that), right? This is what I was writing about: since ext3.h is not available in kernel's include directory, and in order to avoid duplicating this file in lttng-modules sources, we have to use that ../ based reference, and this requires having full kernel source for building. The same issue will later be with btrfs and ext4 probes: their necessary include files are also not available in kernel headers. Trying to duplicate these header files to lttng-modules source seems to be not a good idea, especially if we need to add many ifdef's to them to track different kernel versions. We can try to insert some checks to the Makefile and skip building those probes if we do not have full kernel sources (actually if we do not have necessary include files). Or to declare having full kernel source as a requirement for building lttng-modules. What do you think? The user experience should be that if lttng-modules is built against linux-headers packages, the build should work fine, even if it means some extra features are disabled. And we don't want local copies of kernel headers, because then we could trigger runtime bugs if the headers don't match between the kernel version and local version. OK, then I'm inserting the checks to Makefile, that build such probes if the full kernel source is available and to skip them if not. Also, although the README file states that we support only 3.6.38+, we happen to also have support for kernels down to 2.6.32 with the patches in the linux-patches/ directory applied to the appropriate kernel versions. So it would be good that the instrumentation compiles for those older kernels too. I promise we won't go further back than 2.6.32 though. We'll try to add the ifdef's for earlier version down to 2.6.32 to the probes, although the additional amount of conditions makes the code overcomplicated ;-) Indeed, but I never said supporting various kernels was easy ;) I don't care immensely if older kernels have less features: in some cases, it can be just OK to disable an entire tracepoint probe for older kernels. I just want the build to work fine for those. Understood, updated probes (as well as some additional ones) will be sent soon. By the way, we're planning to go for a 2.1 release in a matter of days. At that point, I'll have to decide if we keep the recent instrumentation commits introduced after the start of the -rc cycle in this release (if mature enough), or if we should branch before those for the 2.1 release if they need more testing (and keep these instrumentation extensions for 2.2). Any thoughts on this ? I believe, it would be desirable to make more thorough testing of new instrumentations. On our side the testing was not too extensive, and was done for ARM platform only. Although the probes are mostly replicating the kernel's tracepoints, it would be reasonable to keep the commits in a branch for some time so that more people in the community could make some testing. Also, a side note: the lock probe seems to be not working. It causes kernel hang when trying to perform 'lttng start' at least on arm platform). We suspect that it may be caused by some kind of locks recursion or deadlocks (since tracing engine use locks too and they may be tried to trace again). There is no solution or even more detailed analysis so far, so I'll disable building of lock probe in the next update. Thanks. Best
Re: [lttng-dev] [PATCH lttng-modules] Update kernel probes to more detailed match to kernel versions
Hi Mathieu, * Andrew Gabbasov (andrew_gabba...@mentor.com) wrote: We have added some more ifdef's to kernel probes to more closely match the kernel tracepoints for different kernel versions. The changes cover kernel versions from 2.6.38 up to 3.7. We did not track the earlier versions, and they are filtered out in probes Makefile. ext3 probe was modified to use #include ../fs/ext3/ext3.h instead of copying this include from kernel source to lttng-modules source. Using such includes (there will be more similar in other probes) imposes a restriction to use the full kernel source for building lttng-modules rather than having just kernel include directory, but it looks like full source is required anyway, at least with respect to Makefiles structure. May be it is worth mentioning somewhere in the documentation. The code was verified to compile with latest versions in all stable branches from 2.6.38.x to 3.6.x and 3.7-rc7. I get this build failure on 3.5 with this patch: CC [M] /home/compudj/work/lttng-modules/probes/lttng-probe-ext3.o /home/compudj/work/lttng-modules/probes/lttng-probe-ext3.c:30:29: fatal error: ../fs/ext3/ext3.h: No such file or directory compilation terminated. can you look into it and submit a revised patch ? You must be using kernel headers for building lttng-modules (linux-headers package or something like that), right? This is what I was writing about: since ext3.h is not available in kernel's include directory, and in order to avoid duplicating this file in lttng-modules sources, we have to use that ../ based reference, and this requires having full kernel source for building. The same issue will later be with btrfs and ext4 probes: their necessary include files are also not available in kernel headers. Trying to duplicate these header files to lttng-modules source seems to be not a good idea, especially if we need to add many ifdef's to them to track different kernel versions. We can try to insert some checks to the Makefile and skip building those probes if we do not have full kernel sources (actually if we do not have necessary include files). Or to declare having full kernel source as a requirement for building lttng-modules. What do you think? Also, although the README file states that we support only 3.6.38+, we happen to also have support for kernels down to 2.6.32 with the patches in the linux-patches/ directory applied to the appropriate kernel versions. So it would be good that the instrumentation compiles for those older kernels too. I promise we won't go further back than 2.6.32 though. We'll try to add the ifdef's for earlier version down to 2.6.32 to the probes, although the additional amount of conditions makes the code overcomplicated ;-) Thanks, Mathieu Thanks. Best regards, Andrew Gabbasov ___ lttng-dev mailing list lttng-dev@lists.lttng.org http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
