tracing: linux-next updates

From: "Steven Rostedt" <[email protected]>
To: [email protected]
Cc: "Ingo Molnar" <[email protected]>, "Frederic Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Mathieu Desnoyers" <[email protected]>, "Johannes Berg" <[email protected]>
Sent: Friday, March 7, 2014 10:09:28 AM
Subject: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

From: Steven Rostedt <[email protected]>

Tracepoints were made to allow enabling a tracepoint in a module before that module was loaded. When a tracepoint is enabled and it does not exist, the name is stored and will be enabled when the tracepoint is created.

The problem with this approach is that when a tracepoint is enabled when it expects to be there, it gives no warning that it does not exist.

To add salt to the wound, if a module is added and sets the FORCED flag, which
can happen if it isn't signed properly, the tracepoint code will not enabled the tracepoints, but they will be created in the debugfs system! When a user goes to enable the tracepoint, the tracepoint code will not see it existing and will think it is to be enabled later AND WILL NOT GIVE A WARNING.

The tracing will look like it succeeded but will actually be doing nothing. This will cause lots of confusion and headaches for developers trying to figure out why they are not seeing their tracepoints.

Link http://lkml.kernel.org/r/[email protected] open here

Reported-by Mathieu Desnoyers <[email protected]>

Reported-by Johannes Berg <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 kernel/tracepoint.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

kernel/tracepoint.c +2 -0

index 0d4ef26..0058f33 100644
@@ -62,6 +62,7 @@ struct tracepoint_entry {
 	struct hlist_node hlist;
 	struct tracepoint_func *funcs;
 	int refcount;	/* Number of times armed. 0 if disarmed. */
+	int enabled;	/* Tracepoint enabled */
 	char name[0];
 };
 

@@ -237,6 +238,7 @@ static struct tracepoint_entry *add_tracepoint(const char
 *name)
 	memcpy(&e->name[0], name, name_len);
 	e->funcs = NULL;
 	e->refcount = 0;
+	e->enabled = 0;
 	hlist_add_head(&e->hlist, head);
 	return e;

}

kernel/tracepoint.c +1 -0

@@ -316,6 +318,7 @@ static void tracepoint_update_probe_range(struct
 tracepoint * const *begin,
 		if (mark_entry) {
 			set_tracepoint(&mark_entry, *iter,
 					!!mark_entry->refcount);
+			mark_entry->enabled = !!mark_entry->refcount;
 		} else {
 			disable_tracepoint(*iter);

kernel/tracepoint.c +2 -0

 		}

@@ -380,6 +383,8 @@ tracepoint_add_probe(const char *name, void *probe, void
 *data)
 int tracepoint_probe_register(const char *name, void *probe, void *data)
 {
 	struct tracepoint_func *old;
+	struct tracepoint_entry *entry;
+	int ret = 0;
 
 	mutex_lock(&tracepoints_mutex);

kernel/tracepoint.c +0 -0

 	old = tracepoint_add_probe(name, probe, data);

kernel/tracepoint.c +1 -2

 	mutex_unlock(&tracepoints_mutex);
 	release_probes(old);
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(tracepoint_probe_register);
 
--

1.8.5.3

kernel/tracepoint.c int tracepoint_probe_register(const char *name, void · line 400

Quoted 11 lines

kernel/tracepoint.c +4 -0

 	mutex_lock(&tracepoints_mutex);
 	old = tracepoint_add_probe(name, probe, data);

@@ -388,9 +393,13 @@ int tracepoint_probe_register(const char *name, void
 *probe, void *data)
 		return PTR_ERR(old);
 	}
 	tracepoint_update_probes();		/* may update entry */
+	entry = get_tracepoint(name);
+	/* Make sure the entry was enabled */
+	if (!entry || !entry->enabled)
+		ret = -ENODEV;

Hi Steven,

Returning -ENODEV when the probe is still registered might come as a surprise to the caller. For instance, a caller may dynamically allocate name, probe, and/or data, it may want to free them when tracepoint_probe_register returns an error. But this "-ENODEV" return value is not really an error, and the parameters passed are still used.

If we go down this route, we might want at the very least to add documentation of tracepoint_probe_register() return values and their meaning
in a comment on top of this function (perhaps also in the header). But even if we do so, this weird return value semantic with respect to use of the received parameters will likely cause memory corruption at some point.

Thoughts ?

From: "Steven Rostedt" <[email protected]>
To: "Mathieu Desnoyers" <[email protected]>
Cc: [email protected], "Ingo Molnar" <[email protected]>, "Frederic Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Johannes Berg" <[email protected]>
Sent: Monday, March 10, 2014 4:19:27 PM
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

Quoted 14 lines · On Mon, 10 Mar 2014 20:01:34 +0000 (UTC) Mathieu Desnoyers…

kernel/tracepoint.c int tracepoint_probe_register(const char *name, void · line 400

Quoted 11 lines

kernel/tracepoint.c +4 -0

 	mutex_lock(&tracepoints_mutex);
 	old = tracepoint_add_probe(name, probe, data);

@@ -388,9 +393,13 @@ int tracepoint_probe_register(const char *name, void
 *probe, void *data)
 		return PTR_ERR(old);
 	}
 	tracepoint_update_probes();		/* may update entry */
+	entry = get_tracepoint(name);
+	/* Make sure the entry was enabled */
+	if (!entry || !entry->enabled)
+		ret = -ENODEV;

Hi Steven,

Quoted 5 lines

Returning -ENODEV when the probe is still registered might come as a surprise to the caller. For instance, a caller may dynamically allocate name, probe, and/or data, it may want to free them when tracepoint_probe_register returns an error. But this "-ENODEV" return value is not really an error, and the parameters passed are still used.

It's an error when you wanted to enable a probe and the probe doesn't exist. There are no in tree users of this call that expect it to work when the probe does not exist.

Quoted 10 lines

If we go down this route, we might want at the very least to add documentation
of tracepoint_probe_register() return values and their meaning in a comment on top of this function (perhaps also in the header). But even if we do so, this weird return value semantic with respect to use of the
received parameters will likely cause memory corruption at some point.

Thoughts ?

Send a patch to document the return values. Your module can expect this return value when it doesn't expect the probe to exist.

Again, it's really strange when you go to enable a probe, and there is no probe to enable.

Note, I was nice. I removed the logic to unregister the probe in this case.

Anyway, this should even help you. Before there was no way to enable a probe and know if it was enabled or not. That is, if it didn't exist, there was no feedback letting you know that.

If you expect to enable a probe that doesn't exist, then you can expect this return value.

Quoted 8 lines

From: Steven Rostedt <[email protected]>

Tracepoints were made to allow enabling a tracepoint in a module before that module was loaded. When a tracepoint is enabled and it does not exist, the name is stored and will be enabled when the tracepoint is created.

The problem with this approach is that when a tracepoint is enabled when it expects to be there, it gives no warning that it does not exist.

kernel/tracepoint.c int tracepoint_probe_register(const char *name, void *probe, void *data) · line 399

Quoted 4 lines

kernel/tracepoint.c +4 -0

+	entry = get_tracepoint(name);
+	/* Make sure the entry was enabled */
+	if (!entry || !entry->enabled)
+		ret = -ENODEV;

Hi -

Quoted 6 lines

Quoted 6 lines

From: Steven Rostedt <[email protected]>

Tracepoints were made to allow enabling a tracepoint in a module before that module was loaded. When a tracepoint is enabled and it does not exist, the name is stored and will be enabled when the tracepoint is created.

Quoted 2 lines

Quoted 2 lines

The problem with this approach is that when a tracepoint is enabled when it expects to be there, it gives no warning that it does not exist.

So it is a deferred-activation kind of call, with no way of knowing when or if the tracepoints will start coming in. Why is that supported at all, considering that clients could monitor modules coming & going via the module_notifier chain, and update registration at that time?

Quoted 6 lines

kernel/tracepoint.c int tracepoint_probe_register(const char *name, void *probe, void *data) · line 399

Quoted 6 lines

kernel/tracepoint.c +4 -0

+	entry = get_tracepoint(name);
+	/* Make sure the entry was enabled */
+	if (!entry || !entry->enabled)
+		ret = -ENODEV;

For what it's worth, I agree with Mathieu that this sort of successful failure result code is a problem for tracking what needs cleanup and what doesn't. (In systemtap's case, if this change gets merged, we'll have to treat -ENODEV as if it were 0.)

From: "Steven Rostedt" <[email protected]>
To: "Frank Ch. Eigler" <[email protected]>
Cc: "Mathieu Desnoyers" <[email protected]>, [email protected], "Ingo Molnar" <[email protected]>, "Frederic Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Johannes Berg" <[email protected]>
Sent: Monday, March 10, 2014 10:58:20 PM
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

Quoted 15 lines · On Mon, 10 Mar 2014 22:41:31 -0400 [email protected] (Frank Ch. Eigler) wrote:

Hi -

Quoted 10 lines

Quoted 10 lines

From: Steven Rostedt <[email protected]>

Tracepoints were made to allow enabling a tracepoint in a module before that
module was loaded. When a tracepoint is enabled and it does not exist, the
name is stored and will be enabled when the tracepoint is created.

The problem with this approach is that when a tracepoint is enabled when it expects to be there, it gives no warning that it does not exist.

Quoted 5 lines

So it is a deferred-activation kind of call, with no way of knowing when or if the tracepoints will start coming in. Why is that supported at all, considering that clients could monitor modules coming & going via the module_notifier chain, and update registration at that time?

That's my argument.

Quoted 12 lines

Quoted 7 lines

kernel/tracepoint.c int tracepoint_probe_register(const char *name, void *probe, void *data) · line 399

Quoted 7 lines

kernel/tracepoint.c +4 -0

+	entry = get_tracepoint(name);
+	/* Make sure the entry was enabled */
+	if (!entry || !entry->enabled)
+		ret = -ENODEV;

For what it's worth, I agree with Mathieu that this sort of successful failure result code is a problem for tracking what needs cleanup and what doesn't. (In systemtap's case, if this change gets merged, we'll have to treat -ENODEV as if it were 0.)

Does systemtap enable tracepoints before they are created? That is, do you allow enabling of a tracepoint in a module that is not loaded yet?

If not, than you want this as an error.

-- Steve

Quoted 1 line

That's my argument.

So basically, all we'd have to do in LTTng is to add a hash table tracking the tracepoint probes which are registered, but for which there are no tracepoint call sites. Whenever registration of a probe would fail due to

(quoted diff) +0 -2

-ENODEV (assuming we unregister the probe within tracepoint.c when we return
-ENODEV, as you initially proposed), we would put this probe in the hash table.

Upon module coming, we would iterate on the module's tracepoints and check if any of those match the content of the hash table, and then register the probe.

I guess I'd prefer that to the weird successful failure return value in tracepoint.c.

Quoted 5 lines

So it is a deferred-activation kind of call, with no way of knowing when or if the tracepoints will start coming in. Why is that supported at all, considering that clients could monitor modules coming & going via the module_notifier chain, and update registration at that time?

That's my argument.

Quoted 9 lines

Quoted 4 lines

kernel/tracepoint.c int tracepoint_probe_register(const char *name, void *probe, void *data) · line 399

Quoted 4 lines

kernel/tracepoint.c +4 -0

+	entry = get_tracepoint(name);
+	/* Make sure the entry was enabled */
+	if (!entry || !entry->enabled)
+		ret = -ENODEV;

For what it's worth, I agree with Mathieu that this sort of successful failure result code is a problem for tracking what needs cleanup and what doesn't. (In systemtap's case, if this change gets merged, we'll have to treat -ENODEV as if it were 0.)

Does systemtap enable tracepoints before they are created? That is, do you allow enabling of a tracepoint in a module that is not loaded yet?

If not, than you want this as an error.

From: "Frank Ch. Eigler" <[email protected]>
To: "Steven Rostedt" <[email protected]>
Cc: "Mathieu Desnoyers" <[email protected]>, [email protected], "Ingo Molnar" <[email protected]>, "Frederic Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Johannes Berg" <[email protected]>
Sent: Tuesday, March 11, 2014 10:26:50 AM
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

Hi, Steven -

Quoted 6 lines

Quoted 5 lines

So it is a deferred-activation kind of call, with no way of knowing when or if the tracepoints will start coming in. Why is that supported at all, considering that clients could monitor modules coming & going via the module_notifier chain, and update registration at that time?

Quoted 1 line

That's my argument.

Was there an answer?

Quoted 12 lines

Quoted 11 lines

Quoted 6 lines

kernel/tracepoint.c int tracepoint_probe_register(const char *name, void *probe, void *data) · line 399

Quoted 6 lines

kernel/tracepoint.c +4 -0

+	entry = get_tracepoint(name);
+	/* Make sure the entry was enabled */
+	if (!entry || !entry->enabled)
+		ret = -ENODEV;

For what it's worth, I agree with Mathieu that this sort of successful failure result code is a problem for tracking what needs cleanup and what doesn't. (In systemtap's case, if this change gets merged, we'll have to treat -ENODEV as if it were 0.)

Quoted 2 lines

Does systemtap enable tracepoints before they are created? That is, do you allow enabling of a tracepoint in a module that is not loaded yet?

We have no formal opinion on whether or not this makes sense. If the kernel permits it, fine.

Quoted 1 line

If not, than you want this as an error.

But it's not exactly an error! It's a success of sorts, and means that later on we have to unregister the callback, just as if it were successful.

- FChE

Let's step back and look at the overall picture here, along with the possible solutions that are available so far.

My intent is to let end users specify "I want to trace this specific tracepoint" from the tracer interface as long as the tracepoint probe provider module is loaded (the file with CREATE_TRACE_POINTS defined). For instance, the tracepoint call site can be within a driver loaded by the USB stack when a USB device is plugged in. While tracing is enabled, the user may want to plug in the said USB device to investigate what is the culprit of an issue he would be facing when the the device is plugged in.

There seems to be 2 elegant ways to achieve this while giving feedback to tracers about whether there are active tracepoint callsites or not:

1) We add a trace_has_callsites_enabled() (or any better name) function to tracepoint.h, which allows tracers to query whether a given tracepoint name has any callsites enabled,

2) We change tracepoint_probe_register() so it unregisters the tracepoint and return -ENODEV if there are no callsites enabled, and deal with the use-case I explain above with module coming and going within the tracer. This duplicates part of the tracepoint infrastructure into the tracer though, which is why I was not so keen on going for this solution.

