Re: [PATCH V3] KSM: allow dedup all tasks memory

From: Timofey Titovets
Date: Tue Nov 13 2018 - 12:11:30 EST


ÐÑ, 13 ÐÐÑÐ. 2018 Ð. Ð 19:33, Oleksandr Natalenko <oleksandr@xxxxxxxxxxxxxx>:
>
> So,
>
> > âsnipâ
> > +static int ksm_seeker_thread(void *nothing)
> > +{
> > + pid_t last_pid = 1;
> > + pid_t curr_pid;
> > + struct task_struct *task;
> > +
> > + set_freezable();
> > + set_user_nice(current, 5);
> > +
> > + while (!kthread_should_stop()) {
> > + wait_while_offlining();
> > +
> > + try_to_freeze();
> > +
> > + if (!ksm_mode_always()) {
> > + wait_event_freezable(ksm_seeker_thread_wait,
> > + ksm_mode_always() || kthread_should_stop());
> > + continue;
> > + }
> > +
> > + /*
> > + * import one task's vma per run
> > + */
> > + read_lock(&tasklist_lock);
> > +
> > + /* Try always get next task */
> > + for_each_process(task) {
> > + curr_pid = task_pid_nr(task);
> > + if (curr_pid == last_pid) {
> > + task = next_task(task);
> > + break;
> > + }
> > +
> > + if (curr_pid > last_pid)
> > + break;
> > + }
> > +
> > + last_pid = task_pid_nr(task);
> > + ksm_import_task_vma(task);
>
> This seems to be a bad idea. ksm_import_task_vma() may sleep with
> tasklist_lock being held. Thus, IIUC, you'll get this:

Yep, that one of the reason why i move code from ksmd thread, i'm not
fully understood how to properly fix that.
But i misunderstood problem symptoms.

> [ 1754.410322] BUG: scheduling while atomic: ksmd_seeker/50/0x00000002
> â
> [ 1754.410444] Call Trace:
> [ 1754.410455] dump_stack+0x5c/0x80
> [ 1754.410460] __schedule_bug.cold.19+0x38/0x51
> [ 1754.410464] __schedule+0x11dc/0x2080
> [ 1754.410483] schedule+0x32/0xb0
> [ 1754.410487] rwsem_down_write_failed+0x15d/0x240
> [ 1754.410496] call_rwsem_down_write_failed+0x13/0x20
> [ 1754.410499] down_write+0x20/0x30
> [ 1754.410502] ksm_import_task_vma+0x22/0x70
> [ 1754.410505] ksm_seeker_thread+0x134/0x1c0
> [ 1754.410512] kthread+0x113/0x130
> [ 1754.410518] ret_from_fork+0x35/0x40
>
> I think you may want to get a reference to task_struct before releasing
> tasklist_lock, and then put it after ksm_import_task_vma() does its job.

Maybe i misunderstood something, but currently i do exactly that.

> > + read_unlock(&tasklist_lock);
> > +
> > + schedule_timeout_interruptible(
> > + msecs_to_jiffies(ksm_thread_seeker_sleep_millisecs));
> > + }
> > + return 0;
> > +}
> > âsnipâ
>
> --
> Oleksandr Natalenko (post-factum)


That's good that you got that in any way (because i can't reproduce currently).

You mean try do something, like that right?

read_lock(&tasklist_lock);
<get reference to task>
task_lock(task);
read_unlock(&tasklist_lock);
last_pid = task_pid_nr(task);
ksm_import_task_vma(task);
task_unlock(task);

Thanks!