2022-08-24 07:53:35

by Kefeng Wang

Subject: [PATCH 2/2] mm: slince possible data races about pgdat->kswapd

The pgdat->kswapd could be accessed concurrently by kswapd_run() and
kcompactd(), it don't be protected by any lock, which could leads to
data races, adding READ/WRITE_ONCE() to slince it.

Signed-off-by: Kefeng Wang <[email protected]>
---
mm/compaction.c | 4 +++-
mm/vmscan.c | 8 ++++----
2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 640fa76228dd..aa1cfe47f046 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1983,7 +1983,9 @@ static inline bool is_via_compact_memory(int order)

static bool kswapd_is_running(pg_data_t *pgdat)
{
- return pgdat->kswapd && task_is_running(pgdat->kswapd);
+ struct task_struct *t = READ_ONCE(pgdat->kswapd);
+
+ return t && task_is_running(t);
}

/*
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 08c6497f76c3..65b19ca8c8ee 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -4644,7 +4644,7 @@ void kswapd_run(int nid)
pg_data_t *pgdat = NODE_DATA(nid);
struct task_struct *t;

- if (pgdat->kswapd)
+ if (READ_ONCE(pgdat->kswapd))
return;

t = kthread_run(kswapd, pgdat, "kswapd%d", nid);
@@ -4653,7 +4653,7 @@ void kswapd_run(int nid)
BUG_ON(system_state < SYSTEM_RUNNING);
pr_err("Failed to start kswapd on node %d\n", nid);
} else {
- pgdat->kswapd = t;
+ WRITE_ONCE(pgdat->kswapd, t);
}
}

@@ -4663,11 +4663,11 @@ void kswapd_run(int nid)
*/
void kswapd_stop(int nid)
{
- struct task_struct *kswapd = NODE_DATA(nid)->kswapd;
+ struct task_struct *kswapd = READ_ONCE(NODE_DATA(nid)->kswapd);

if (kswapd) {
kthread_stop(kswapd);
- NODE_DATA(nid)->kswapd = NULL;
+ WRITE_ONCE(NODE_DATA(nid)->kswapd, NULL);
}
}

--
2.35.3

2022-08-24 08:32:45

by David Hildenbrand

Subject: Re: [PATCH 2/2] mm: slince possible data races about pgdat->kswapd

On 24.08.22 09:19, Kefeng Wang wrote:
> The pgdat->kswapd could be accessed concurrently by kswapd_run() and
> kcompactd(), it don't be protected by any lock, which could leads to
> data races, adding READ/WRITE_ONCE() to slince it.

Okay, I think this patch here makes it clearer that we really just want
proper synchronization instead of hacking around it.

What speaks against protecting pgdat->kswapd this using some proper
locking primitive?

>
> Signed-off-by: Kefeng Wang <[email protected]>
> ---
> mm/compaction.c | 4 +++-
> mm/vmscan.c | 8 ++++----
> 2 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 640fa76228dd..aa1cfe47f046 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1983,7 +1983,9 @@ static inline bool is_via_compact_memory(int order)
>
> static bool kswapd_is_running(pg_data_t *pgdat)
> {
> - return pgdat->kswapd && task_is_running(pgdat->kswapd);
> + struct task_struct *t = READ_ONCE(pgdat->kswapd);
> +
> + return t && task_is_running(t);
> }
>
> /*
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 08c6497f76c3..65b19ca8c8ee 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -4644,7 +4644,7 @@ void kswapd_run(int nid)
> pg_data_t *pgdat = NODE_DATA(nid);
> struct task_struct *t;
>
> - if (pgdat->kswapd)
> + if (READ_ONCE(pgdat->kswapd))
> return;
>
> t = kthread_run(kswapd, pgdat, "kswapd%d", nid);
> @@ -4653,7 +4653,7 @@ void kswapd_run(int nid)
> BUG_ON(system_state < SYSTEM_RUNNING);
> pr_err("Failed to start kswapd on node %d\n", nid);
> } else {
> - pgdat->kswapd = t;
> + WRITE_ONCE(pgdat->kswapd, t);
> }
> }
>
> @@ -4663,11 +4663,11 @@ void kswapd_run(int nid)
> */
> void kswapd_stop(int nid)
> {
> - struct task_struct *kswapd = NODE_DATA(nid)->kswapd;
> + struct task_struct *kswapd = READ_ONCE(NODE_DATA(nid)->kswapd);
>
> if (kswapd) {
> kthread_stop(kswapd);
> - NODE_DATA(nid)->kswapd = NULL;
> + WRITE_ONCE(NODE_DATA(nid)->kswapd, NULL);
> }
> }
>