The other solution proposed by Steven (returning -ENODEV without unregistering the tracepoint) does not appear to be an elegant solution, as we discussed earlier in this thread. It kind of weird to have a negative value treated as an OK special case.

The other solution proposed by Steven in an earlier thread was to tie tracepoints very deeply with module loading infrastructure and add module parameters specifically to specify if tracepoint callsites need to be enabled on module load. This approach unfortunately expect that everyone interacts with module loading, as root, in a system-wide (no multi-session)

fashion and is not suitable for the user-base we are targeting. I seems to be a user experience disaster IMHO.

I'm OK as long as we have an elegant way forward. Ideally I would have prefered (1) to eliminate code duplication between tracers and tracepoint infrastructure (we have to reimplement a hash table similar to tracepoints within the tracer with solution (2)), but (2) technically works too.

From: "Steven Rostedt" <[email protected]>
To: "Mathieu Desnoyers" <[email protected]>
Cc: "Frank Ch. Eigler" <[email protected]>, [email protected], "Ingo Molnar" <[email protected]>, "Frederic Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Johannes Berg" <[email protected]>
Sent: Tuesday, March 11, 2014 11:40:23 AM
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

Quoted 12 lines · On Tue, 11 Mar 2014 15:06:22 +0000 (UTC) Mathieu Desnoyers…

Let's step back and look at the overall picture here, along with the possible solutions that are available so far.

My intent is to let end users specify "I want to trace this specific tracepoint" from the tracer interface as long as the tracepoint probe provider module is loaded (the file with CREATE_TRACE_POINTS defined). For instance, the tracepoint call site can be within a driver loaded by the USB stack when a USB device is plugged in. While tracing is enabled, the user may want to plug in the said USB device to investigate what is the culprit of an issue he would be facing when the the device is plugged in.

Quoted 6 lines

There seems to be 2 elegant ways to achieve this while giving feedback to tracers about whether there are active tracepoint callsites or not:

1) We add a trace_has_callsites_enabled() (or any better name) function to tracepoint.h, which allows tracers to query whether a given tracepoint name has any callsites enabled,

I don't find that one very elegant at all.

Quoted 6 lines

2) We change tracepoint_probe_register() so it unregisters the tracepoint and return -ENODEV if there are no callsites enabled, and deal with the use-case I explain above with module coming and going within the tracer. This duplicates part of the tracepoint infrastructure into the tracer though, which is why I was not so keen on going for this solution.

I don't mind the above.

Quoted 5 lines

The other solution proposed by Steven (returning -ENODEV without unregistering the tracepoint) does not appear to be an elegant solution, as we discussed earlier in this thread. It kind of weird to have a negative value treated as an OK special case.

Right, but it was a compromise as you didn't like #2 earlier.

Quoted 1 line

Quoted 5 lines

The other solution proposed by Steven in an earlier thread was to tie tracepoints very deeply with module loading infrastructure and add module parameters specifically to specify if tracepoint callsites need to be enabled on module load. This approach unfortunately expect that everyone interacts with module loading, as root, in a system-wide (no multi-session)

Who can load modules not as root?? That is utterly broken. As once you can load a module, YOU ARE ROOT.

Quoted 3 lines

fashion and is not suitable for the user-base we are targeting. I seems to be a user experience disaster IMHO.

For your case only. But it is normal operation for normal uses of Linux.

Quoted 6 lines

I'm OK as long as we have an elegant way forward. Ideally I would have prefered (1) to eliminate code duplication between tracers and tracepoint infrastructure (we have to reimplement a hash table similar to tracepoints within the tracer with solution (2)), but (2) technically works too.

Here's what I propose then. We implement 2 for now. You can "duplicate" the code into your own work.

Then we should be able to simplify the
tracepoint code as it no longer will have the requirement to enable tracepoints that do not exist.

Also we can remove that tracepoint_probe_unregister_noupdate() as it has no in-tree users. Which, you have been lucky that no one noticed that yet, as that is a legitimate excuse to remove a function.

Quoted 2 lines

Who can load modules not as root?? That is utterly broken. As once you can load a module, YOU ARE ROOT.

