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

[Timofey Titovets <timofey.titovets@synesis.ru>]

вт, 13 нояб. 2018 г. в 19:33, Oleksandr Natalenko <oleksandr@natalenko.name>:
>
> 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!