--
Thanks,

David / dhildenb

2022-08-24 10:07:48

by Kefeng Wang

Subject: Re: [PATCH 2/2] mm: slince possible data races about pgdat->kswapd

On 2022/8/24 16:24, David Hildenbrand wrote:
> On 24.08.22 09:19, Kefeng Wang wrote:
>> The pgdat->kswapd could be accessed concurrently by kswapd_run() and
>> kcompactd(), it don't be protected by any lock, which could leads to
>> data races, adding READ/WRITE_ONCE() to slince it.
> Okay, I think this patch here makes it clearer that we really just want
> proper synchronization instead of hacking around it.
>
> What speaks against protecting pgdat->kswapd this using some proper
> locking primitive?

So add a new lock into struct pglist_data to protect pgdat->kswapd，other

option, thanks.

>> Signed-off-by: Kefeng Wang <[email protected]>
>> ---
>> mm/compaction.c | 4 +++-
>> mm/vmscan.c | 8 ++++----
>> 2 files changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/mm/compaction.c b/mm/compaction.c
>> index 640fa76228dd..aa1cfe47f046 100644
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -1983,7 +1983,9 @@ static inline bool is_via_compact_memory(int order)
>>
>> static bool kswapd_is_running(pg_data_t *pgdat)
>> {
>> - return pgdat->kswapd && task_is_running(pgdat->kswapd);
>> + struct task_struct *t = READ_ONCE(pgdat->kswapd);
>> +
>> + return t && task_is_running(t);
>> }
>>
>> /*
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index 08c6497f76c3..65b19ca8c8ee 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -4644,7 +4644,7 @@ void kswapd_run(int nid)
>> pg_data_t *pgdat = NODE_DATA(nid);
>> struct task_struct *t;
>>
>> - if (pgdat->kswapd)
>> + if (READ_ONCE(pgdat->kswapd))
>> return;
>>
>> t = kthread_run(kswapd, pgdat, "kswapd%d", nid);
>> @@ -4653,7 +4653,7 @@ void kswapd_run(int nid)
>> BUG_ON(system_state < SYSTEM_RUNNING);
>> pr_err("Failed to start kswapd on node %d\n", nid);
>> } else {
>> - pgdat->kswapd = t;
>> + WRITE_ONCE(pgdat->kswapd, t);
>> }
>> }
>>
>> @@ -4663,11 +4663,11 @@ void kswapd_run(int nid)
>> */
>> void kswapd_stop(int nid)
>> {
>> - struct task_struct *kswapd = NODE_DATA(nid)->kswapd;
>> + struct task_struct *kswapd = READ_ONCE(NODE_DATA(nid)->kswapd);
>>
>> if (kswapd) {
>> kthread_stop(kswapd);
>> - NODE_DATA(nid)->kswapd = NULL;
>> + WRITE_ONCE(NODE_DATA(nid)->kswapd, NULL);
>> }
>> }
>>
>

2022-08-25 03:05:54

by Kefeng Wang

Subject: Re: [PATCH 2/2] mm: slince possible data races about pgdat->kswapd

On 2022/8/24 16:24, David Hildenbrand wrote:
> On 24.08.22 09:19, Kefeng Wang wrote:
>> The pgdat->kswapd could be accessed concurrently by kswapd_run() and
>> kcompactd(), it don't be protected by any lock, which could leads to
>> data races, adding READ/WRITE_ONCE() to slince it.
> Okay, I think this patch here makes it clearer that we really just want
> proper synchronization instead of hacking around it.
>
> What speaks against protecting pgdat->kswapd this using some proper
> locking primitive?