udevd runs as root, and listens to events such as USB hotplug, and loads modules in the back of users. The users don't need to be root for this to happen.

Quoted 6 lines

Quoted 4 lines

fashion and is not suitable for the user-base we are targeting. I seems to be a user experience disaster IMHO.

For your case only. But it is normal operation for normal uses of Linux.

AFAIK pretty much all distros use udev nowadays. Are you suggesting that all users using udev and distribution kernels are not "normal uses of Linux" ?

Quoted 10 lines

Quoted 7 lines

I'm OK as long as we have an elegant way forward. Ideally I would have prefered (1) to eliminate code duplication between tracers and tracepoint infrastructure (we have to reimplement a hash table similar to tracepoints within the tracer with solution (2)), but (2) technically works too.

Here's what I propose then. We implement 2 for now. You can "duplicate" the code into your own work.

Works for me.

Quoted 3 lines

Then we should be able to simplify the
tracepoint code as it no longer will have the requirement to enable tracepoints that do not exist.

What happens for the case where we enable a tracepoint, and then the only module containing a callsite of that tracepoint is unloaded, and then reloaded ?

From: "Steven Rostedt" <[email protected]>
To: "Mathieu Desnoyers" <[email protected]>
Cc: "Frank Ch. Eigler" <[email protected]>, [email protected], "Ingo Molnar" <[email protected]>, "Frederic Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Johannes Berg" <[email protected]>
Sent: Tuesday, March 11, 2014 3:13:57 PM
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

Quoted 3 lines · On Tue, 11 Mar 2014 17:34:23 +0000 (UTC) Mathieu Desnoyers…

Quoted 2 lines

Who can load modules not as root?? That is utterly broken. As once you can load a module, YOU ARE ROOT.

Quoted 3 lines

udevd runs as root, and listens to events such as USB hotplug, and loads modules
in the back of users. The users don't need to be root for this to happen.

udevd may run as a proxy for users. But it is still a root user. Sysadmins are the ones that set up udevd.

If you own your own box, you are technically the sysadmin for it.

Modifications to udev require sudo
privileges.

Tracepoints should not be something a non-sysadmin can modify or enable. If you want non root users to do so, create a proxy daemon like udevd to do it for them. But the kernel isn't going to allow that directly.

Quoted 9 lines

Quoted 8 lines

Quoted 6 lines

fashion and is not suitable for the user-base we are targeting. I seems to
be a user experience disaster IMHO.

For your case only. But it is normal operation for normal uses of Linux.

Quoted 3 lines

AFAIK pretty much all distros use udev nowadays. Are you suggesting that all
users using udev and distribution kernels are not "normal uses of Linux" ?

udev is root, and is modified by root users. A normal user can not just interact with udev. And sticking in a usb stick into a computer counts as a sysadmin operation, even if the person doesn't official have the title.

Quoted 20 lines

Quoted 11 lines

Quoted 10 lines

I'm OK as long as we have an elegant way forward. Ideally I would have prefered (1) to eliminate code duplication between tracers and tracepoint
infrastructure (we have to reimplement a hash table similar to tracepoints
within the tracer with solution (2)), but (2) technically works too.

Quoted 2 lines

Here's what I propose then. We implement 2 for now. You can "duplicate" the code into your own work.

Works for me.

Quoted 3 lines

Then we should be able to simplify the
tracepoint code as it no longer will have the requirement to enable tracepoints that do not exist.

Quoted 3 lines

What happens for the case where we enable a tracepoint, and then the only module containing a callsite of that tracepoint is unloaded, and then reloaded ?

When a module is unloaded, it usually loses state. Is there any state that is maintained for a module being unloaded and reloaded again? Besides tracepoints? If not, then the module should lose its state for tracepoints as well.

----- Original Message -----

Quoted 8 lines

From: "Steven Rostedt" <[email protected]>
To: "Mathieu Desnoyers" <[email protected]>
Cc: "Frank Ch. Eigler" <[email protected]>, [email protected], "Ingo Molnar" <[email protected]>, "Frederic Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Johannes Berg" <[email protected]>
Sent: Tuesday, March 11, 2014 3:13:57 PM
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

For those just added in CC, https://lkml.org/lkml/2014/3/11/431 provides the missing context. The discussion is about adding module load parameters as ABI for end users to enable tracepoints in modules (Steven has proposed the idea a couple of times in the past, I am against for the reasons I am exposing below). This discussion will likely have deep impact on other changes Steven is proposing for tracepoint.c (such as making tracepoint_probe_register() return -ENODEV when no tracepoint call site is currently loaded for a given tracepoint), so I think the audience should be broadened.

Quoted 11 lines

Quoted 6 lines · On Tue, 11 Mar 2014 17:34:23 +0000 (UTC) Mathieu Desnoyers…

Quoted 2 lines

Who can load modules not as root?? That is utterly broken. As once you can load a module, YOU ARE ROOT.

udevd runs as root, and listens to events such as USB hotplug, and loads modules
in the back of users. The users don't need to be root for this to happen.

Quoted 2 lines

udevd may run as a proxy for users. But it is still a root user. Sysadmins are the ones that set up udevd.

Uh ? There are tons of distributions that setup udevd for their users, and no sysadmin has to interact with it.

Quoted 2 lines

If you own your own box, you are technically the sysadmin for it.

If you use a box owned by your company, running Linux, but enforcing module signature, you might be physically allowed to connect USB devices, and let udevd load the appropriate modules for the connected devices, but that does not mean you have root access at all. But having the ability to trace the said computer from a user having "tracing" privileges is very valuable to help debugging that machine.

Even without going that far, if you want tracing to be widely usable to pinpoint issues within Linux and/or its applications or libraries, you basically have to remove most user interactions from the picture, and let automated tools gather tracing data to perform analysis. As soon as you require users to fiddle with udev configuration, or pass special parameters to modprobe, or if you start coupling the tracing infrastructure with the module configuration, you just made tracing unusable for a vast majority of Linux distribution users out there.

I've been trying to make my point that tracing should be first of all _usable_ by non-expert Linux end users, but it seems that I'm completely failing at getting the message through somehow. And believe me I've spent years trying to voice this message. Perhaps I'm not cursing enough, perhaps I'm not yelling enough. Rather than arguing, the path I'm currently taking is to prove my point by putting the tools needed by my user community in their hands.

If to you the majority of Linux users means "kernel developers", then I can see how our point of views cannot be reconciled easily. From my point of view, there are far more users who need those tracing tools that are not kernel developers.

Quoted 8 lines

Modifications to udev require sudo
privileges.

Tracepoints should not be something a non-sysadmin can modify or enable. If you want non root users to do so, create a proxy daemon like udevd to do it for them. But the kernel isn't going to allow that directly.

I never said the kernel should directly expose ABIs accessible by others than root. Actually, this is the very reason we have a "lttng-sessiond" daemon running as root to control kernel tracing. However, it can be controlled by a "lttng" command line tool over unix sockets if the user is part of the "tracing" group. Right there, this is the concept of your proxy daemon. However, you seem to imply that this proxy daemon should somehow fiddle with udevd and/or modprobe configuration to add temporary system-wide parameters to how modules are loaded, depending on tracing needs. Trying to couple something usually configured initially for a large portion of a system's life (and even with tripwire validation that /etc does not change over time) with something as transient as tracing simply cannot work without causing havoc in the system configuration. So no, having a tracing proxy daemon changing configuration of module arguments cannot fly.

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Quoted 7 lines

fashion and is not suitable for the user-base we are targeting. I seems to
be a user experience disaster IMHO.

For your case only. But it is normal operation for normal uses of Linux.

Quoted 8 lines

Quoted 3 lines

AFAIK pretty much all distros use udev nowadays. Are you suggesting that all
users using udev and distribution kernels are not "normal uses of Linux" ?

udev is root, and is modified by root users. A normal user can not just interact with udev. And sticking in a usb stick into a computer counts as a sysadmin operation, even if the person doesn't official have the title.

So if I understand your line of thought, you imply that anyone who can plug a USB stick of any kind (memory stick, USB wifi card, etc) into a computer should automatically have root access ? Is it just me, or it makes no sense ?

Quoted 25 lines

Quoted 24 lines

Quoted 12 lines

Quoted 11 lines

I'm OK as long as we have an elegant way forward. Ideally I would have prefered (1) to eliminate code duplication between tracers and tracepoint
infrastructure (we have to reimplement a hash table similar to tracepoints
within the tracer with solution (2)), but (2) technically works too.

Quoted 2 lines

Here's what I propose then. We implement 2 for now. You can "duplicate" the code into your own work.

Works for me.

Quoted 3 lines

Then we should be able to simplify the
tracepoint code as it no longer will have the requirement to enable tracepoints that do not exist.

What happens for the case where we enable a tracepoint, and then the only module containing a callsite of that tracepoint is unloaded, and then reloaded ?

Quoted 4 lines

When a module is unloaded, it usually loses state. Is there any state that is maintained for a module being unloaded and reloaded again? Besides tracepoints? If not, then the module should lose its state for tracepoints as well.

What you're missing here is that the same tracepoint can appear within more than one module.

