From: William Cohen <wcohen@redhat.com>
To: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: wcohen@redhat.com, systemtap@sourceware.org
Subject: Re: [Bug runtime/30831] Systemtap scripts fail to compile on newest linux 6.6 kernels
Date: Wed, 13 Dec 2023 09:47:02 -0500 [thread overview]
Message-ID: <d315d861-191d-4c69-a7fa-df3cd6d5e673@redhat.com> (raw)
In-Reply-To: <ae964c3e-6e10-4920-8479-59f209fa8b46@I-love.SAKURA.ne.jp>
[-- Attachment #1: Type: text/plain, Size: 3074 bytes --]
On 11/23/23 10:16, Tetsuo Handa wrote:
> On 2023/09/28 5:03, wcohen at redhat dot com via Systemtap wrote:
>> commit ca71442b93af61cbd9a1386e24e967373f928eae (HEAD -> master, origin/master,
>> origin/HEAD)
>> Author: William Cohen <wcohen@redhat.com>
>> Date: Wed Sep 27 10:09:11 2023 -0400
>>
>> Eliminate use of kernel's flush_scheduled_work() in systemtap modules
>>
>> Kernel git commit 20bdedafd2f63e0ba70991127f9b5c0826ebdb32 turns use
>> of flush_scheduled_work() into a warning which causes builds of
>> anything using it to fail because warnings are treated as errors.
>> Previous users of flush_scheduled_work() in the kernel have been
>> converted over to use individual workqueues. Systemtap runtime now
>> does the same. It creates and uses its own workqueue to eliminate the
>> use of flush_scheduled_work().
>
Hi,
Sorry for not getting to this earlier. Attached is a patch that attempts to address the problems mentioned in this email. Could you verify that the patch addresses all the concerns.
> That commit does not look correct.
>
> systemtap_wq = alloc_workqueue("systemtap-wq", WQ_UNBOUND | WQ_MEM_RECLAIM, 0);
> if (!systemtap_wq)
> return !systemtap_wq;
>
> causes stap_init_module() to return 1 (i.e. a positive value) when alloc_workqueue()
> failed. The caller ( https://elixir.bootlin.com/linux/v6.6/source/kernel/module/main.c#L2528 )
> emits a warning message but thinks that stap_init_module() succeeded.
> stap_init_module() should return -ENOMEM when alloc_workqueue() failed.
This area of code now return -ENOMEM if alloc_workqueue fails. It also eliminates the WQ_MEM_RECLAIM flag.
>
> Also, when alloc_workqueue() with WQ_MEM_RECLAIM flag succeeded, alloc_workqueue()
> allocated a "struct task_struct". Not calling destroy_workqueue(systemtap_wq) leaks
> that "struct task_struct" and related kernel resources.
>
> rc = systemtap_kernel_module_init();
> if (rc)
> return rc;
>
> I don't know the content of systemtap_kernel_module_init() emitted by translate.cxx ,
> but it seems to me that destroy_workqueue(systemtap_wq) will not be called.
> Like https://lkml.kernel.org/r/49925af7-78a8-a3dd-bce6-cfc02e1a9236@I-love.SAKURA.ne.jp
> says, please be sure to call destroy_workqueue(systemtap_wq) when stap_init_module()
> returns an error after alloc_workqueue() succeeded.
The stap_init_module is generated by the stap translator, but it doesn't call destroy_workqueue(). The code now calls destroy_workqueue before each of the returns in stap_init_module(). There is a use of the workqueue in systemtap_kernel_module_exit(), so the destroy_workqueue() follows it.
The stap_cleanup_module() was simplified as systemtap_wq must be available after the stap_init_module() is successful.
-Will
>
> Also, system_wq is created without WQ_MEM_RECLAIM flag
> ( https://elixir.bootlin.com/linux/v6.6/source/kernel/workqueue.c#L6596 ).
> If you don't have a reason to create systemtap_wq with WQ_MEM_RECLAIM flag,
> use of WQ_MEM_RECLAIM flag is a waste of kernel resource.
>
[-- Attachment #2: pr30831_revision.patch --]
[-- Type: text/x-patch, Size: 1069 bytes --]
diff --git a/runtime/linux/runtime.h b/runtime/linux/runtime.h
index a5840794a..d501f7990 100644
--- a/runtime/linux/runtime.h
+++ b/runtime/linux/runtime.h
@@ -434,15 +434,19 @@ static int stap_init_module (void)
map hash. */
get_random_bytes(&stap_hash_seed, sizeof (stap_hash_seed));
systemtap_wq = alloc_workqueue("systemtap-wq",
- WQ_UNBOUND | WQ_MEM_RECLAIM, 0);
+ WQ_UNBOUND, 0);
if (!systemtap_wq)
- return !systemtap_wq;
+ return -ENOMEM;
rc = systemtap_kernel_module_init();
- if (rc)
+ if (rc){
+ destroy_workqueue(systemtap_wq);
return rc;
+ }
rc = _stp_transport_init();
- if (rc)
+ if (rc){
systemtap_kernel_module_exit();
+ destroy_workqueue(systemtap_wq);
+ }
return rc;
}
@@ -455,8 +459,7 @@ void stap_cleanup_module(void)
due to tapset-procfs.cxx cleaning up after procfs probes (such
as in --monitor mode). */
systemtap_kernel_module_exit();
- if (systemtap_wq)
- destroy_workqueue(systemtap_wq);
+ destroy_workqueue(systemtap_wq);
}
module_exit(stap_cleanup_module);
next prev parent reply other threads:[~2023-12-13 14:47 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-07 17:46 [Bug runtime/30831] New: The do_each_thread macro has been removed from " wcohen at redhat dot com
2023-09-07 19:23 ` [Bug runtime/30831] Systemtap scripts fail to compile on " wcohen at redhat dot com
2023-09-08 2:49 ` wcohen at redhat dot com
2023-09-18 13:46 ` wcohen at redhat dot com
2023-09-18 13:51 ` wcohen at redhat dot com
2023-09-27 14:24 ` wcohen at redhat dot com
2023-09-27 20:03 ` wcohen at redhat dot com
2023-11-23 15:16 ` Tetsuo Handa
2023-12-13 14:47 ` William Cohen [this message]
2023-12-17 10:41 ` Tetsuo Handa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d315d861-191d-4c69-a7fa-df3cd6d5e673@redhat.com \
--to=wcohen@redhat.com \
--cc=penguin-kernel@I-love.SAKURA.ne.jp \
--cc=systemtap@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).