| Message ID | 20210609155657.26972-1-yanfei.xu@windriver.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/kmemleak: use READ_ONCE() for accessing jiffies_scan_wait | expand |
On 6/11/21 7:17 PM, Xu, Yanfei wrote: > > > On 6/11/21 4:59 PM, Catalin Marinas wrote: >> [Please note: This e-mail is from an EXTERNAL e-mail address] >> >> On Wed, Jun 09, 2021 at 11:56:57PM +0800, Yanfei Xu wrote: >>> The stop_scan_thread() and start_scan_thread() cannot really solve >>> the problem of concurrent accessing the global jiffies_scan_wait. >>> >>> kmemleak_write kmemleak_scan_thread >>> while (!kthread_should_stop()) >>> stop_scan_thread >>> jiffies_scan_wait = xxx timeout = jiffies_scan_wait >>> start_scan_thread >>> >>> We could replace these with a READ_ONCE() when reading >>> jiffies_scan_wait. It also can prevent compiler from reordering the >>> jiffies_scan_wait which is in while loop. >> >> I'm ok with READ_ONCE but your patch introduces functional changes. >> >>> diff --git a/mm/kmemleak.c b/mm/kmemleak.c >>> index 92a2d4885808..5ccf3969b7fe 100644 >>> --- a/mm/kmemleak.c >>> +++ b/mm/kmemleak.c >>> @@ -1567,7 +1567,7 @@ static int kmemleak_scan_thread(void *arg) >>> } >>> >>> while (!kthread_should_stop()) { >>> - signed long timeout = jiffies_scan_wait; >>> + signed long timeout = READ_ONCE(jiffies_scan_wait); >>> >>> mutex_lock(&scan_mutex); >>> kmemleak_scan(); >>> @@ -1812,11 +1812,8 @@ static ssize_t kmemleak_write(struct file >>> *file, const char __user *user_buf, >>> ret = kstrtoul(buf + 5, 0, &secs); >>> if (ret < 0) >>> goto out; >>> - stop_scan_thread(); >>> - if (secs) { >>> + if (secs) >>> jiffies_scan_wait = msecs_to_jiffies(secs * >>> 1000); >> >> For symmetry, I'd add a WRITE_ONCE here as well. >> >>> - start_scan_thread(); >>> - } >> >> The reason for stop/start_scan_thread() wasn't to protect against >> jiffies_scan_wait access but rather to force a new delay. Let's say you >> start by default with a 10min delay between scans (default) but you want >> to lower it to 1min. With the above removal of stop/start, you'd still >> have to wait for 10min until the scanning thread will notice the change. >> Also, with secs=0, the expectations is that the thread won't be >> restarted but this is removed by your patch. >> > > I see. > Thanks for your explain and sorry for my bad introduction. Will send a v2. > Hi Catalin and Andrew, I sent the v2 patch which is renamed to: [PATCH] mm/kmemleak: fix the possible wrong memory scanning period I have tested it on qemux86, and hope you can help to review. Thanks. --Yanfei > Thanks, > Yanfei > >> -- >> Catalin >>
diff --git a/mm/kmemleak.c b/mm/kmemleak.c index 92a2d4885808..5ccf3969b7fe 100644 --- a/mm/kmemleak.c +++ b/mm/kmemleak.c @@ -1567,7 +1567,7 @@ static int kmemleak_scan_thread(void *arg) } while (!kthread_should_stop()) { - signed long timeout = jiffies_scan_wait; + signed long timeout = READ_ONCE(jiffies_scan_wait); mutex_lock(&scan_mutex); kmemleak_scan(); @@ -1812,11 +1812,8 @@ static ssize_t kmemleak_write(struct file *file, const char __user *user_buf, ret = kstrtoul(buf + 5, 0, &secs); if (ret < 0) goto out; - stop_scan_thread(); - if (secs) { + if (secs) jiffies_scan_wait = msecs_to_jiffies(secs * 1000); - start_scan_thread(); - } } else if (strncmp(buf, "scan", 4) == 0) kmemleak_scan(); else if (strncmp(buf, "dump=", 5) == 0)
The stop_scan_thread() and start_scan_thread() cannot really solve the problem of concurrent accessing the global jiffies_scan_wait. kmemleak_write kmemleak_scan_thread while (!kthread_should_stop()) stop_scan_thread jiffies_scan_wait = xxx timeout = jiffies_scan_wait start_scan_thread We could replace these with a READ_ONCE() when reading jiffies_scan_wait. It also can prevent compiler from reordering the jiffies_scan_wait which is in while loop. Signed-off-by: Yanfei Xu <yanfei.xu@windriver.com> --- mm/kmemleak.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)