I think there is a core, fundamental difference between the ways we see tracing. You seem to see tracing as being solely part of the modules that contain the caller tracing code. From my perspective, tracing fits much better in the "aspect oriented programming" paradigm, since it spreads the same "crosscutting concerns" across multiple modules. So yes, in AOP, it makes perfect sense that tracing activation state goes beyond the lifetime of a single loaded module: it spawns across core kernel, and zero, one, or more loaded modules, independently of whether they are unloaded and reloaded again.

So going back on the changes you proposed to the tracepoint infrastructure:

A) Making tracepoint_probe_register() unregister the probe and fail when there is no tracepoint call site present for a probe at that time:

With this approach, if the probe is registered when the module containing the call site is loaded, and then that module is unloaded, it will leave the probe in a registered state, leaving the tracer lying to the user, letting them think there are currently callsites for this tracepoint when there are none.

B) Making tracepoint_probe_register() just return an error (-ENOEDV), but leaving the probe registered.

I think we have already agreed that this compromise solution is a weird API choice, since an error condition must be treated as a success by the caller. Moreover, it has the same issue as (A) above: if the module is unloaded after probe registration, the tracer is lying to the user, making them think there is a call site loaded when there are none.

C) Adding module load arguments to specify which tracepoint should be enabled.

Please refer to my response above in this email to understand why I object.

Back to your original requirement: you want to let the tracer communicate to end users whether a specific tracepoint has call sites or not. I'm currently preparing a patch implementing a "tracepoint_callsite_count()" API that will allow this, and it won't lie to the tracer like solutions
(A) and (B) above. Whenever the tracer queries the state, it will get the current information about the number of enabled call sites, not some stale data based on a prior registration error.

The solution I like the most that I believe will work for both of us, is to to move this magic "enable tracepoint in the future" to your LTTng module. Have your module register a module load and unload handler to be able to see the tracepoints that exist in the module, and you can enable them then. When a module is unloaded, your module can do the accounting to record that, and the state of its tracepoints.

Looks like we can have it both ways. A way that works well for the kernel, and a way that works well for you. But your module will need to do the heavy work for what you want.

To me, a tracepoint should only be enabled when it exists. If it is enabled in module when the module is unloaded, then it should be removed after the module has left. If the module is loaded again, it is up to the user (or your module) to enable that tracepoint again.

From: "Steven Rostedt" <[email protected]>
To: "Mathieu Desnoyers" <[email protected]>
Cc: "Frank Ch. Eigler" <[email protected]>, [email protected], "Ingo Molnar" <[email protected]>, "Frederic Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Johannes Berg" <[email protected]>, "Linus Torvalds" <[email protected]>, "Peter Zijlstra" <[email protected]>, "Thomas Gleixner" <[email protected]>, "Greg Kroah-Hartman" <[email protected]>, "lttng-dev" <[email protected]>, "Rusty Russell" <[email protected]>
Sent: Wednesday, March 12, 2014 11:46:11 AM
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

To sum up this thread, and get the signal vs noise ratio up.

Quoted 6 lines · On Wed, 12 Mar 2014 11:11:00 -0400 Steven Rostedt <[email protected]> wrote:

The solution I like the most that I believe will work for both of us, is to to move this magic "enable tracepoint in the future" to your LTTng module. Have your module register a module load and unload handler to be able to see the tracepoints that exist in the module, and you can enable them then. When a module is unloaded, your module can do the accounting to record that, and the state of its tracepoints.

This is my final proposal.

I'll add the patch that removes the tracepoint on failure along with returning -ENODEV. That way, there will be no registered tracepoints that do not exist.

I'll also make sure that on module unload, the tracepoints are disabled for the module as well.

Then, you can simply add a module notifier that does the work that you like, and save and restore the state of named tracepoints and enabled them on module load. Just set the priority of the notifier to 1 so that it runs after the tracepoint notifier that adds the new tracepoints to the system.

Quoted 6 lines

Looks like we can have it both ways. A way that works well for the kernel, and a way that works well for you. But your module will need to do the heavy work for what you want.

Quoted 4 lines

To me, a tracepoint should only be enabled when it exists. If it is enabled in module when the module is unloaded, then it should be removed after the module has left. If the module is loaded again, it is up to the user (or your module) to enable that tracepoint again.

I want to point out that LTTng should not be dictating the way the kernel works, but it should be the other way around.

-- Steve

----- Original Message -----

Quoted 30 lines

From: "Steven Rostedt" <[email protected]>
To: "Mathieu Desnoyers" <[email protected]>
Cc: "Frank Ch. Eigler" <[email protected]>, [email protected], "Ingo Molnar" <[email protected]>, "Frederic Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Johannes Berg" <[email protected]>, "Linus Torvalds" <[email protected]>, "Peter Zijlstra" <[email protected]>, "Thomas Gleixner" <[email protected]>, "Greg Kroah-Hartman" <[email protected]>, "lttng-dev" <[email protected]>, "Rusty Russell" <[email protected]>
Sent: Wednesday, March 12, 2014 11:46:11 AM
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

To sum up this thread, and get the signal vs noise ratio up.

Quoted 6 lines · On Wed, 12 Mar 2014 11:11:00 -0400 Steven Rostedt <[email protected]> wrote:

The solution I like the most that I believe will work for both of us, is to to move this magic "enable tracepoint in the future" to your LTTng module. Have your module register a module load and unload handler to be able to see the tracepoints that exist in the module, and you can enable them then. When a module is unloaded, your module can do the accounting to record that, and the state of its tracepoints.

This is my final proposal.

I'll add the patch that removes the tracepoint on failure along with returning -ENODEV. That way, there will be no registered tracepoints that do not exist.

Quoted 2 lines

I'll also make sure that on module unload, the tracepoints are disabled for the module as well.

Do you mean that the tracepoint probe will be unregistered from within tracepoint.c whenever all modules containing tracepoint call sites are unloaded ? If so, how do you plan to handle ownership of the "name", "probe" and "data" pointer ? They belong to the tracer. Would they simply leak ?

Quoted 7 lines

Then, you can simply add a module notifier that does the work that you like, and save and restore the state of named tracepoints and enabled them on module load. Just set the priority of the notifier to 1 so that it runs after the tracepoint notifier that adds the new tracepoints to the system.

I don't mind the extra work on the LTTng side at all. What I am concerned about are changes that would make the tracepoint API sloppy.

From: "Steven Rostedt" <[email protected]>
To: "Mathieu Desnoyers" <[email protected]>
Cc: "Frank Ch. Eigler" <[email protected]>, [email protected], "Ingo Molnar" <[email protected]>, "Frederic Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Johannes Berg" <[email protected]>, "Linus Torvalds" <[email protected]>, "Peter Zijlstra" <[email protected]>, "Thomas Gleixner" <[email protected]>, "Greg Kroah-Hartman" <[email protected]>, "lttng-dev" <[email protected]>, "Rusty Russell" <[email protected]>
Sent: Wednesday, March 12, 2014 12:18:13 PM
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

Quoted 41 lines · On Wed, 12 Mar 2014 16:05:36 +0000 (UTC) Mathieu Desnoyers…

----- Original Message -----

Quoted 39 lines

From: "Steven Rostedt" <[email protected]>
To: "Mathieu Desnoyers" <[email protected]>
Cc: "Frank Ch. Eigler" <[email protected]>, [email protected], "Ingo Molnar" <[email protected]>, "Frederic
Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Johannes Berg" <[email protected]>, "Linus Torvalds" <[email protected]>, "Peter Zijlstra" <[email protected]>, "Thomas Gleixner" <[email protected]>, "Greg Kroah-Hartman" <[email protected]>,
"lttng-dev" <[email protected]>, "Rusty Russell" <[email protected]>
Sent: Wednesday, March 12, 2014 11:46:11 AM
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

To sum up this thread, and get the signal vs noise ratio up.

Quoted 7 lines · On Wed, 12 Mar 2014 11:11:00 -0400 Steven Rostedt <[email protected]> wrote:

The solution I like the most that I believe will work for both of us, is to to move this magic "enable tracepoint in the future" to your LTTng module. Have your module register a module load and unload handler
to be able to see the tracepoints that exist in the module, and you can enable them then. When a module is unloaded, your module can do the accounting to record that, and the state of its tracepoints.

This is my final proposal.

I'll add the patch that removes the tracepoint on failure along with returning -ENODEV. That way, there will be no registered tracepoints that do not exist.

I'll also make sure that on module unload, the tracepoints are disabled for the module as well.

Quoted 5 lines

Do you mean that the tracepoint probe will be unregistered from within tracepoint.c whenever all modules containing tracepoint call sites are unloaded ? If so, how do you plan to handle ownership of the "name", "probe" and "data" pointer ? They belong to the tracer. Would they simply leak ?

Are you telling me it's not possible to delete the entire probe?

What I'm proposing is to do what the trace events do. Delete everything associated to the tracepoints associated to the module.

Quoted 1 line

Are you telling me it's not possible to delete the entire probe?

The ownership flow is the following:

1) Tracer creates name, probe, data objects. The probe can be typically code within a probe provider module, which needs to have a reference count incremented. The name and data objects can be dynamically allocated, or in some special cases part of a probe provider module (again with refcount incremented).

2) The tracer registers the tracepoint probe. If registration returns 0, the tracer should not free those elements until it calls tracepoint probe unregister for that (name, probe, data) tuple.

