Messages in this thread | | | Subject | Re: [PATCH] mm/kmemleak: use READ_ONCE() for accessing jiffies_scan_wait | From | "Xu, Yanfei" <> | Date | Fri, 11 Jun 2021 19:17:46 +0800 |
| |
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.
Thanks, Yanfei
> -- > Catalin >
| |