Re: [PATCH v3 2/5] tracing: Use __free() in trace_probe for cleanup

On Tue, 7 Jan 2025 20:34:32 -0500
Steven Rostedt [off-list ref] wrote:

On Wed, 8 Jan 2025 09:38:43 +0900
Masami Hiramatsu (Google) [off-list ref] wrote:

quoted

quoted

I don't get this? You are telling the compiler not to free tmp, because you
decided to free it yourself? Why not just remove the kfree() here altogether?  

In the for-loop block, the __free() work only when we exit the loop, not
each iteration. In each iteration, kstrdup() is assigned to the 'tmp',
so we need to kfree() each time.

Really? It doesn't trigger for each iteration? That's rather unintuitive. :-/
And sounds buggy, as wouldn't that then cause a memory leak?

Ahh, sorry, it was my misunderstood. I made a test code and confirmed that
kfree() is called in each iteration. Previously I checked but I confused the result.

----------
#include <stdio.h>

void count_func(int *p)
{
	printf("Scope out: %d\n", *p);
}

int main(void)
{
	for (int i = 0; i < 10; i++) {
		int j __attribute((cleanup(count_func))) = 0;

		j++;
	}
	return 0;
}
----------

$ ./loop_cleanup 
Scope out: 1
Scope out: 1
Scope out: 1
Scope out: 1
Scope out: 1
Scope out: 1
Scope out: 1
Scope out: 1
Scope out: 1
Scope out: 1

Let me fix that.

Thanks,

I would say not to use __free() for tmp at all. Because now it's just
getting confusing.

-- Steve

quoted

Hmm, maybe this is a sign that I should not use __free() for the 'tmp',
or I should call kfree(tmp) right before kstrdup(), like below.

 	for (i = 0; i < argc; i++) {
		char *tmp __free(kfree) = NULL;
		...
		kfree(tmp);
		tmp = kstrdup(argv[i], GFP_KERNEL);
	}

Does this make sense?

-- 
Masami Hiramatsu (Google) [off-list ref]