3) Tracer calls tracepoint probe unregister for the (name, probe, data) tuple.

4) Tracer calls tracepoint_synchronize_unregister() to ensure quiescence of tracepoint call sites wrt the probe that has just been unregistered.

5) Tracer can free/unref the probe provider module.

Quoted 4 lines

What I'm proposing is to do what the trace events do. Delete everything associated to the tracepoints associated to the module.

Is your intent to have a module "going" notifier in tracepoint.c managing ownership of objects it does not own ? If not, I guess I'm not understanding your proposal fully.

From: "Steven Rostedt" <[email protected]>
To: "Mathieu Desnoyers" <[email protected]>
Cc: "Frank Ch. Eigler" <[email protected]>, [email protected], "Ingo Molnar" <[email protected]>, "Frederic Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Johannes Berg" <[email protected]>, "Linus Torvalds" <[email protected]>, "Peter Zijlstra" <[email protected]>, "Thomas Gleixner" <[email protected]>, "Greg Kroah-Hartman" <[email protected]>, "lttng-dev" <[email protected]>, "Rusty Russell" <[email protected]>
Sent: Wednesday, March 12, 2014 1:50:59 PM
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

Quoted 15 lines · On Wed, 12 Mar 2014 16:39:55 +0000 (UTC) Mathieu Desnoyers…

Quoted 1 line

Are you telling me it's not possible to delete the entire probe?

The ownership flow is the following:

1) Tracer creates name, probe, data objects. The probe can be typically code within a probe provider module, which needs to have a reference count incremented. The name and data objects can be dynamically allocated,
or in some special cases part of a probe provider module (again with refcount incremented).

2) The tracer registers the tracepoint probe. If registration returns 0, the tracer should not free those elements until it calls tracepoint probe unregister for that (name, probe, data) tuple.

Quoted 2 lines

3) Tracer calls tracepoint probe unregister for the (name, probe, data) tuple.

We can make the registered tracepoint up the mod count to prevent it from unloading. But that probably defeats the purpose.

Quoted 4 lines

4) Tracer calls tracepoint_synchronize_unregister() to ensure quiescence of tracepoint call sites wrt the probe that has just been unregistered.

Tracepoints should all be disabled when the module is unloaded.

If you
have a tracepoint callback still being called when the module is unloaded then something is seriously wrong. That means the callback will go back to the trace_foo() call which is in the module code and will no longer exist. Kernel panic is the result.

Quoted 4 lines

5) Tracer can free/unref the probe provider module.

I'm a bit confused at what you are doing. As this is totally unrelated to anything that happens in the kernel.

Quoted 10 lines

Quoted 5 lines

What I'm proposing is to do what the trace events do. Delete everything associated to the tracepoints associated to the module.

Is your intent to have a module "going" notifier in tracepoint.c managing ownership of objects it does not own ? If not, I guess I'm not understanding
your proposal fully.

Well, actually that's exactly what the trace_event code does. It disables any event of the module that happens to be enabled.

Look at event_remove() in kernel/trace/trace_events.c

On module unload, the events are destroyed.

Thus, what your module should do, is exactly what event_remove() does. On module unload, you unregister any of the tracepoints that were registered. Just like any other module resource. If you request a resource on the behalf of a module, it is up to you to free it when the module is unloaded.

The tracepoint code will just destroy what it set up when the module was loaded. It's up to your module to clean up the allocations that you made when the module was loaded on unload. Just like we do for all other resources.

Mathieu, stop thinking that tracepoints are special. They are not.

-- Steve

Quoted 1 line

On module unload, the events are destroyed.

Isn't trace_event.c responsible for dealing with tracepoint probes rather than call sites ? This is quite different. A tracepoint probe "foo" is only located within a single module (the one you are unloading here). However, if you try to unload a module that contains the callsite "foo", you have no guarantee that no other modules also contain this callsite, and therefore you cannot destroy the associated name, probe, nor data objects.

Quoted 7 lines

Thus, what your module should do, is exactly what event_remove() does. On module unload, you unregister any of the tracepoints that were registered. Just like any other module resource. If you request a resource on the behalf of a module, it is up to you to free it when the module is unloaded.

You seem to try to apply a logic that works in the case of the probes defined by trace event to tracepoint call sites, but the fact is that they are very different. Or again perhaps I'm just on the wrong track.

Quoted 6 lines

The tracepoint code will just destroy what it set up when the module was loaded. It's up to your module to clean up the allocations that you made when the module was loaded on unload. Just like we do for all other resources.

Quoted 1 line

Mathieu, stop thinking that tracepoints are special. They are not.

I'm trying to understand how module going of tracepoint probes and call sites can be considered the same. What am I missing ?

Two modules should not have the same name. Is there any duplicate tracepoints you are aware of. Namespace collisions in tracepoints should be avoided, as that would cause people to trace things they did not intend on tracing.

That should be a new patch as well. Enforce unique tracepoint names.

Two modules should not have the same name. Is there any duplicate tracepoints you are aware of. Namespace collisions in tracepoints should be avoided, as that would cause people to trace things they did not intend on tracing.

That should be a new patch as well. Enforce unique tracepoint names.

From: "Steven Rostedt" <[email protected]>
To: "Mathieu Desnoyers" <[email protected]>
Cc: "Frank Ch. Eigler" <[email protected]>, [email protected], "Ingo Molnar" <[email protected]>, "Frederic Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Johannes Berg" <[email protected]>, "Linus Torvalds" <[email protected]>, "Peter Zijlstra" <[email protected]>, "Thomas Gleixner" <[email protected]>, "Greg Kroah-Hartman" <[email protected]>, "lttng-dev" <[email protected]>, "Rusty Russell" <[email protected]>
Sent: Wednesday, March 12, 2014 3:30:06 PM
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

Quoted 7 lines · On Wed, 12 Mar 2014 14:58:02 -0400 Steven Rostedt <[email protected]> wrote:

Two modules should not have the same name. Is there any duplicate tracepoints you are aware of. Namespace collisions in tracepoints should be avoided, as that would cause people to trace things they did not intend on tracing.

That should be a new patch as well. Enforce unique tracepoint names.

This may be why you are not understanding what I want. It's the way things are implemented today, which I believe are wrong. I see what you did. You have probes that are registered, and tracepoints that are where the code lies. You just add and remove probes from a hash list, and then you loop through all the tracepoints seeing if the iter->name matches a probe->name.

I'm fine with keeping the probe separate, but there really should be no more than just a one to one mapping between probes and tracepoints. Have the probe point to the matching tracepoint. The probe is registered, it enables the tracepoint static key, when it's ref count goes to zero, it disables the tracepoint static key. We can get rid of that loop then, as well as the duplicate names between probes and tracepoints.

Here's the steps we should take:

1) Prevent duplicate tracepoints. They are just namespace collisions that we already try to avoid. How to do this? We may need to add a hlist_node to the tracepoint structure, and keep them in a hash by name. Check for collisions when the name is added to the hash.

2) Change the way tracepoints are enabled. Do not do a loop of all tracepoints, but instead have the first probe of a tracepoint enable it, and the last one to disable it. This would require a pointer from the probe to the tracepoint it represents. Again, it should not represent more than one.

3) On module unload, it would be the responsibility of the user to unload all the tracepoints that may have been enabled for a module. We can add a mod pointer in the probe to make this easier, as well as to the tp_module structure.

The way tracepoints are today are to handle two completely different tracepoints with the same name. That should be avoided, and will make things much less complex.

Then you can easily handle the accounting of modules loading and unloading in your module, and the tracepoint code will match what the rest of the kernel does for resource management.

-- Steve

From: "Steven Rostedt" <[email protected]>
To: "Mathieu Desnoyers" <[email protected]>
Cc: "Frank Ch. Eigler" <[email protected]>, [email protected], "Ingo Molnar" <[email protected]>, "Frederic Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Johannes Berg" <[email protected]>, "Linus Torvalds" <[email protected]>, "Peter Zijlstra" <[email protected]>, "Thomas Gleixner" <[email protected]>, "Greg Kroah-Hartman" <[email protected]>, "lttng-dev" <[email protected]>, "Rusty Russell" <[email protected]>
Sent: Wednesday, March 12, 2014 2:58:02 PM
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

Quoted 8 lines · On Wed, 12 Mar 2014 18:47:15 +0000 (UTC) Mathieu Desnoyers…

Quoted 1 line

On module unload, the events are destroyed.

Isn't trace_event.c responsible for dealing with tracepoint probes rather than call sites ? This is quite different. A tracepoint probe "foo" is only located within a single module (the one you are unloading here). However, if you try to unload a module that contains the callsite "foo", you have no guarantee that no other modules also contain this callsite, and therefore you cannot destroy the associated name, probe, nor data objects.

No module should have the same tracepoint name as another module. That's just broken. Although we do not technically enforce this, in practice that has been the case.

Quoted 18 lines

Quoted 7 lines

Thus, what your module should do, is exactly what event_remove() does. On module unload, you unregister any of the tracepoints that were registered. Just like any other module resource. If you request a resource on the behalf of a module, it is up to you to free it when the module is unloaded.

You seem to try to apply a logic that works in the case of the probes defined by trace event to tracepoint call sites, but the fact is that they are very different. Or again perhaps I'm just on the wrong track.

Quoted 6 lines

