Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
Hi - On Thu, Oct 25, 2007 at 03:17:22PM -0400, Mathieu Desnoyers wrote: > [...] > Since gcc is required to build the systemtap probes on the development > marchine, I don't see why it would be much harder to also require prople > to install drawf ? Or maybe the "crash" tool ? The crash tool requires the dwarf data to work. The dwarf data for an entire kernel (including all the modules) is on the order of hundreds of megabytes. The symbol & marker list would be one thousandth the size. You can see the deployment attractiveness of the latter. > I guess you must already need to extract the symbols for your kprobes. > Do you use kallsyms for this? Nope. /proc/kallsyms is a another run-time-only source of data, and so is not applicable for off-line (ahead-of-time) mapping. > I would rather prefer not to implement superfluous built-time data > extraction in the kernel build system just to make userspace > simpler. [...] It is not superfluous, as it would solve a real distribution problem. - FChE - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
Hi - On Thu, Oct 25, 2007 at 03:17:22PM -0400, Mathieu Desnoyers wrote: [...] Since gcc is required to build the systemtap probes on the development marchine, I don't see why it would be much harder to also require prople to install drawf ? Or maybe the crash tool ? The crash tool requires the dwarf data to work. The dwarf data for an entire kernel (including all the modules) is on the order of hundreds of megabytes. The symbol marker list would be one thousandth the size. You can see the deployment attractiveness of the latter. I guess you must already need to extract the symbols for your kprobes. Do you use kallsyms for this? Nope. /proc/kallsyms is a another run-time-only source of data, and so is not applicable for off-line (ahead-of-time) mapping. I would rather prefer not to implement superfluous built-time data extraction in the kernel build system just to make userspace simpler. [...] It is not superfluous, as it would solve a real distribution problem. - FChE - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
* Roland McGrath ([EMAIL PROTECTED]) wrote: > > I think the main issue with the solution you propose is that it doesn't > > deal with markers in modules, am I right ? > > My suggestion applies as well to modules as anything else. > What "like Module.symvers" means is something like: > > name1 vmlinux %s > name2 fs/nfs/nfs %d > > All the modules built by the same kernel build go into this one file. > > Modules packaged separately for the same kernel could provide additional > files of the same kind. > > > I will soon come with a marker iterator and a module that provides a > > userspace -and in kernel- interface to enable/disable markers. Actually, > > I already have the code ready in my LTTng snapshots. I can provide a > > link if you want to have a look. > > That's clearly straightforward to do given the basic markers data structures. > > It does not address the need for an offline list of markers available in a > particular kernel build or set of modules that you are not running right now. > The approach now available for that is grovelling through the markers data > structures extracted from vmlinux and .ko ELF files offline. That is more > work than one should have to do, and has lots of problems with coping with > different packaging details, etc. > Since gcc is required to build the systemtap probes on the development marchine, I don't see why it would be much harder to also require prople to install drawf ? Or maybe the "crash" tool ? I guess you must already need to extract the symbols for your kprobes. Do you use kallsyms for this ? The way I see it, you could maybe extract kallsyms symbols corresponding to the markers data structures quite easily. I would rather prefer not to implement superfluous built-time data extraction in the kernel build system just to make userspace simpler. If we can leverage what currently exists, that would be better. Mathieu > > Thanks, > Roland -- Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
* Roland McGrath ([EMAIL PROTECTED]) wrote: I think the main issue with the solution you propose is that it doesn't deal with markers in modules, am I right ? My suggestion applies as well to modules as anything else. What like Module.symvers means is something like: name1 vmlinux %s name2 fs/nfs/nfs %d All the modules built by the same kernel build go into this one file. Modules packaged separately for the same kernel could provide additional files of the same kind. I will soon come with a marker iterator and a module that provides a userspace -and in kernel- interface to enable/disable markers. Actually, I already have the code ready in my LTTng snapshots. I can provide a link if you want to have a look. That's clearly straightforward to do given the basic markers data structures. It does not address the need for an offline list of markers available in a particular kernel build or set of modules that you are not running right now. The approach now available for that is grovelling through the markers data structures extracted from vmlinux and .ko ELF files offline. That is more work than one should have to do, and has lots of problems with coping with different packaging details, etc. Since gcc is required to build the systemtap probes on the development marchine, I don't see why it would be much harder to also require prople to install drawf ? Or maybe the crash tool ? I guess you must already need to extract the symbols for your kprobes. Do you use kallsyms for this ? The way I see it, you could maybe extract kallsyms symbols corresponding to the markers data structures quite easily. I would rather prefer not to implement superfluous built-time data extraction in the kernel build system just to make userspace simpler. If we can leverage what currently exists, that would be better. Mathieu Thanks, Roland -- Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
> I think the main issue with the solution you propose is that it doesn't > deal with markers in modules, am I right ? My suggestion applies as well to modules as anything else. What "like Module.symvers" means is something like: name1 vmlinux %s name2 fs/nfs/nfs %d All the modules built by the same kernel build go into this one file. Modules packaged separately for the same kernel could provide additional files of the same kind. > I will soon come with a marker iterator and a module that provides a > userspace -and in kernel- interface to enable/disable markers. Actually, > I already have the code ready in my LTTng snapshots. I can provide a > link if you want to have a look. That's clearly straightforward to do given the basic markers data structures. It does not address the need for an offline list of markers available in a particular kernel build or set of modules that you are not running right now. The approach now available for that is grovelling through the markers data structures extracted from vmlinux and .ko ELF files offline. That is more work than one should have to do, and has lots of problems with coping with different packaging details, etc. Thanks, Roland - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
* Frank Ch. Eigler ([EMAIL PROTECTED]) wrote: > Hi - > > I wrote: > > > [...] > > > The marker metadata must be stored in at least one place in the kernel > > > image - this just happens to be a convenient one that David Smith's > > > recent systemtap code used. Without it, we'd probably have to do a > > > more complicated search, following the pointers within the __markers > > > structs. [...] > > Our team is farther along adapting to this change against 2.6.23-mm1, > and we have run into a complication. It's more of a distribution > issue. > > We would prefer to retain systemtap's capability to build > instrumentation for a kernel other than the currently running one. > Such instrumentation can be then copied and run on a distinct machine. > This has meant relying on development data: make install_headers + > Makefiles (as packaged by Fedora/RHEL), and to a lesser extent > separated debugging information. > > Markers are attractive partly because they don't require debugging > information, so the data needs to be found in an executable image. > But we prefer not to force the executable image itself to be > installed, for example because /boot is relatively small. So we would > prefer something in between: something small that we can put into the > development package. > > If there exists sympathy to this problem, Roland McGrath supposes we > could implement a standardized solution, a file like Module.symvers, > containing the marker names & format strings extracted at build time. > Any opinions? > Hi Frank, I think the main issue with the solution you propose is that it doesn't deal with markers in modules, am I right ? I will soon come with a marker iterator and a module that provides a userspace -and in kernel- interface to enable/disable markers. Actually, I already have the code ready in my LTTng snapshots. I can provide a link if you want to have a look. > > PS. I wonder why the marker name/format strings are put into a > __markers_strings object section at all, considering that the only > place where that is used again appears to be this code in > kernel/module.c: > > markersstringsindex = find_sec(hdr, sechdrs, secstrings, > "__markers_strings"); > > and the "markersstringsindex" variable is never used. > Considering that I want to minimize the impact on the system, I put the marker strings in their own memory location rather than clobbering the memory containing the kernel strings (which will likely be used more often than markers). It makes sure that I don't pollute cachelines otherwise containing useful kernel strings. Mathieu > > - FChE -- Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
Hi - I wrote: > [...] > > The marker metadata must be stored in at least one place in the kernel > > image - this just happens to be a convenient one that David Smith's > > recent systemtap code used. Without it, we'd probably have to do a > > more complicated search, following the pointers within the __markers > > structs. [...] Our team is farther along adapting to this change against 2.6.23-mm1, and we have run into a complication. It's more of a distribution issue. We would prefer to retain systemtap's capability to build instrumentation for a kernel other than the currently running one. Such instrumentation can be then copied and run on a distinct machine. This has meant relying on development data: make install_headers + Makefiles (as packaged by Fedora/RHEL), and to a lesser extent separated debugging information. Markers are attractive partly because they don't require debugging information, so the data needs to be found in an executable image. But we prefer not to force the executable image itself to be installed, for example because /boot is relatively small. So we would prefer something in between: something small that we can put into the development package. If there exists sympathy to this problem, Roland McGrath supposes we could implement a standardized solution, a file like Module.symvers, containing the marker names & format strings extracted at build time. Any opinions? PS. I wonder why the marker name/format strings are put into a __markers_strings object section at all, considering that the only place where that is used again appears to be this code in kernel/module.c: markersstringsindex = find_sec(hdr, sechdrs, secstrings, "__markers_strings"); and the "markersstringsindex" variable is never used. - FChE - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
Hi - I wrote: [...] The marker metadata must be stored in at least one place in the kernel image - this just happens to be a convenient one that David Smith's recent systemtap code used. Without it, we'd probably have to do a more complicated search, following the pointers within the __markers structs. [...] Our team is farther along adapting to this change against 2.6.23-mm1, and we have run into a complication. It's more of a distribution issue. We would prefer to retain systemtap's capability to build instrumentation for a kernel other than the currently running one. Such instrumentation can be then copied and run on a distinct machine. This has meant relying on development data: make install_headers + Makefiles (as packaged by Fedora/RHEL), and to a lesser extent separated debugging information. Markers are attractive partly because they don't require debugging information, so the data needs to be found in an executable image. But we prefer not to force the executable image itself to be installed, for example because /boot is relatively small. So we would prefer something in between: something small that we can put into the development package. If there exists sympathy to this problem, Roland McGrath supposes we could implement a standardized solution, a file like Module.symvers, containing the marker names format strings extracted at build time. Any opinions? PS. I wonder why the marker name/format strings are put into a __markers_strings object section at all, considering that the only place where that is used again appears to be this code in kernel/module.c: markersstringsindex = find_sec(hdr, sechdrs, secstrings, __markers_strings); and the markersstringsindex variable is never used. - FChE - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
* Frank Ch. Eigler ([EMAIL PROTECTED]) wrote: Hi - I wrote: [...] The marker metadata must be stored in at least one place in the kernel image - this just happens to be a convenient one that David Smith's recent systemtap code used. Without it, we'd probably have to do a more complicated search, following the pointers within the __markers structs. [...] Our team is farther along adapting to this change against 2.6.23-mm1, and we have run into a complication. It's more of a distribution issue. We would prefer to retain systemtap's capability to build instrumentation for a kernel other than the currently running one. Such instrumentation can be then copied and run on a distinct machine. This has meant relying on development data: make install_headers + Makefiles (as packaged by Fedora/RHEL), and to a lesser extent separated debugging information. Markers are attractive partly because they don't require debugging information, so the data needs to be found in an executable image. But we prefer not to force the executable image itself to be installed, for example because /boot is relatively small. So we would prefer something in between: something small that we can put into the development package. If there exists sympathy to this problem, Roland McGrath supposes we could implement a standardized solution, a file like Module.symvers, containing the marker names format strings extracted at build time. Any opinions? Hi Frank, I think the main issue with the solution you propose is that it doesn't deal with markers in modules, am I right ? I will soon come with a marker iterator and a module that provides a userspace -and in kernel- interface to enable/disable markers. Actually, I already have the code ready in my LTTng snapshots. I can provide a link if you want to have a look. PS. I wonder why the marker name/format strings are put into a __markers_strings object section at all, considering that the only place where that is used again appears to be this code in kernel/module.c: markersstringsindex = find_sec(hdr, sechdrs, secstrings, __markers_strings); and the markersstringsindex variable is never used. Considering that I want to minimize the impact on the system, I put the marker strings in their own memory location rather than clobbering the memory containing the kernel strings (which will likely be used more often than markers). It makes sure that I don't pollute cachelines otherwise containing useful kernel strings. Mathieu - FChE -- Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
I think the main issue with the solution you propose is that it doesn't deal with markers in modules, am I right ? My suggestion applies as well to modules as anything else. What like Module.symvers means is something like: name1 vmlinux %s name2 fs/nfs/nfs %d All the modules built by the same kernel build go into this one file. Modules packaged separately for the same kernel could provide additional files of the same kind. I will soon come with a marker iterator and a module that provides a userspace -and in kernel- interface to enable/disable markers. Actually, I already have the code ready in my LTTng snapshots. I can provide a link if you want to have a look. That's clearly straightforward to do given the basic markers data structures. It does not address the need for an offline list of markers available in a particular kernel build or set of modules that you are not running right now. The approach now available for that is grovelling through the markers data structures extracted from vmlinux and .ko ELF files offline. That is more work than one should have to do, and has lots of problems with coping with different packaging details, etc. Thanks, Roland - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
* Steven Rostedt ([EMAIL PROTECTED]) wrote: > On Tue, Sep 18, 2007 at 05:13:25PM -0400, Mathieu Desnoyers wrote: > > +/* > > + * Sets the probe callback corresponding to one marker. > > + */ > > +static int set_marker(struct marker_entry **entry, > > + struct __mark_marker *elem) > > +{ > > + int ret; > > + BUG_ON(strcmp((*entry)->name, elem->name) != 0); > > Can you switch this at least to WARN_ON? Killing a system with X > running where the user just sees a freeze is not that nice. But a nasty > message in dmesg is very noticable. > Sure. > -- Steve > > > + > > + if ((*entry)->format) { > > + if (strcmp((*entry)->format, elem->format) != 0) { > > + printk(KERN_NOTICE > > + "Format mismatch for probe %s " > > + "(%s), marker (%s)\n", > > + (*entry)->name, > > + (*entry)->format, > > + elem->format); > > + return -EPERM; > > + } > > + } else { > > + ret = marker_set_format(entry, elem->format); > > + if (ret) > > + return ret; > > + } > > + elem->call = (*entry)->probe; > > + elem->pdata = (*entry)->pdata; > > + _immediate_set(elem->state, 1); > > + return 0; > > +} -- Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
* Frank Ch. Eigler ([EMAIL PROTECTED]) wrote: > Hi - > > On Fri, Sep 21, 2007 at 08:58:19AM -0400, Mathieu Desnoyers wrote: > > [...] > > > > Current systemtap marker support code relies on the __markers_strings > > > > section. > > > Let users know that in comment above section definition in ld script. > > [...] > > /* Markers: strings (used by SystemTAP) */ \ > > [...] > > I did not mean to imply that this was a necessary state of affairs. > > The marker metadata must be stored in at least one place in the kernel > image - this just happens to be a convenient one that David Smith's > recent systemtap code used. Without it, we'd probably have to do a > more complicated search, following the pointers within the __markers > structs. That could work, but it hasn't been built/tested. > > So, this proposed change (removal of this section) would break > systemtap, and we have to jump through more hoops to make it work > again. Is the change worth it? > I guess so. Getting the markers as clean as we can is very important for kernel inclusion. Mathieu -- Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
Hi - On Fri, Sep 21, 2007 at 08:58:19AM -0400, Mathieu Desnoyers wrote: > [...] > > > Current systemtap marker support code relies on the __markers_strings > > > section. > > Let users know that in comment above section definition in ld script. > [...] > /* Markers: strings (used by SystemTAP) */ \ > [...] I did not mean to imply that this was a necessary state of affairs. The marker metadata must be stored in at least one place in the kernel image - this just happens to be a convenient one that David Smith's recent systemtap code used. Without it, we'd probably have to do a more complicated search, following the pointers within the __markers structs. That could work, but it hasn't been built/tested. So, this proposed change (removal of this section) would break systemtap, and we have to jump through more hoops to make it work again. Is the change worth it? - FChE - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
On Fri, Sep 21, 2007 at 08:58:19AM -0400, Mathieu Desnoyers wrote: > Ok, I am changing it to: As I mentioned before pleae just kill this gunk entirely as it's not needed at all intree. markers are already getting far too complex, I'd rather want a simple useable version in now than trying to cater for every possible use-case. Once the systemtap people are ready for merging their stuff into the kernel tree we can cater towards their needs. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
* Denys Vlasenko ([EMAIL PROTECTED]) wrote: > On Wednesday 19 September 2007 14:53, Frank Ch. Eigler wrote: > > Mathieu Desnoyers <[EMAIL PROTECTED]> writes: > > > > > [...] Do you think I should also remove the __markers_strings > > > section from here ? > > > > Current systemtap marker support code relies on the __markers_strings > > section. > > Let users know that in comment above section definition in ld script. > Ok, I am changing it to: /* Markers: strings (used by SystemTAP) */ \ __markers_strings : AT(ADDR(__markers_strings) - LOAD_OFFSET) { \ *(__markers_strings)\ } \ Mathieu > Thanks! > -- > vda -- Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
* Denys Vlasenko ([EMAIL PROTECTED]) wrote: On Wednesday 19 September 2007 14:53, Frank Ch. Eigler wrote: Mathieu Desnoyers [EMAIL PROTECTED] writes: [...] Do you think I should also remove the __markers_strings section from here ? Current systemtap marker support code relies on the __markers_strings section. Let users know that in comment above section definition in ld script. Ok, I am changing it to: /* Markers: strings (used by SystemTAP) */ \ __markers_strings : AT(ADDR(__markers_strings) - LOAD_OFFSET) { \ *(__markers_strings)\ } \ Mathieu Thanks! -- vda -- Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
On Fri, Sep 21, 2007 at 08:58:19AM -0400, Mathieu Desnoyers wrote: Ok, I am changing it to: As I mentioned before pleae just kill this gunk entirely as it's not needed at all intree. markers are already getting far too complex, I'd rather want a simple useable version in now than trying to cater for every possible use-case. Once the systemtap people are ready for merging their stuff into the kernel tree we can cater towards their needs. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
Hi - On Fri, Sep 21, 2007 at 08:58:19AM -0400, Mathieu Desnoyers wrote: [...] Current systemtap marker support code relies on the __markers_strings section. Let users know that in comment above section definition in ld script. [...] /* Markers: strings (used by SystemTAP) */ \ [...] I did not mean to imply that this was a necessary state of affairs. The marker metadata must be stored in at least one place in the kernel image - this just happens to be a convenient one that David Smith's recent systemtap code used. Without it, we'd probably have to do a more complicated search, following the pointers within the __markers structs. That could work, but it hasn't been built/tested. So, this proposed change (removal of this section) would break systemtap, and we have to jump through more hoops to make it work again. Is the change worth it? - FChE - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
* Frank Ch. Eigler ([EMAIL PROTECTED]) wrote: Hi - On Fri, Sep 21, 2007 at 08:58:19AM -0400, Mathieu Desnoyers wrote: [...] Current systemtap marker support code relies on the __markers_strings section. Let users know that in comment above section definition in ld script. [...] /* Markers: strings (used by SystemTAP) */ \ [...] I did not mean to imply that this was a necessary state of affairs. The marker metadata must be stored in at least one place in the kernel image - this just happens to be a convenient one that David Smith's recent systemtap code used. Without it, we'd probably have to do a more complicated search, following the pointers within the __markers structs. That could work, but it hasn't been built/tested. So, this proposed change (removal of this section) would break systemtap, and we have to jump through more hoops to make it work again. Is the change worth it? I guess so. Getting the markers as clean as we can is very important for kernel inclusion. Mathieu -- Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
* Steven Rostedt ([EMAIL PROTECTED]) wrote: On Tue, Sep 18, 2007 at 05:13:25PM -0400, Mathieu Desnoyers wrote: +/* + * Sets the probe callback corresponding to one marker. + */ +static int set_marker(struct marker_entry **entry, + struct __mark_marker *elem) +{ + int ret; + BUG_ON(strcmp((*entry)-name, elem-name) != 0); Can you switch this at least to WARN_ON? Killing a system with X running where the user just sees a freeze is not that nice. But a nasty message in dmesg is very noticable. Sure. -- Steve + + if ((*entry)-format) { + if (strcmp((*entry)-format, elem-format) != 0) { + printk(KERN_NOTICE + Format mismatch for probe %s + (%s), marker (%s)\n, + (*entry)-name, + (*entry)-format, + elem-format); + return -EPERM; + } + } else { + ret = marker_set_format(entry, elem-format); + if (ret) + return ret; + } + elem-call = (*entry)-probe; + elem-pdata = (*entry)-pdata; + _immediate_set(elem-state, 1); + return 0; +} -- Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
On Tue, Sep 18, 2007 at 05:13:25PM -0400, Mathieu Desnoyers wrote: > +/* > + * Sets the probe callback corresponding to one marker. > + */ > +static int set_marker(struct marker_entry **entry, > + struct __mark_marker *elem) > +{ > + int ret; > + BUG_ON(strcmp((*entry)->name, elem->name) != 0); Can you switch this at least to WARN_ON? Killing a system with X running where the user just sees a freeze is not that nice. But a nasty message in dmesg is very noticable. -- Steve > + > + if ((*entry)->format) { > + if (strcmp((*entry)->format, elem->format) != 0) { > + printk(KERN_NOTICE > + "Format mismatch for probe %s " > + "(%s), marker (%s)\n", > + (*entry)->name, > + (*entry)->format, > + elem->format); > + return -EPERM; > + } > + } else { > + ret = marker_set_format(entry, elem->format); > + if (ret) > + return ret; > + } > + elem->call = (*entry)->probe; > + elem->pdata = (*entry)->pdata; > + _immediate_set(elem->state, 1); > + return 0; > +} - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
On Tue, Sep 18, 2007 at 05:13:25PM -0400, Mathieu Desnoyers wrote: +/* + * Sets the probe callback corresponding to one marker. + */ +static int set_marker(struct marker_entry **entry, + struct __mark_marker *elem) +{ + int ret; + BUG_ON(strcmp((*entry)-name, elem-name) != 0); Can you switch this at least to WARN_ON? Killing a system with X running where the user just sees a freeze is not that nice. But a nasty message in dmesg is very noticable. -- Steve + + if ((*entry)-format) { + if (strcmp((*entry)-format, elem-format) != 0) { + printk(KERN_NOTICE + Format mismatch for probe %s + (%s), marker (%s)\n, + (*entry)-name, + (*entry)-format, + elem-format); + return -EPERM; + } + } else { + ret = marker_set_format(entry, elem-format); + if (ret) + return ret; + } + elem-call = (*entry)-probe; + elem-pdata = (*entry)-pdata; + _immediate_set(elem-state, 1); + return 0; +} - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
On Wednesday 19 September 2007 14:53, Frank Ch. Eigler wrote: > Mathieu Desnoyers <[EMAIL PROTECTED]> writes: > > > [...] Do you think I should also remove the __markers_strings > > section from here ? > > Current systemtap marker support code relies on the __markers_strings > section. Let users know that in comment above section definition in ld script. Thanks! -- vda - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
Mathieu Desnoyers <[EMAIL PROTECTED]> writes: > [...] Do you think I should also remove the __markers_strings > section from here ? Current systemtap marker support code relies on the __markers_strings section. - FChE - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
* Mathieu Desnoyers ([EMAIL PROTECTED]) wrote: > * Denys Vlasenko ([EMAIL PROTECTED]) wrote: > > On Wednesday 19 September 2007 12:37, Mathieu Desnoyers wrote: > > > > Index: linux-2.6-lttng/include/asm-generic/vmlinux.lds.h > > > > === > > > > --- linux-2.6-lttng.orig/include/asm-generic/vmlinux.lds.h > > > > 2007-09-14 10:11:18.0 -0400 > > > > +++ linux-2.6-lttng/include/asm-generic/vmlinux.lds.h 2007-09-14 > > > > 10:11:31.0 -0400 > > > > @@ -129,6 +133,11 @@ > > > > VMLINUX_SYMBOL(__stop___immediate) = .; > > > > \ > > > > } > > > > \ > > > > > > > > \ > > > > + /* Markers: strings */ > > > > \ > > > > +__markers_strings : AT(ADDR(__markers_strings) - LOAD_OFFSET) > > > > {\ > > > > + *(__markers_strings) > > > > \ > > > > + } > > > > \ > > > > + > > > > \ > > > > /* Kernel symbol table: strings */ > > > > \ > > > > __ksymtab_strings : AT(ADDR(__ksymtab_strings) - LOAD_OFFSET) > > > > {\ > > > > *(__ksymtab_strings) > > > > \ > > > [...] > > > > > > Do you think I should also remove the __markers_strings section from here > > > ? > > > > Yes. > > > > It will be beneficial if one can read include/asm-generic/vmlinux.lds.h and > > arch/$ARCH/kernel/vmlinux.lds.S and understand which sections in resulting > > vmlinux serve what purpose. A comment atop each section explaining > > its role will be nice. Even more so that not many people are fluent > > in ld script language. > > > > Currently, one will need to grep around (and not only in kernel tree - > > you need to read depmod.c source too) in order to understand the role > > of various sections in vmlinux. > > > > There are dearth of comments in ld scripts, and some sections > > are created "just because I felt like it". For example, there are > > ".data.page_aligned" and ".data.percpu" sections - can you > > easily tell which one has to be a section, and which does not need > > to be one (can be merged with ".data")? Maybe both must be sections? > > Or none of them? > > -- > > vda > > Oh, wait.. I need it in module.c: > > immediateindex = find_sec(hdr, sechdrs, secstrings, "__immediate"); > > I'll leave the section there then. > > Mathieu Sorry, let me take this back. It applies to what is linked in the core image, but I believe it does not apply to the kernel modules. Mathieu -- Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
* Denys Vlasenko ([EMAIL PROTECTED]) wrote: > On Wednesday 19 September 2007 12:37, Mathieu Desnoyers wrote: > > > Index: linux-2.6-lttng/include/asm-generic/vmlinux.lds.h > > > === > > > --- linux-2.6-lttng.orig/include/asm-generic/vmlinux.lds.h > > > 2007-09-14 10:11:18.0 -0400 > > > +++ linux-2.6-lttng/include/asm-generic/vmlinux.lds.h 2007-09-14 > > > 10:11:31.0 -0400 > > > @@ -129,6 +133,11 @@ > > > VMLINUX_SYMBOL(__stop___immediate) = .; \ > > > } \ > > > \ > > > + /* Markers: strings */ \ > > > +__markers_strings : AT(ADDR(__markers_strings) - LOAD_OFFSET) { > > > \ > > > + *(__markers_strings)\ > > > + } > > > \ > > > + \ > > > /* Kernel symbol table: strings */ \ > > > __ksymtab_strings : AT(ADDR(__ksymtab_strings) - LOAD_OFFSET) { > > > \ > > > *(__ksymtab_strings)\ > > [...] > > > > Do you think I should also remove the __markers_strings section from here ? > > Yes. > > It will be beneficial if one can read include/asm-generic/vmlinux.lds.h and > arch/$ARCH/kernel/vmlinux.lds.S and understand which sections in resulting > vmlinux serve what purpose. A comment atop each section explaining > its role will be nice. Even more so that not many people are fluent > in ld script language. > > Currently, one will need to grep around (and not only in kernel tree - > you need to read depmod.c source too) in order to understand the role > of various sections in vmlinux. > > There are dearth of comments in ld scripts, and some sections > are created "just because I felt like it". For example, there are > ".data.page_aligned" and ".data.percpu" sections - can you > easily tell which one has to be a section, and which does not need > to be one (can be merged with ".data")? Maybe both must be sections? > Or none of them? > -- > vda Oh, wait.. I need it in module.c: immediateindex = find_sec(hdr, sechdrs, secstrings, "__immediate"); I'll leave the section there then. Mathieu -- Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
On Wednesday 19 September 2007 12:37, Mathieu Desnoyers wrote: > > Index: linux-2.6-lttng/include/asm-generic/vmlinux.lds.h > > === > > --- linux-2.6-lttng.orig/include/asm-generic/vmlinux.lds.h 2007-09-14 > > 10:11:18.0 -0400 > > +++ linux-2.6-lttng/include/asm-generic/vmlinux.lds.h 2007-09-14 > > 10:11:31.0 -0400 > > @@ -129,6 +133,11 @@ > > VMLINUX_SYMBOL(__stop___immediate) = .; \ > > } \ > > \ > > + /* Markers: strings */ \ > > +__markers_strings : AT(ADDR(__markers_strings) - LOAD_OFFSET) { > > \ > > + *(__markers_strings)\ > > + } \ > > + \ > > /* Kernel symbol table: strings */ \ > > __ksymtab_strings : AT(ADDR(__ksymtab_strings) - LOAD_OFFSET) { > > \ > > *(__ksymtab_strings)\ > [...] > > Do you think I should also remove the __markers_strings section from here ? Yes. It will be beneficial if one can read include/asm-generic/vmlinux.lds.h and arch/$ARCH/kernel/vmlinux.lds.S and understand which sections in resulting vmlinux serve what purpose. A comment atop each section explaining its role will be nice. Even more so that not many people are fluent in ld script language. Currently, one will need to grep around (and not only in kernel tree - you need to read depmod.c source too) in order to understand the role of various sections in vmlinux. There are dearth of comments in ld scripts, and some sections are created "just because I felt like it". For example, there are ".data.page_aligned" and ".data.percpu" sections - can you easily tell which one has to be a section, and which does not need to be one (can be merged with ".data")? Maybe both must be sections? Or none of them? -- vda - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
* Mathieu Desnoyers ([EMAIL PROTECTED]) wrote: > The marker activation functions sits in kernel/marker.c. A hash table is used > to keep track of the registered probes and armed markers, so the markers > within > a newly loaded module that should be active can be activated at module load > time. > > marker_query has been removed. marker_get_first, marker_get_next and > marker_release should be used as iterators on the markers. > > Changelog: > - markers_mutex now nests inside module_mutex rather than the opposite. > - Iteration on modules is now done in module.c. > - module_mutex is not exported anymore. > [...] > Index: linux-2.6-lttng/include/asm-generic/vmlinux.lds.h > === > --- linux-2.6-lttng.orig/include/asm-generic/vmlinux.lds.h2007-09-14 > 10:11:18.0 -0400 > +++ linux-2.6-lttng/include/asm-generic/vmlinux.lds.h 2007-09-14 > 10:11:31.0 -0400 > @@ -12,7 +12,11 @@ > /* .data section */ > #define DATA_DATA\ > *(.data)\ > - *(.data.init.refok) > + *(.data.init.refok) \ > + . = ALIGN(8); \ > + VMLINUX_SYMBOL(__start___markers) = .; \ > + *(__markers)\ > + VMLINUX_SYMBOL(__stop___markers) = .; > > #define RO_DATA(align) > \ > . = ALIGN((align)); \ > @@ -129,6 +133,11 @@ > VMLINUX_SYMBOL(__stop___immediate) = .; \ > } \ > \ > + /* Markers: strings */ \ > +__markers_strings : AT(ADDR(__markers_strings) - LOAD_OFFSET) { > \ > + *(__markers_strings)\ > + } \ > + \ > /* Kernel symbol table: strings */ \ > __ksymtab_strings : AT(ADDR(__ksymtab_strings) - LOAD_OFFSET) { > \ > *(__ksymtab_strings)\ [...] Do you think I should also remove the __markers_strings section from here ? -- Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
* Mathieu Desnoyers ([EMAIL PROTECTED]) wrote: The marker activation functions sits in kernel/marker.c. A hash table is used to keep track of the registered probes and armed markers, so the markers within a newly loaded module that should be active can be activated at module load time. marker_query has been removed. marker_get_first, marker_get_next and marker_release should be used as iterators on the markers. Changelog: - markers_mutex now nests inside module_mutex rather than the opposite. - Iteration on modules is now done in module.c. - module_mutex is not exported anymore. [...] Index: linux-2.6-lttng/include/asm-generic/vmlinux.lds.h === --- linux-2.6-lttng.orig/include/asm-generic/vmlinux.lds.h2007-09-14 10:11:18.0 -0400 +++ linux-2.6-lttng/include/asm-generic/vmlinux.lds.h 2007-09-14 10:11:31.0 -0400 @@ -12,7 +12,11 @@ /* .data section */ #define DATA_DATA\ *(.data)\ - *(.data.init.refok) + *(.data.init.refok) \ + . = ALIGN(8); \ + VMLINUX_SYMBOL(__start___markers) = .; \ + *(__markers)\ + VMLINUX_SYMBOL(__stop___markers) = .; #define RO_DATA(align) \ . = ALIGN((align)); \ @@ -129,6 +133,11 @@ VMLINUX_SYMBOL(__stop___immediate) = .; \ } \ \ + /* Markers: strings */ \ +__markers_strings : AT(ADDR(__markers_strings) - LOAD_OFFSET) { \ + *(__markers_strings)\ + } \ + \ /* Kernel symbol table: strings */ \ __ksymtab_strings : AT(ADDR(__ksymtab_strings) - LOAD_OFFSET) { \ *(__ksymtab_strings)\ [...] Do you think I should also remove the __markers_strings section from here ? -- Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
On Wednesday 19 September 2007 12:37, Mathieu Desnoyers wrote: Index: linux-2.6-lttng/include/asm-generic/vmlinux.lds.h === --- linux-2.6-lttng.orig/include/asm-generic/vmlinux.lds.h 2007-09-14 10:11:18.0 -0400 +++ linux-2.6-lttng/include/asm-generic/vmlinux.lds.h 2007-09-14 10:11:31.0 -0400 @@ -129,6 +133,11 @@ VMLINUX_SYMBOL(__stop___immediate) = .; \ } \ \ + /* Markers: strings */ \ +__markers_strings : AT(ADDR(__markers_strings) - LOAD_OFFSET) { \ + *(__markers_strings)\ + } \ + \ /* Kernel symbol table: strings */ \ __ksymtab_strings : AT(ADDR(__ksymtab_strings) - LOAD_OFFSET) { \ *(__ksymtab_strings)\ [...] Do you think I should also remove the __markers_strings section from here ? Yes. It will be beneficial if one can read include/asm-generic/vmlinux.lds.h and arch/$ARCH/kernel/vmlinux.lds.S and understand which sections in resulting vmlinux serve what purpose. A comment atop each section explaining its role will be nice. Even more so that not many people are fluent in ld script language. Currently, one will need to grep around (and not only in kernel tree - you need to read depmod.c source too) in order to understand the role of various sections in vmlinux. There are dearth of comments in ld scripts, and some sections are created just because I felt like it. For example, there are .data.page_aligned and .data.percpu sections - can you easily tell which one has to be a section, and which does not need to be one (can be merged with .data)? Maybe both must be sections? Or none of them? -- vda - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
* Denys Vlasenko ([EMAIL PROTECTED]) wrote: On Wednesday 19 September 2007 12:37, Mathieu Desnoyers wrote: Index: linux-2.6-lttng/include/asm-generic/vmlinux.lds.h === --- linux-2.6-lttng.orig/include/asm-generic/vmlinux.lds.h 2007-09-14 10:11:18.0 -0400 +++ linux-2.6-lttng/include/asm-generic/vmlinux.lds.h 2007-09-14 10:11:31.0 -0400 @@ -129,6 +133,11 @@ VMLINUX_SYMBOL(__stop___immediate) = .; \ } \ \ + /* Markers: strings */ \ +__markers_strings : AT(ADDR(__markers_strings) - LOAD_OFFSET) { \ + *(__markers_strings)\ + } \ + \ /* Kernel symbol table: strings */ \ __ksymtab_strings : AT(ADDR(__ksymtab_strings) - LOAD_OFFSET) { \ *(__ksymtab_strings)\ [...] Do you think I should also remove the __markers_strings section from here ? Yes. It will be beneficial if one can read include/asm-generic/vmlinux.lds.h and arch/$ARCH/kernel/vmlinux.lds.S and understand which sections in resulting vmlinux serve what purpose. A comment atop each section explaining its role will be nice. Even more so that not many people are fluent in ld script language. Currently, one will need to grep around (and not only in kernel tree - you need to read depmod.c source too) in order to understand the role of various sections in vmlinux. There are dearth of comments in ld scripts, and some sections are created just because I felt like it. For example, there are .data.page_aligned and .data.percpu sections - can you easily tell which one has to be a section, and which does not need to be one (can be merged with .data)? Maybe both must be sections? Or none of them? -- vda Oh, wait.. I need it in module.c: immediateindex = find_sec(hdr, sechdrs, secstrings, __immediate); I'll leave the section there then. Mathieu -- Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
* Mathieu Desnoyers ([EMAIL PROTECTED]) wrote: * Denys Vlasenko ([EMAIL PROTECTED]) wrote: On Wednesday 19 September 2007 12:37, Mathieu Desnoyers wrote: Index: linux-2.6-lttng/include/asm-generic/vmlinux.lds.h === --- linux-2.6-lttng.orig/include/asm-generic/vmlinux.lds.h 2007-09-14 10:11:18.0 -0400 +++ linux-2.6-lttng/include/asm-generic/vmlinux.lds.h 2007-09-14 10:11:31.0 -0400 @@ -129,6 +133,11 @@ VMLINUX_SYMBOL(__stop___immediate) = .; \ } \ \ + /* Markers: strings */ \ +__markers_strings : AT(ADDR(__markers_strings) - LOAD_OFFSET) {\ + *(__markers_strings) \ + } \ + \ /* Kernel symbol table: strings */ \ __ksymtab_strings : AT(ADDR(__ksymtab_strings) - LOAD_OFFSET) {\ *(__ksymtab_strings) \ [...] Do you think I should also remove the __markers_strings section from here ? Yes. It will be beneficial if one can read include/asm-generic/vmlinux.lds.h and arch/$ARCH/kernel/vmlinux.lds.S and understand which sections in resulting vmlinux serve what purpose. A comment atop each section explaining its role will be nice. Even more so that not many people are fluent in ld script language. Currently, one will need to grep around (and not only in kernel tree - you need to read depmod.c source too) in order to understand the role of various sections in vmlinux. There are dearth of comments in ld scripts, and some sections are created just because I felt like it. For example, there are .data.page_aligned and .data.percpu sections - can you easily tell which one has to be a section, and which does not need to be one (can be merged with .data)? Maybe both must be sections? Or none of them? -- vda Oh, wait.. I need it in module.c: immediateindex = find_sec(hdr, sechdrs, secstrings, __immediate); I'll leave the section there then. Mathieu Sorry, let me take this back. It applies to what is linked in the core image, but I believe it does not apply to the kernel modules. Mathieu -- Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
Mathieu Desnoyers [EMAIL PROTECTED] writes: [...] Do you think I should also remove the __markers_strings section from here ? Current systemtap marker support code relies on the __markers_strings section. - FChE - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
On Wednesday 19 September 2007 14:53, Frank Ch. Eigler wrote: Mathieu Desnoyers [EMAIL PROTECTED] writes: [...] Do you think I should also remove the __markers_strings section from here ? Current systemtap marker support code relies on the __markers_strings section. Let users know that in comment above section definition in ld script. Thanks! -- vda - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
* Rusty Russell ([EMAIL PROTECTED]) wrote: > On Fri, 2007-08-24 at 12:26 -0400, Mathieu Desnoyers wrote: > > * Rusty Russell ([EMAIL PROTECTED]) wrote: > > > On Mon, 2007-08-20 at 16:27 -0400, Mathieu Desnoyers wrote: > > > > +{ > > > > + struct hlist_head *head; > > > > + struct hlist_node *node; > > > > + struct marker_entry *e; > > > > + size_t len = strlen(name) + 1; > > > > + u32 hash = jhash(name, len-1, 0); > > > > + > > > > + head = _table[hash & ((1 << MARKER_HASH_BITS)-1)]; > > > > + hlist_for_each_entry(e, node, head, hlist) { > > > > + if (!strcmp(name, e->name)) > > > > + return e; > > > > + } > > > > + return NULL; > > > > +} > > > > > > OK, don't understand the strlen, len, len-1 dance here? > > > > > > > Let's say we have abc\0 for marker name as name input. > > > > len = 3 + 1 = 4 (including \0) > > hash is done only on the 3 first chars, excluding the \0 (therefore the > > len-1 there) > > > > Actually, it's like this only for a matter of consistency between > > add_marker and remove_marker, which are quite similar, but add_marker > > needs name_len to include the \0 value. It would be odd to change the > > logic between the two functions to one including the \0 and the other > > excluding it. > > Sure, but that doesn't really explain why the code does: > > size_t len = strlen(name) + 1; > u32 hash = jhash(name, len-1, 0); > > Rather than: > > u32 hash = jhash(name, strlen(name), 0); > Yup, good point. Fixed. Thanks, Mathieu > > Thanks for the review, > > That's fine, just some light reading... > > Cheers, > Rusty. > > > -- Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
On Fri, 2007-08-24 at 12:26 -0400, Mathieu Desnoyers wrote: > * Rusty Russell ([EMAIL PROTECTED]) wrote: > > On Mon, 2007-08-20 at 16:27 -0400, Mathieu Desnoyers wrote: > > > +{ > > > + struct hlist_head *head; > > > + struct hlist_node *node; > > > + struct marker_entry *e; > > > + size_t len = strlen(name) + 1; > > > + u32 hash = jhash(name, len-1, 0); > > > + > > > + head = _table[hash & ((1 << MARKER_HASH_BITS)-1)]; > > > + hlist_for_each_entry(e, node, head, hlist) { > > > + if (!strcmp(name, e->name)) > > > + return e; > > > + } > > > + return NULL; > > > +} > > > > OK, don't understand the strlen, len, len-1 dance here? > > > > Let's say we have abc\0 for marker name as name input. > > len = 3 + 1 = 4 (including \0) > hash is done only on the 3 first chars, excluding the \0 (therefore the > len-1 there) > > Actually, it's like this only for a matter of consistency between > add_marker and remove_marker, which are quite similar, but add_marker > needs name_len to include the \0 value. It would be odd to change the > logic between the two functions to one including the \0 and the other > excluding it. Sure, but that doesn't really explain why the code does: size_t len = strlen(name) + 1; u32 hash = jhash(name, len-1, 0); Rather than: u32 hash = jhash(name, strlen(name), 0); > Thanks for the review, That's fine, just some light reading... Cheers, Rusty. > - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
On Fri, 2007-08-24 at 12:26 -0400, Mathieu Desnoyers wrote: * Rusty Russell ([EMAIL PROTECTED]) wrote: On Mon, 2007-08-20 at 16:27 -0400, Mathieu Desnoyers wrote: +{ + struct hlist_head *head; + struct hlist_node *node; + struct marker_entry *e; + size_t len = strlen(name) + 1; + u32 hash = jhash(name, len-1, 0); + + head = marker_table[hash ((1 MARKER_HASH_BITS)-1)]; + hlist_for_each_entry(e, node, head, hlist) { + if (!strcmp(name, e-name)) + return e; + } + return NULL; +} OK, don't understand the strlen, len, len-1 dance here? Let's say we have abc\0 for marker name as name input. len = 3 + 1 = 4 (including \0) hash is done only on the 3 first chars, excluding the \0 (therefore the len-1 there) Actually, it's like this only for a matter of consistency between add_marker and remove_marker, which are quite similar, but add_marker needs name_len to include the \0 value. It would be odd to change the logic between the two functions to one including the \0 and the other excluding it. Sure, but that doesn't really explain why the code does: size_t len = strlen(name) + 1; u32 hash = jhash(name, len-1, 0); Rather than: u32 hash = jhash(name, strlen(name), 0); Thanks for the review, That's fine, just some light reading... Cheers, Rusty. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
* Rusty Russell ([EMAIL PROTECTED]) wrote: On Fri, 2007-08-24 at 12:26 -0400, Mathieu Desnoyers wrote: * Rusty Russell ([EMAIL PROTECTED]) wrote: On Mon, 2007-08-20 at 16:27 -0400, Mathieu Desnoyers wrote: +{ + struct hlist_head *head; + struct hlist_node *node; + struct marker_entry *e; + size_t len = strlen(name) + 1; + u32 hash = jhash(name, len-1, 0); + + head = marker_table[hash ((1 MARKER_HASH_BITS)-1)]; + hlist_for_each_entry(e, node, head, hlist) { + if (!strcmp(name, e-name)) + return e; + } + return NULL; +} OK, don't understand the strlen, len, len-1 dance here? Let's say we have abc\0 for marker name as name input. len = 3 + 1 = 4 (including \0) hash is done only on the 3 first chars, excluding the \0 (therefore the len-1 there) Actually, it's like this only for a matter of consistency between add_marker and remove_marker, which are quite similar, but add_marker needs name_len to include the \0 value. It would be odd to change the logic between the two functions to one including the \0 and the other excluding it. Sure, but that doesn't really explain why the code does: size_t len = strlen(name) + 1; u32 hash = jhash(name, len-1, 0); Rather than: u32 hash = jhash(name, strlen(name), 0); Yup, good point. Fixed. Thanks, Mathieu Thanks for the review, That's fine, just some light reading... Cheers, Rusty. -- Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
* Rusty Russell ([EMAIL PROTECTED]) wrote: > On Mon, 2007-08-20 at 16:27 -0400, Mathieu Desnoyers wrote: > > The marker activation functions sits in kernel/marker.c. A hash table is > > used > > to keep track of the registered probes and armed markers, so the markers > > within > > a newly loaded module that should be active can be activated at module load > > time. > > Hi Mathieu! > > Just reading through this patch, a couple of comments: > Hi Rusty, > > +/* To be used for string format validity checking with gcc */ > > +static inline void __mark_check_format(const char *fmt, ...) > > + __attribute__ ((format (printf, 1, 2))); > > +static inline void __mark_check_format(const char *fmt, ...) { } > > If you place the __attribute__() before the function name, you can do > this in the definition. > Ok, will fix. > > === > > --- linux-2.6-lttng.orig/kernel/module.c2007-08-10 19:44:18.0 > > -0400 > > +++ linux-2.6-lttng/kernel/module.c 2007-08-10 23:54:38.0 -0400 > > @@ -1980,6 +1986,10 @@ static struct module *load_module(void _ > > sechdrs[immediateindex].sh_size / > > sizeof(*mod->immediate); > > } > > #endif > > + if (markersindex) > > + sechdrs[markersindex].sh_flags |= SHF_ALLOC; > > + if (markersstringsindex) > > + sechdrs[markersstringsindex].sh_flags |= SHF_ALLOC; > > > > Perhaps I'm missing something, but I don't see why these sections > wouldn't be SHF_ALLOC already. > Declaring variables with __attribute__((section("__markers_strings"))) will likely put them in an allocated section, you are right. Will fix. The same applies to immediate values with .section __immediate, \"a\", @progbits, which is explicitely allocated. > > mod->unused_syms = (void *)sechdrs[unusedindex].sh_addr; > > if (unusedcrcindex) > > @@ -2021,6 +2031,13 @@ static struct module *load_module(void _ > > if (err < 0) > > goto cleanup; > > } > > +#ifdef CONFIG_MARKERS > > + if (markersindex) { > > + mod->markers = (void *)sechdrs[markersindex].sh_addr; > > + mod->num_markers = > > + sechdrs[markersindex].sh_size / sizeof(*mod->markers); > > + } > > +#endif > > Because of the wonders of ELF, section 0 has sh_addr and sh_size 0. So > the if (markersindex) is unnecessary here. > Ok. Will apply to immediate values too. > > +/* > > + * Get marker if the marker is present in the marker hash table. > > + * Must be called with markers_mutex held. > > + * Returns NULL if not present. > > + */ > > +static struct marker_entry *_get_marker(const char *name) > > You seem to really enjoy underscores, yet I'm having trouble > understanding why this would have an underscore in it. > Just they are internal functiona meant to be called with markers_mutex held. But I guess having a static prefix and not being exported is enough. Will remove. I'll just keep _marker_update_probes/marker_update_probes to differentiate between locked/non-locked version. > > +{ > > + struct hlist_head *head; > > + struct hlist_node *node; > > + struct marker_entry *e; > > + size_t len = strlen(name) + 1; > > + u32 hash = jhash(name, len-1, 0); > > + > > + head = _table[hash & ((1 << MARKER_HASH_BITS)-1)]; > > + hlist_for_each_entry(e, node, head, hlist) { > > + if (!strcmp(name, e->name)) > > + return e; > > + } > > + return NULL; > > +} > > OK, don't understand the strlen, len, len-1 dance here? > Let's say we have abc\0 for marker name as name input. len = 3 + 1 = 4 (including \0) hash is done only on the 3 first chars, excluding the \0 (therefore the len-1 there) Actually, it's like this only for a matter of consistency between add_marker and remove_marker, which are quite similar, but add_marker needs name_len to include the \0 value. It would be odd to change the logic between the two functions to one including the \0 and the other excluding it. > > +/* > > + * Updates the probe callback corresponding to a range of markers. > > + * Must be called with markers_mutex held. > > + */ > > +static void _marker_update_probe_range( > > And yet: > > > +void module_marker_update(struct module *mod) > > +{ > > + if (!mod->taints) > > + _marker_update_probe_range(mod->markers, > > + mod->markers+mod->num_markers, NULL, NULL); > > +} > > This doesn't hold the markers_mutex. > Since this function is only meant to be called upon module load, before the module is added to the module list, this marker range cannot be seen by other calls to marker_update_probe_range. But you are right. It also protects the hash table, so I should take the mutex there. Fixing. Thanks for the review, Mathieu > Cheers, > Rusty. > -- Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
* Rusty Russell ([EMAIL PROTECTED]) wrote: On Mon, 2007-08-20 at 16:27 -0400, Mathieu Desnoyers wrote: The marker activation functions sits in kernel/marker.c. A hash table is used to keep track of the registered probes and armed markers, so the markers within a newly loaded module that should be active can be activated at module load time. Hi Mathieu! Just reading through this patch, a couple of comments: Hi Rusty, +/* To be used for string format validity checking with gcc */ +static inline void __mark_check_format(const char *fmt, ...) + __attribute__ ((format (printf, 1, 2))); +static inline void __mark_check_format(const char *fmt, ...) { } If you place the __attribute__() before the function name, you can do this in the definition. Ok, will fix. === --- linux-2.6-lttng.orig/kernel/module.c2007-08-10 19:44:18.0 -0400 +++ linux-2.6-lttng/kernel/module.c 2007-08-10 23:54:38.0 -0400 @@ -1980,6 +1986,10 @@ static struct module *load_module(void _ sechdrs[immediateindex].sh_size / sizeof(*mod-immediate); } #endif + if (markersindex) + sechdrs[markersindex].sh_flags |= SHF_ALLOC; + if (markersstringsindex) + sechdrs[markersstringsindex].sh_flags |= SHF_ALLOC; Perhaps I'm missing something, but I don't see why these sections wouldn't be SHF_ALLOC already. Declaring variables with __attribute__((section(__markers_strings))) will likely put them in an allocated section, you are right. Will fix. The same applies to immediate values with .section __immediate, \a\, @progbits, which is explicitely allocated. mod-unused_syms = (void *)sechdrs[unusedindex].sh_addr; if (unusedcrcindex) @@ -2021,6 +2031,13 @@ static struct module *load_module(void _ if (err 0) goto cleanup; } +#ifdef CONFIG_MARKERS + if (markersindex) { + mod-markers = (void *)sechdrs[markersindex].sh_addr; + mod-num_markers = + sechdrs[markersindex].sh_size / sizeof(*mod-markers); + } +#endif Because of the wonders of ELF, section 0 has sh_addr and sh_size 0. So the if (markersindex) is unnecessary here. Ok. Will apply to immediate values too. +/* + * Get marker if the marker is present in the marker hash table. + * Must be called with markers_mutex held. + * Returns NULL if not present. + */ +static struct marker_entry *_get_marker(const char *name) You seem to really enjoy underscores, yet I'm having trouble understanding why this would have an underscore in it. Just they are internal functiona meant to be called with markers_mutex held. But I guess having a static prefix and not being exported is enough. Will remove. I'll just keep _marker_update_probes/marker_update_probes to differentiate between locked/non-locked version. +{ + struct hlist_head *head; + struct hlist_node *node; + struct marker_entry *e; + size_t len = strlen(name) + 1; + u32 hash = jhash(name, len-1, 0); + + head = marker_table[hash ((1 MARKER_HASH_BITS)-1)]; + hlist_for_each_entry(e, node, head, hlist) { + if (!strcmp(name, e-name)) + return e; + } + return NULL; +} OK, don't understand the strlen, len, len-1 dance here? Let's say we have abc\0 for marker name as name input. len = 3 + 1 = 4 (including \0) hash is done only on the 3 first chars, excluding the \0 (therefore the len-1 there) Actually, it's like this only for a matter of consistency between add_marker and remove_marker, which are quite similar, but add_marker needs name_len to include the \0 value. It would be odd to change the logic between the two functions to one including the \0 and the other excluding it. +/* + * Updates the probe callback corresponding to a range of markers. + * Must be called with markers_mutex held. + */ +static void _marker_update_probe_range( And yet: +void module_marker_update(struct module *mod) +{ + if (!mod-taints) + _marker_update_probe_range(mod-markers, + mod-markers+mod-num_markers, NULL, NULL); +} This doesn't hold the markers_mutex. Since this function is only meant to be called upon module load, before the module is added to the module list, this marker range cannot be seen by other calls to marker_update_probe_range. But you are right. It also protects the hash table, so I should take the mutex there. Fixing. Thanks for the review, Mathieu Cheers, Rusty. -- Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
On Mon, 2007-08-20 at 16:27 -0400, Mathieu Desnoyers wrote: > The marker activation functions sits in kernel/marker.c. A hash table is used > to keep track of the registered probes and armed markers, so the markers > within > a newly loaded module that should be active can be activated at module load > time. Hi Mathieu! Just reading through this patch, a couple of comments: > +/* To be used for string format validity checking with gcc */ > +static inline void __mark_check_format(const char *fmt, ...) > + __attribute__ ((format (printf, 1, 2))); > +static inline void __mark_check_format(const char *fmt, ...) { } If you place the __attribute__() before the function name, you can do this in the definition. > === > --- linux-2.6-lttng.orig/kernel/module.c 2007-08-10 19:44:18.0 > -0400 > +++ linux-2.6-lttng/kernel/module.c 2007-08-10 23:54:38.0 -0400 > @@ -1980,6 +1986,10 @@ static struct module *load_module(void _ > sechdrs[immediateindex].sh_size / > sizeof(*mod->immediate); > } > #endif > + if (markersindex) > + sechdrs[markersindex].sh_flags |= SHF_ALLOC; > + if (markersstringsindex) > + sechdrs[markersstringsindex].sh_flags |= SHF_ALLOC; > Perhaps I'm missing something, but I don't see why these sections wouldn't be SHF_ALLOC already. > mod->unused_syms = (void *)sechdrs[unusedindex].sh_addr; > if (unusedcrcindex) > @@ -2021,6 +2031,13 @@ static struct module *load_module(void _ > if (err < 0) > goto cleanup; > } > +#ifdef CONFIG_MARKERS > + if (markersindex) { > + mod->markers = (void *)sechdrs[markersindex].sh_addr; > + mod->num_markers = > + sechdrs[markersindex].sh_size / sizeof(*mod->markers); > + } > +#endif Because of the wonders of ELF, section 0 has sh_addr and sh_size 0. So the if (markersindex) is unnecessary here. > +/* > + * Get marker if the marker is present in the marker hash table. > + * Must be called with markers_mutex held. > + * Returns NULL if not present. > + */ > +static struct marker_entry *_get_marker(const char *name) You seem to really enjoy underscores, yet I'm having trouble understanding why this would have an underscore in it. > +{ > + struct hlist_head *head; > + struct hlist_node *node; > + struct marker_entry *e; > + size_t len = strlen(name) + 1; > + u32 hash = jhash(name, len-1, 0); > + > + head = _table[hash & ((1 << MARKER_HASH_BITS)-1)]; > + hlist_for_each_entry(e, node, head, hlist) { > + if (!strcmp(name, e->name)) > + return e; > + } > + return NULL; > +} OK, don't understand the strlen, len, len-1 dance here? > +/* > + * Updates the probe callback corresponding to a range of markers. > + * Must be called with markers_mutex held. > + */ > +static void _marker_update_probe_range( And yet: > +void module_marker_update(struct module *mod) > +{ > + if (!mod->taints) > + _marker_update_probe_range(mod->markers, > + mod->markers+mod->num_markers, NULL, NULL); > +} This doesn't hold the markers_mutex. Cheers, Rusty. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
On Mon, 2007-08-20 at 16:27 -0400, Mathieu Desnoyers wrote: The marker activation functions sits in kernel/marker.c. A hash table is used to keep track of the registered probes and armed markers, so the markers within a newly loaded module that should be active can be activated at module load time. Hi Mathieu! Just reading through this patch, a couple of comments: +/* To be used for string format validity checking with gcc */ +static inline void __mark_check_format(const char *fmt, ...) + __attribute__ ((format (printf, 1, 2))); +static inline void __mark_check_format(const char *fmt, ...) { } If you place the __attribute__() before the function name, you can do this in the definition. === --- linux-2.6-lttng.orig/kernel/module.c 2007-08-10 19:44:18.0 -0400 +++ linux-2.6-lttng/kernel/module.c 2007-08-10 23:54:38.0 -0400 @@ -1980,6 +1986,10 @@ static struct module *load_module(void _ sechdrs[immediateindex].sh_size / sizeof(*mod-immediate); } #endif + if (markersindex) + sechdrs[markersindex].sh_flags |= SHF_ALLOC; + if (markersstringsindex) + sechdrs[markersstringsindex].sh_flags |= SHF_ALLOC; Perhaps I'm missing something, but I don't see why these sections wouldn't be SHF_ALLOC already. mod-unused_syms = (void *)sechdrs[unusedindex].sh_addr; if (unusedcrcindex) @@ -2021,6 +2031,13 @@ static struct module *load_module(void _ if (err 0) goto cleanup; } +#ifdef CONFIG_MARKERS + if (markersindex) { + mod-markers = (void *)sechdrs[markersindex].sh_addr; + mod-num_markers = + sechdrs[markersindex].sh_size / sizeof(*mod-markers); + } +#endif Because of the wonders of ELF, section 0 has sh_addr and sh_size 0. So the if (markersindex) is unnecessary here. +/* + * Get marker if the marker is present in the marker hash table. + * Must be called with markers_mutex held. + * Returns NULL if not present. + */ +static struct marker_entry *_get_marker(const char *name) You seem to really enjoy underscores, yet I'm having trouble understanding why this would have an underscore in it. +{ + struct hlist_head *head; + struct hlist_node *node; + struct marker_entry *e; + size_t len = strlen(name) + 1; + u32 hash = jhash(name, len-1, 0); + + head = marker_table[hash ((1 MARKER_HASH_BITS)-1)]; + hlist_for_each_entry(e, node, head, hlist) { + if (!strcmp(name, e-name)) + return e; + } + return NULL; +} OK, don't understand the strlen, len, len-1 dance here? +/* + * Updates the probe callback corresponding to a range of markers. + * Must be called with markers_mutex held. + */ +static void _marker_update_probe_range( And yet: +void module_marker_update(struct module *mod) +{ + if (!mod-taints) + _marker_update_probe_range(mod-markers, + mod-markers+mod-num_markers, NULL, NULL); +} This doesn't hold the markers_mutex. Cheers, Rusty. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/