as comments about kswapd in struct pglist_data,  pgdat->kswapd should be

protected by mem_hotplug_begin/done(), how about this way?

diff --git a/mm/compaction.c b/mm/compaction.c
index 640fa76228dd..62018f35242a 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1983,7 +1983,13 @@ static inline bool is_via_compact_memory(int order)

 static bool kswapd_is_running(pg_data_t *pgdat)
 {
-       return pgdat->kswapd && task_is_running(pgdat->kswapd);
+       bool running;
+
+       mem_hotplug_begin();
+       running = pgdat->kswapd && task_is_running(pgdat->kswapd);
+       mem_hotplug_end();
+
+       return running;
 }

2022-08-25 08:27:02

by David Hildenbrand

Subject: Re: [PATCH 2/2] mm: slince possible data races about pgdat->kswapd

On 25.08.22 04:34, Kefeng Wang wrote:
>
> On 2022/8/24 16:24, David Hildenbrand wrote:
>> On 24.08.22 09:19, Kefeng Wang wrote:
>>> The pgdat->kswapd could be accessed concurrently by kswapd_run() and
>>> kcompactd(), it don't be protected by any lock, which could leads to
>>> data races, adding READ/WRITE_ONCE() to slince it.
>> Okay, I think this patch here makes it clearer that we really just want
>> proper synchronization instead of hacking around it.
>>
>> What speaks against protecting pgdat->kswapd this using some proper
>> locking primitive?
>
> as comments about kswapd in struct pglist_data,  pgdat->kswapd should be
>
> protected by mem_hotplug_begin/done(), how about this way?
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 640fa76228dd..62018f35242a 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1983,7 +1983,13 @@ static inline bool is_via_compact_memory(int order)
>
>  static bool kswapd_is_running(pg_data_t *pgdat)
>  {
> -       return pgdat->kswapd && task_is_running(pgdat->kswapd);
> +       bool running;
> +
> +       mem_hotplug_begin();
> +       running = pgdat->kswapd && task_is_running(pgdat->kswapd);
> +       mem_hotplug_end();
> +
> +       return running;
>  }

I'd much rather just use a dedicated lock that does not involve memory
hotplug.


--
Thanks,

David / dhildenb

2022-08-25 10:21:57

by Kefeng Wang

Subject: Re: [PATCH 2/2] mm: slince possible data races about pgdat->kswapd

On 2022/8/25 16:22, David Hildenbrand wrote:
> On 25.08.22 04:34, Kefeng Wang wrote:
>> On 2022/8/24 16:24, David Hildenbrand wrote:
>>> On 24.08.22 09:19, Kefeng Wang wrote:
>>>> The pgdat->kswapd could be accessed concurrently by kswapd_run() and
>>>> kcompactd(), it don't be protected by any lock, which could leads to
>>>> data races, adding READ/WRITE_ONCE() to slince it.
>>> Okay, I think this patch here makes it clearer that we really just want
>>> proper synchronization instead of hacking around it.
>>>
>>> What speaks against protecting pgdat->kswapd this using some proper
>>> locking primitive?
>> as comments about kswapd in struct pglist_data,  pgdat->kswapd should be
>>
>> protected by mem_hotplug_begin/done(), how about this way?
>>
>> diff --git a/mm/compaction.c b/mm/compaction.c
>> index 640fa76228dd..62018f35242a 100644
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -1983,7 +1983,13 @@ static inline bool is_via_compact_memory(int order)
>>
>>  static bool kswapd_is_running(pg_data_t *pgdat)
>>  {
>> -       return pgdat->kswapd && task_is_running(pgdat->kswapd);
>> +       bool running;
>> +
>> +       mem_hotplug_begin();
>> +       running = pgdat->kswapd && task_is_running(pgdat->kswapd);
>> +       mem_hotplug_end();
>> +
>> +       return running;
>>  }
> I'd much rather just use a dedicated lock that does not involve memory
> hotplug.

The issue only occurred due memory hotplug, without mem-hotplug,

the kswapd won't stop or re-run, there is no above issue too, add a new

lock would be duplicated, but the scope of protection is smaller, I could

repost with new lock if no more comment.

>
>