The tracepoint code will just destroy what it set up when the module was loaded. It's up to your module to clean up the allocations that you made when the module was loaded on unload. Just like we do for all other resources.

Quoted 4 lines

Quoted 1 line

Mathieu, stop thinking that tracepoints are special. They are not.

I'm trying to understand how module going of tracepoint probes and call sites can be considered the same. What am I missing ?

What creates the tracepoint probe? For all purposes, it should be either created on boot up (on core tracepoints), or when a module is loaded.

Two modules should not have the same name. Is there any duplicate tracepoints you are aware of. Namespace collisions in tracepoints should be avoided, as that would cause people to trace things they did not intend on tracing.

That should be a new patch as well. Enforce unique tracepoint names.

-- Steve

So I understand that you wish to banish tracepoints from static inline functions within headers to ensure they only appear within a single module. This seems to be a step backward, but let's assume we stick to that rule. Then how do you envision dealing with Link-Time Optimisations (LTO) ?

From: "Andi Kleen" <[email protected]>
To: "Mathieu Desnoyers" <[email protected]>
Cc: "Steven Rostedt" <[email protected]>, "Frank Ch. Eigler" <[email protected]>, [email protected], "Ingo Molnar" <[email protected]>, "Frederic Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Johannes Berg" <[email protected]>, "Linus Torvalds" <[email protected]>, "Peter Zijlstra" <[email protected]>, "Thomas Gleixner" <[email protected]>, "Greg Kroah-Hartman" <[email protected]>, "lttng-dev" <[email protected]>, "Rusty Russell" <[email protected]>, "Andi Kleen" <[email protected]>
Sent: Wednesday, March 12, 2014 4:35:15 PM
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

Quoted 4 lines

So I understand that you wish to banish tracepoints from static inline functions within headers to ensure they only appear within a single module. This seems to be a step backward, but let's assume we stick to that rule. Then how do you envision dealing with Link-Time Optimisations (LTO) ?

I assume it uses the file name defines set by Kbuild?

These don't change with
LTO. It's whatever was specified at compile time. Also LTO doesn't inline over module boundaries (if the module is not built in)

-Andi

----- Original Message -----

Quoted 15 lines

From: "Andi Kleen" <[email protected]>
To: "Mathieu Desnoyers" <[email protected]>
Cc: "Steven Rostedt" <[email protected]>, "Frank Ch. Eigler" <[email protected]>, [email protected], "Ingo Molnar" <[email protected]>, "Frederic Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Johannes Berg" <[email protected]>, "Linus Torvalds" <[email protected]>, "Peter Zijlstra" <[email protected]>, "Thomas Gleixner" <[email protected]>, "Greg Kroah-Hartman" <[email protected]>, "lttng-dev" <[email protected]>, "Rusty Russell" <[email protected]>, "Andi Kleen" <[email protected]>
Sent: Wednesday, March 12, 2014 4:35:15 PM
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

Quoted 4 lines

So I understand that you wish to banish tracepoints from static inline functions within headers to ensure they only appear within a single module. This seems to be a step backward, but let's assume we stick to that rule. Then how do you envision dealing with Link-Time Optimisations (LTO) ?

Quoted 1 line

I assume it uses the file name defines set by Kbuild?

Just to make sure I understand your question: I understand that you are asking whether tracepoints use file name defines at all in the naming of a tracepoint. The answer to this question is: No, they do not.

Quoted 4 lines

These don't change with
LTO. It's whatever was specified at compile time. Also LTO doesn't inline over module boundaries (if the module is not built in)

Good to know. Can it inline core kernel functions into a module ?

From: "Andi Kleen" <[email protected]>
To: "Mathieu Desnoyers" <[email protected]>
Cc: "Andi Kleen" <[email protected]>, "Steven Rostedt" <[email protected]>, "Frank Ch. Eigler" <[email protected]>, [email protected], "Ingo Molnar" <[email protected]>, "Frederic Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Johannes Berg" <[email protected]>, "Linus Torvalds" <[email protected]>, "Peter Zijlstra" <[email protected]>, "Thomas Gleixner" <[email protected]>, "Greg Kroah-Hartman" <[email protected]>, "lttng-dev" <[email protected]>, "Rusty Russell" <[email protected]>
Sent: Wednesday, March 12, 2014 11:15:01 PM
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

Quoted 26 lines · Mathieu Desnoyers

----- Original Message -----

Quoted 24 lines

From: "Andi Kleen" <[email protected]>
To: "Mathieu Desnoyers" <[email protected]>
Cc: "Steven Rostedt" <[email protected]>, "Frank Ch. Eigler" <[email protected]>, [email protected], "Ingo Molnar" <[email protected]>, "Frederic Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>,
"Johannes Berg" <[email protected]>, "Linus Torvalds" <[email protected]>, "Peter Zijlstra" <[email protected]>, "Thomas Gleixner" <[email protected]>, "Greg Kroah-Hartman" <[email protected]>,
"lttng-dev" <[email protected]>, "Rusty Russell" <[email protected]>, "Andi Kleen" <[email protected]>
Sent: Wednesday, March 12, 2014 4:35:15 PM
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

Quoted 6 lines

So I understand that you wish to banish tracepoints from static inline functions within headers to ensure they only appear within a single module.
This seems to be a step backward, but let's assume we stick to that rule.
Then how do you envision dealing with Link-Time Optimisations (LTO) ?

I assume it uses the file name defines set by Kbuild?

Quoted 5 lines

Just to make sure I understand your question: I understand that you are asking
whether tracepoints use file name defines at all in the naming of a tracepoint.
The answer to this question is: No, they do not.

Ok. It uses kallsyms? That can change of course.

Quoted 6 lines

Quoted 4 lines

These don't change with
LTO. It's whatever was specified at compile time. Also LTO doesn't inline over module boundaries (if the module is not built in)

Good to know. Can it inline core kernel functions into a module ?

Each module and the main kernel are currently LTO'ed separately.

In theory it would be possible to change this, but likely at some compile time cost.

-Andi

----- Original Message -----

Quoted 15 lines

From: "Andi Kleen" <[email protected]>
To: "Mathieu Desnoyers" <[email protected]>
Cc: "Steven Rostedt" <[email protected]>, "Frank Ch. Eigler" <[email protected]>, [email protected], "Ingo Molnar" <[email protected]>, "Frederic Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Johannes Berg" <[email protected]>, "Linus Torvalds" <[email protected]>, "Peter Zijlstra" <[email protected]>, "Thomas Gleixner" <[email protected]>, "Greg Kroah-Hartman" <[email protected]>, "lttng-dev" <[email protected]>, "Rusty Russell" <[email protected]>, "Andi Kleen" <[email protected]>
Sent: Wednesday, March 12, 2014 4:35:15 PM
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

Quoted 4 lines

So I understand that you wish to banish tracepoints from static inline functions within headers to ensure they only appear within a single module. This seems to be a step backward, but let's assume we stick to that rule. Then how do you envision dealing with Link-Time Optimisations (LTO) ?

Quoted 1 line

I assume it uses the file name defines set by Kbuild?

Just to make sure I understand your question: I understand that you are asking whether tracepoints use file name defines at all in the naming of a tracepoint. The answer to this question is: No, they do not.

Quoted 4 lines

These don't change with
LTO. It's whatever was specified at compile time. Also LTO doesn't inline over module boundaries (if the module is not built in)

Good to know. Can it inline core kernel functions into a module ?

This only leaves tracepoints in header files and the impact of LTO as requirements for having tracepoint callsites with the same name across modules.

From: "Steven Rostedt" <[email protected]>
To: "Mathieu Desnoyers" <[email protected]>
Cc: "Frank Ch. Eigler" <[email protected]>, [email protected], "Ingo Molnar" <[email protected]>, "Frederic Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Johannes Berg" <[email protected]>, "Linus Torvalds" <[email protected]>, "Peter Zijlstra" <[email protected]>, "Thomas Gleixner" <[email protected]>, "Greg Kroah-Hartman" <[email protected]>, "lttng-dev" <[email protected]>, "Rusty Russell" <[email protected]>, "Andi Kleen" <[email protected]>
Sent: Wednesday, March 12, 2014 8:49:07 PM
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

Quoted 3 lines · On Wed, 12 Mar 2014 19:51:01 +0000 (UTC) Mathieu Desnoyers…

This only leaves tracepoints in header files and the impact of LTO as requirements for having tracepoint callsites with the same name across modules.

The only thing that needs to be unique is the struct tracepoint __tracepoint_##name. There should not be any duplicates of those. I can't see how the LTO would duplicate a data structure without screwing everything (not just tracepoints) up.

We can still have more than one trace_##name() called, as that is handled by the static key.

Note, I'm scrambling to get ready for my trip tomorrow. Thus, I'm not as much at the computer. I may work on some patches in my 6 hour layover though.

-- Steve

From: "Mathieu Desnoyers" <[email protected]>
To: "Steven Rostedt" <[email protected]>
Cc: "Frank Ch. Eigler" <[email protected]>, [email protected], "Ingo Molnar" <[email protected]>, "Frederic Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Johannes Berg" <[email protected]>, "Linus Torvalds" <[email protected]>, "Peter Zijlstra" <[email protected]>, "Thomas Gleixner" <[email protected]>, "Greg Kroah-Hartman" <[email protected]>, "lttng-dev" <[email protected]>, "Rusty Russell" <[email protected]>, "Andi Kleen" <[email protected]>
Sent: Wednesday, March 12, 2014 11:10:53 PM
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

----- Original Message -----

Quoted 23 lines

From: "Steven Rostedt" <[email protected]>
To: "Mathieu Desnoyers" <[email protected]>
Cc: "Frank Ch. Eigler" <[email protected]>, [email protected], "Ingo Molnar" <[email protected]>, "Frederic
Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Johannes Berg" <[email protected]>, "Linus Torvalds" <[email protected]>, "Peter Zijlstra" <[email protected]>, "Thomas Gleixner" <[email protected]>, "Greg Kroah-Hartman" <[email protected]>,
"lttng-dev" <[email protected]>, "Rusty Russell" <[email protected]>, "Andi Kleen" <[email protected]>
Sent: Wednesday, March 12, 2014 8:49:07 PM
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

Quoted 3 lines · On Wed, 12 Mar 2014 19:51:01 +0000 (UTC) Mathieu Desnoyers…

This only leaves tracepoints in header files and the impact of LTO as requirements for having tracepoint callsites with the same name across modules.

Quoted 7 lines

The only thing that needs to be unique is the struct tracepoint __tracepoint_##name. There should not be any duplicates of those. I can't see how the LTO would duplicate a data structure without screwing everything (not just tracepoints) up.

We can still have more than one trace_##name() called, as that is handled by the static key.

Hrm, I seem to have mixed up the concerns regarding compiler optimisations between the static keys with those related to tracepoints (including that their predecessors "kernel markers" worked more like static keys than tracepoint). I did work in both area pretty much at the same time back in 2007-2008.

Having gotten my head back up straight, I now see the point in your proposal. Thanks for bearing with me.

Even if we ever want to have tracepoints within header files, as long as we have the DECLARE_TRACE() within the header, and a DEFINE_TRACE() in one location in the loaded kernel image (or a loaded module we depend on), it should be possible too. All tracepoint.c cares about is the site defined by DEFINE_TRACE().

Quoted 5 lines

Note, I'm scrambling to get ready for my trip tomorrow. Thus, I'm not as much at the computer. I may work on some patches in my 6 hour layover though.

Allright. Have a good trip!

Thanks,

Mathieu

Quoted 3 lines

-- Steve

--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

From: "Steven Rostedt" <[email protected]>
To: "Mathieu Desnoyers" <[email protected]>
Cc: "Frank Ch. Eigler" <[email protected]>, [email protected], "Ingo Molnar" <[email protected]>, "Frederic Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Johannes Berg" <[email protected]>, "Linus Torvalds" <[email protected]>, "Peter Zijlstra" <[email protected]>, "Thomas Gleixner" <[email protected]>, "Greg Kroah-Hartman" <[email protected]>, "lttng-dev" <[email protected]>, "Rusty Russell" <[email protected]>
Sent: Wednesday, March 12, 2014 11:11:00 AM
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

Quoted 13 lines · On Wed, 12 Mar 2014 14:24:54 +0000 (UTC) Mathieu Desnoyers…

----- Original Message -----

Quoted 11 lines

From: "Steven Rostedt" <[email protected]>
To: "Mathieu Desnoyers" <[email protected]>
Cc: "Frank Ch. Eigler" <[email protected]>, [email protected], "Ingo Molnar" <[email protected]>, "Frederic
Weisbecker" <[email protected]>, "Andrew Morton" <[email protected]>, "Johannes Berg" <[email protected]>
Sent: Tuesday, March 11, 2014 3:13:57 PM
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

Quoted 12 lines

For those just added in CC, https://lkml.org/lkml/2014/3/11/431 provides the
missing context. The discussion is about adding module load parameters as ABI
for end users to enable tracepoints in modules (Steven has proposed the idea
a couple of times in the past, I am against for the reasons I am exposing below). This discussion will likely have deep impact on other changes Steven
is proposing for tracepoint.c (such as making tracepoint_probe_register() return -ENODEV when no tracepoint call site is currently loaded for a given tracepoint), so I think the audience should be broadened.

Please tell me one in-tree user that will be impacted?

Quoted 18 lines

Quoted 17 lines

Quoted 9 lines · On Tue, 11 Mar 2014 17:34:23 +0000 (UTC) Mathieu Desnoyers…

Quoted 3 lines

Who can load modules not as root?? That is utterly broken. As once you
can load a module, YOU ARE ROOT.

udevd runs as root, and listens to events such as USB hotplug, and loads
modules
in the back of users. The users don't need to be root for this to happen.

udevd may run as a proxy for users. But it is still a root user. Sysadmins are the ones that set up udevd.

Quoted 2 lines

Uh ? There are tons of distributions that setup udevd for their users, and no sysadmin has to interact with it.

And the distribution just magically installs itself onto a computer? No, a user does, and in doing so, they set up udev. A person installing a distribution is a sysadmin. Even if it's grandma installing it herself. She owns the box, no one else is doing anything for her (unless it's your grandma).

Quoted 4 lines

Quoted 3 lines

If you own your own box, you are technically the sysadmin for it.

Quoted 6 lines

If you use a box owned by your company, running Linux, but enforcing module signature, you might be physically allowed to connect USB devices, and let udevd load the appropriate modules for the connected devices, but that does not mean you have root access at all. But having the ability to trace the said computer from a user having "tracing" privileges is very valuable to help debugging that machine.

Tracing is a root privilege.

Quoted 10 lines

Even without going that far, if you want tracing to be widely usable to pinpoint issues within Linux and/or its applications or libraries, you basically have to remove most user interactions from the picture, and let automated tools gather tracing data to perform analysis. As soon as you require users to fiddle with udev configuration, or pass special parameters to modprobe, or if you start coupling the tracing infrastructure with the module configuration, you just made tracing unusable for a vast majority of Linux distribution users out there.

This seems very specific to a single tracing tool that happens not to be in the kernel tree.

Quoted 9 lines

I've been trying to make my point that tracing should be first of all _usable_ by non-expert Linux end users, but it seems that I'm completely failing at getting the message through somehow. And believe me I've spent years trying to voice this message. Perhaps I'm not cursing enough, perhaps I'm not yelling enough. Rather than arguing, the path I'm currently taking is to prove my point by putting the tools needed by my user community in their hands.

I don't know why you are complaining. Think of this as your job security.

Quoted 6 lines

If to you the majority of Linux users means "kernel developers", then I can see how our point of views cannot be reconciled easily. From my point of view, there are far more users who need those tracing tools that are not kernel developers.

No, but the majority of tracing users *are* kernel developers, or those that are experts in the system.

Why would grandma want to trace the kernel?

Quoted 10 lines

Quoted 9 lines

Modifications to udev require sudo
privileges.

Tracepoints should not be something a non-sysadmin can modify or enable. If you want non root users to do so, create a proxy daemon like udevd to do it for them. But the kernel isn't going to allow that directly.

Quoted 14 lines

I never said the kernel should directly expose ABIs accessible by others than root. Actually, this is the very reason we have a "lttng-sessiond" daemon running as root to control kernel tracing. However, it can be controlled by a "lttng" command line tool over unix sockets if the user is part of the "tracing" group. Right there, this is the concept of your proxy daemon. However, you seem to imply that this proxy daemon should somehow fiddle with udevd and/or modprobe configuration to add temporary system-wide parameters to how modules are loaded, depending on tracing needs. Trying to couple something usually configured initially for a large portion of a system's life (and even with tripwire validation that /etc does not change over time) with something as transient as tracing simply cannot work without causing havoc in the system configuration. So no, having a tracing proxy daemon changing configuration of module arguments cannot fly.

I actually never said that. I said that you could have a proxy to enable or disable the tracepoint. I don't know where you're getting this crap from. Yes, I have suggested in the past that the ideal approach is to have modprobe handle it, just like it handles other module changes for one time deals. And yes, tracing a module is a one time deal. But you seem to be getting in a tizzy thinking that's what I recommended yesterday.

Quoted 24 lines

Quoted 19 lines

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

fashion and is not suitable for the user-base we are targeting. I seems
to
be a user experience disaster IMHO.

Quoted 8 lines

Quoted 2 lines

For your case only. But it is normal operation for normal uses of Linux.

AFAIK pretty much all distros use udev nowadays. Are you suggesting that
all
users using udev and distribution kernels are not "normal uses of Linux" ?

Quoted 4 lines

udev is root, and is modified by root users. A normal user can not just interact with udev. And sticking in a usb stick into a computer counts as a sysadmin operation, even if the person doesn't official have the title.

Quoted 4 lines

So if I understand your line of thought, you imply that anyone who can plug a USB stick of any kind (memory stick, USB wifi card, etc) into a computer should automatically have root access ? Is it just me, or it makes no sense ?

I wouldn't want people to have access to any box I own to be able to just stick any usb device into the computer. Look at all the USB drivers we have. You think they are all perfect. I'm sure it wouldn't be too hard to find a bad USB device driver and make your own dongle to stick in, that can root the box.

Quoted 29 lines

Quoted 29 lines

Quoted 28 lines

Quoted 15 lines

Quoted 6 lines

Quoted 8 lines

I'm OK as long as we have an elegant way forward. Ideally I would have
prefered (1) to eliminate code duplication between tracers and tracepoint
infrastructure (we have to reimplement a hash table similar to tracepoints
within the tracer with solution (2)), but (2) technically works too.

Quoted 3 lines

Here's what I propose then. We implement 2 for now. You can "duplicate"
the code into your own work.

Works for me.

Quoted 3 lines

Then we should be able to simplify the
tracepoint code as it no longer will have the requirement to enable tracepoints that do not exist.

What happens for the case where we enable a tracepoint, and then the only module containing a callsite of that tracepoint is unloaded, and then reloaded ?

Quoted 7 lines

Quoted 4 lines

When a module is unloaded, it usually loses state. Is there any state that is maintained for a module being unloaded and reloaded again? Besides tracepoints? If not, then the module should lose its state for tracepoints as well.

What you're missing here is that the same tracepoint can appear within more than one module.

So?

Quoted 10 lines

I think there is a core, fundamental difference between the ways we see tracing. You seem to see tracing as being solely part of the modules that contain the caller tracing code. From my perspective, tracing fits much better in the "aspect oriented programming" paradigm, since it spreads the same "crosscutting concerns" across multiple modules. So yes, in AOP, it makes perfect sense that tracing activation state goes beyond the lifetime of a single loaded module: it spawns across core kernel, and zero, one, or more loaded modules, independently of whether they are unloaded and reloaded again.

Have your LTTNg module dictate that policy.

Quoted 8 lines

So going back on the changes you proposed to the tracepoint infrastructure:

A) Making tracepoint_probe_register() unregister the probe and fail when there is no tracepoint call site present for a probe at that time:

Quoted 5 lines

With this approach, if the probe is registered when the module containing the call site is loaded, and then that module is unloaded, it will leave the probe in a registered state, leaving the tracer lying to the user, letting them think there are currently callsites for this tracepoint when there are none.

Hmm, currently the probe lies to the user when the tracepoint doesn't exist. We enable it and nothing happens. This BROKE IN TREE USERS!!!!

I suggest that we remove the probe when the module is unloaded. No lying. Either a tracepoint exists when you enable it, or it doesn't.

Quoted 6 lines

B) Making tracepoint_probe_register() just return an error (-ENOEDV), but leaving the probe registered.

Quoted 6 lines

I think we have already agreed that this compromise solution is a weird API choice, since an error condition must be treated as a success by the caller.
Moreover, it has the same issue as (A) above: if the module is unloaded after probe registration, the tracer is lying to the user, making them think there is a call site loaded when there are none.

But isn't that what we do today?

Quoted 6 lines

C) Adding module load arguments to specify which tracepoint should be enabled.

Please refer to my response above in this email to understand why I object.

I didn't suggest this in this thread. I only pointed out that is the most sane situation. But I've been trying to work with you, and you don't seem to see that.

Quoted 9 lines

Back to your original requirement: you want to let the tracer communicate to end users whether a specific tracepoint has call sites or not. I'm currently preparing a patch implementing a "tracepoint_callsite_count()" API that will allow this, and it won't lie to the tracer like solutions
(A) and (B) above. Whenever the tracer queries the state, it will get the current information about the number of enabled call sites, not some stale data based on a prior registration error.

No, I don't want to know if this call site exists or not. This stupid "tracepoint_probe_register()" that activates a tracepoint by name. But if that tracepoint doesn't exist, it just returns normally as if everything is fine and dandy. WTF!

Look Mathieu, I've been trying to be nice here. This crap has broken in tree kernel users, and this magical "enable a non-existent tracepoint, so when one is created, it will be enabled" is only used by LTTng! NOBODY ELSE USES IT. This means that I can strip this usage out of the kernel, as there are no uses of it in the kernel.

What you don't seem to realize, is that your methods broke the stuff that is in the kernel, and you don't seem to give a shit about that.

The solution I like the most that I believe will work for both of us, is to to move this magic "enable tracepoint in the future" to your LTTng module. Have your module register a module load and unload handler to be able to see the tracepoints that exist in the module, and you can enable them then. When a module is unloaded, your module can do the accounting to record that, and the state of its tracepoints.

Looks like we can have it both ways. A way that works well for the kernel, and a way that works well for you. But your module will need to do the heavy work for what you want.

To me, a tracepoint should only be enabled when it exists. If it is enabled in module when the module is unloaded, then it should be removed after the module has left. If the module is loaded again, it is up to the user (or your module) to enable that tracepoint again.

-- Steve

Quoted 1 line

Please tell me one in-tree user that will be impacted?

I'm talking about all users of tracepoints, all in tree and out of tree users. We need the design and API for tracepoint to stay sound, and for that we need to study carefully the impact of the changes you propose on module load/unload scenarios.

Quoted 5 lines

And the distribution just magically installs itself onto a computer? No, a user does, and in doing so, they set up udev. A person installing a distribution is a sysadmin. Even if it's grandma installing it herself. She owns the box, no one else is doing anything for her (unless it's your grandma).

In a corporate environment, it's pretty much never the end user who ends up being the system administrator of his machine.

Quoted 1 line

Tracing is a root privilege.

Tracing the kernel should require root privilege for the process interacting with the kernel ABI, I agree with that part. However, the end user of the machine should not need to be in a root shell to interact with tracing. For instance, if the tracer only provides summarized information pinpointing issues, the system administrator could very well allow the end user to use tracing without being root.

An example of this is the way laptops nowadays handle wifi connexions through a root daemon setting up the network (interfacing with kernel ABI), but which gets the user interaction though a user interface running in user context.

Quoted 2 lines

This seems very specific to a single tracing tool that happens not to be in the kernel tree.

And the reason for that is because the said kernel community has been deaf to the requirements of the user-base we target. Unlike Ftrace, we are not targeting kernel developers, and unlike perf, we are not targeting sampling use-cases. Yes, there is a partial infrastructure overlap, but LTTng has been rejected on the ground of "fear of work duplication" which, frankly, makes no sense, since we are targeting different use cases. Some of the infrastructure work needs to be done in common, and some of the work needs to go in different directions.

Quoted 2 lines

Why would grandma want to trace the kernel?

Perhaps because her kernel crashes and she hates to lose the emails she is writing to her grandchildren when the crash happens, and because there is a nice tool that allow her to click "yes, I want to report issues to my favorite Linux distribution", which will then analyze the trace report and come up with an automated kernel upgrade a few weeks later.

Quoted 7 lines

I actually never said that. I said that you could have a proxy to enable or disable the tracepoint. I don't know where you're getting this crap from. Yes, I have suggested in the past that the ideal approach is to have modprobe handle it, just like it handles other module changes for one time deals. And yes, tracing a module is a one time deal. But you seem to be getting in a tizzy thinking that's what I recommended yesterday.

This was the logical consequence of your recommendation of having a root proxy daemon to handle this.

Quoted 5 lines

I wouldn't want people to have access to any box I own to be able to just stick any usb device into the computer. Look at all the USB drivers we have. You think they are all perfect. I'm sure it wouldn't be too hard to find a bad USB device driver and make your own dongle to stick in, that can root the box.

Yes, no code is perfect, and that includes USB drivers. But in that line of reasoning, why bother about security at all ? Since the ssh server code is imperfect, you might as well publish your private key. Sorry, this line of reasoning makes no sense.

Quoted 1 line

Have your LTTNg module dictate that policy.

I'm OK with that, as long as the tracepoint infrastructure itself is consistent.

Quoted 2 lines

Hmm, currently the probe lies to the user when the tracepoint doesn't exist. We enable it and nothing happens. This BROKE IN TREE USERS!!!!

The root of the original problem here was that tracepoint.c was skipping non-signed modules without printing any error. We added an printk error message for this.

Quoted 4 lines

I suggest that we remove the probe when the module is unloaded. No lying. Either a tracepoint exists when you enable it, or it doesn't.

See other leg of this thread for the "high signal/noise" ratio technical discussion.

Quoted 13 lines

Quoted 12 lines

B) Making tracepoint_probe_register() just return an error (-ENOEDV), but leaving the probe registered.

I think we have already agreed that this compromise solution is a weird API choice, since an error condition must be treated as a success by the caller.
Moreover, it has the same issue as (A) above: if the module is unloaded after probe registration, the tracer is lying to the user, making them think there is a call site loaded when there are none.

Quoted 1 line

But isn't that what we do today?

No, currently tracepoint offers no API to query the state of tracepoint callsites other than the seqfile iterator (which I proposed to removed in a RFC patch yesterday).

Quoted 8 lines

Quoted 7 lines

C) Adding module load arguments to specify which tracepoint should be enabled.

Please refer to my response above in this email to understand why I object.

Quoted 3 lines

I didn't suggest this in this thread. I only pointed out that is the most sane situation. But I've been trying to work with you, and you don't seem to see that.

I appreciate the compromise efforts you have made, but my points here are about sound design/API, and I have technical concerns still unanswered (see other leg of the thread). This is not about what LTTng needs, but about keeping the tracepoint API consistent.

I don't mind doing the heavy lifting in LTTng at all. My concern here is (again) about keeping the tracepoint API semantically consistent when module load/unload occur. A lot of thinking went into this when I initially created tracepoint.c, and I don't want it to be degraded by a change that is not thought thoroughly.