All of lore.kernel.org
 help / color / mirror / Atom feed
From: Oleg Nesterov <oleg@redhat.com>
To: Sultan Alsawaf <sultan@kerneltoast.com>
Cc: "Christian Brauner" <christian@brauner.io>,
	"Daniel Colascione" <dancol@google.com>,
	"Suren Baghdasaryan" <surenb@google.com>,
	"Steven Rostedt" <rostedt@goodmis.org>,
	"Tim Murray" <timmurray@google.com>,
	"Michal Hocko" <mhocko@kernel.org>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Arve Hjønnevåg" <arve@android.com>,
	"Todd Kjos" <tkjos@android.com>,
	"Martijn Coenen" <maco@android.com>,
	"Ingo Molnar" <mingo@redhat.com>,
	"Peter Zijlstra" <peterz@infradead.org>,
	LKML <linux-kernel@vger.kernel.org>,
	"open list:ANDROID DRIVERS" <devel@driverdev.osuosl.org>,
	linux-mm <linux-mm@kvack.org>,
	kernel-team <kernel-team@android.com>,
	"Andy Lutomirski" <luto@amacapital.net>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	"Kees Cook" <keescook@chromium.org>,
	"Joel Fernandes" <joel@joelfernandes.org>
Subject: Re: [RFC] simple_lmk: Introduce Simple Low Memory Killer for Android
Date: Tue, 7 May 2019 17:31:54 +0200	[thread overview]
Message-ID: <20190507153154.GA5750@redhat.com> (raw)
In-Reply-To: <20190507021622.GA27300@sultan-box.localdomain>

I am not going to comment the intent, but to be honest I am skeptical too.

On 05/06, Sultan Alsawaf wrote:
>
> +static unsigned long find_victims(struct victim_info *varr, int *vindex,
> +				  int vmaxlen, int min_adj, int max_adj)
> +{
> +	unsigned long pages_found = 0;
> +	int old_vindex = *vindex;
> +	struct task_struct *tsk;
> +
> +	for_each_process(tsk) {
> +		struct task_struct *vtsk;
> +		unsigned long tasksize;
> +		short oom_score_adj;
> +
> +		/* Make sure there's space left in the victim array */
> +		if (*vindex == vmaxlen)
> +			break;
> +
> +		/* Don't kill current, kthreads, init, or duplicates */
> +		if (same_thread_group(tsk, current) ||
> +		    tsk->flags & PF_KTHREAD ||
> +		    is_global_init(tsk) ||
> +		    vtsk_is_duplicate(varr, *vindex, tsk))
> +			continue;
> +
> +		vtsk = find_lock_task_mm(tsk);

Did you test this patch with lockdep enabled?

If I read the patch correctly, lockdep should complain. vtsk_is_duplicate()
ensures that we do not take the same ->alloc_lock twice or more, but lockdep
can't know this.

> +static void scan_and_kill(unsigned long pages_needed)
> +{
> +	static DECLARE_WAIT_QUEUE_HEAD(victim_waitq);
> +	struct victim_info victims[MAX_VICTIMS];
> +	int i, nr_to_kill = 0, nr_victims = 0;
> +	unsigned long pages_found = 0;
> +	atomic_t victim_count;
> +
> +	/*
> +	 * Hold the tasklist lock so tasks don't disappear while scanning. This
> +	 * is preferred to holding an RCU read lock so that the list of tasks
> +	 * is guaranteed to be up to date. Keep preemption disabled until the
> +	 * SIGKILLs are sent so the victim kill process isn't interrupted.
> +	 */
> +	read_lock(&tasklist_lock);
> +	preempt_disable();

read_lock() disables preemption, every task_lock() too, so this looks
unnecessary.

> +	for (i = 1; i < ARRAY_SIZE(adj_prio); i++) {
> +		pages_found += find_victims(victims, &nr_victims, MAX_VICTIMS,
> +					    adj_prio[i], adj_prio[i - 1]);
> +		if (pages_found >= pages_needed || nr_victims == MAX_VICTIMS)
> +			break;
> +	}
> +
> +	/*
> +	 * Calculate the number of tasks that need to be killed and quickly
> +	 * release the references to those that'll live.
> +	 */
> +	for (i = 0, pages_found = 0; i < nr_victims; i++) {
> +		struct victim_info *victim = &victims[i];
> +		struct task_struct *vtsk = victim->tsk;
> +
> +		/* The victims' mm lock is taken in find_victims; release it */
> +		if (pages_found >= pages_needed) {
> +			task_unlock(vtsk);
> +			continue;
> +		}
> +
> +		/*
> +		 * Grab a reference to the victim so it doesn't disappear after
> +		 * the tasklist lock is released.
> +		 */
> +		get_task_struct(vtsk);

The comment doesn't look correct. the victim can't dissapear until task_unlock()
below, it can't pass exit_mm().

> +		pages_found += victim->size;
> +		nr_to_kill++;
> +	}
> +	read_unlock(&tasklist_lock);
> +
> +	/* Kill the victims */
> +	victim_count = (atomic_t)ATOMIC_INIT(nr_to_kill);
> +	for (i = 0; i < nr_to_kill; i++) {
> +		struct victim_info *victim = &victims[i];
> +		struct task_struct *vtsk = victim->tsk;
> +
> +		pr_info("Killing %s with adj %d to free %lu kiB\n", vtsk->comm,
> +			vtsk->signal->oom_score_adj,
> +			victim->size << (PAGE_SHIFT - 10));
> +
> +		/* Configure the victim's mm to notify us when it's freed */
> +		vtsk->mm->slmk_waitq = &victim_waitq;
> +		vtsk->mm->slmk_counter = &victim_count;
> +
> +		/* Accelerate the victim's death by forcing the kill signal */
> +		do_send_sig_info(SIGKILL, SIG_INFO_TYPE, vtsk, true);
                                                               ^^^^
this should be PIDTYPE_TGID

> +
> +		/* Finally release the victim's mm lock */
> +		task_unlock(vtsk);
> +	}
> +	preempt_enable_no_resched();

See above. And I don't understand how can _no_resched() really help...

> +
> +	/* Try to speed up the death process now that we can schedule again */
> +	for (i = 0; i < nr_to_kill; i++) {
> +		struct task_struct *vtsk = victims[i].tsk;
> +
> +		/* Increase the victim's priority to make it die faster */
> +		set_user_nice(vtsk, MIN_NICE);
> +
> +		/* Allow the victim to run on any CPU */
> +		set_cpus_allowed_ptr(vtsk, cpu_all_mask);
> +
> +		/* Finally release the victim reference acquired earlier */
> +		put_task_struct(vtsk);
> +	}
> +
> +	/* Wait until all the victims die */
> +	wait_event(victim_waitq, !atomic_read(&victim_count));

Can't we avoid the new slmk_waitq/slmk_counter members in mm_struct?

I mean, can't we export victim_waitq and victim_count and, say, set/test
MMF_OOM_VICTIM. In fact I think you should try to re-use mark_oom_victim()
at least.

Oleg.


  parent reply	other threads:[~2019-05-07 15:32 UTC|newest]

Thread overview: 125+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-10 20:34 [RFC] simple_lmk: Introduce Simple Low Memory Killer for Android Sultan Alsawaf
2019-03-10 20:34 ` Sultan Alsawaf
2019-03-10 21:03 ` Greg Kroah-Hartman
2019-03-10 21:26   ` Sultan Alsawaf
2019-03-11 16:32 ` Joel Fernandes
2019-03-11 16:37   ` Joel Fernandes
2019-03-11 17:43 ` Michal Hocko
2019-03-11 17:58   ` Sultan Alsawaf
2019-03-11 20:10     ` Suren Baghdasaryan
2019-03-11 20:46       ` Sultan Alsawaf
2019-03-11 21:11         ` Joel Fernandes
2019-03-11 21:46           ` Sultan Alsawaf
2019-03-11 22:15         ` Suren Baghdasaryan
2019-03-11 22:36           ` Sultan Alsawaf
2019-03-12  8:05           ` Michal Hocko
2019-03-12 14:36             ` Suren Baghdasaryan
2019-03-12 15:25             ` Matthew Wilcox
2019-03-12 15:33               ` Michal Hocko
2019-03-12 15:39                 ` Michal Hocko
2019-03-12 16:37             ` Sultan Alsawaf
2019-03-12 16:48               ` Michal Hocko
2019-03-12 16:58               ` Michal Hocko
2019-03-12 17:15                 ` Suren Baghdasaryan
2019-03-12 17:17               ` Tim Murray
2019-03-12 17:45                 ` Sultan Alsawaf
2019-03-12 18:43                   ` Tim Murray
2019-03-12 18:50                     ` Christian Brauner
2019-03-14 17:47                 ` Joel Fernandes
2019-03-14 20:49                   ` Sultan Alsawaf
2019-03-15  2:54                     ` Joel Fernandes
2019-03-15  3:43                       ` Sultan Alsawaf
2019-03-15  3:16                     ` Steven Rostedt
2019-03-15  3:45                       ` Sultan Alsawaf
2019-03-15  4:36                       ` Daniel Colascione
2019-03-15 13:36                         ` Joel Fernandes
2019-03-15 15:56                         ` Suren Baghdasaryan
2019-03-15 16:12                           ` Daniel Colascione
2019-03-15 16:43                         ` Steven Rostedt
2019-03-15 17:17                           ` Daniel Colascione
2019-03-15 18:03                         ` Christian Brauner
2019-03-15 18:13                           ` Joel Fernandes
2019-03-15 18:24                             ` Christian Brauner
2019-03-15 18:49                               ` Joel Fernandes
2019-03-16 17:31                                 ` Suren Baghdasaryan
2019-03-16 18:00                                   ` Daniel Colascione
2019-03-16 18:57                                     ` Christian Brauner
2019-03-16 19:37                                       ` Suren Baghdasaryan
2019-03-17  1:53                                         ` Joel Fernandes
2019-03-17 11:42                                           ` Christian Brauner
2019-03-17 15:40                                             ` Daniel Colascione
2019-03-18  0:29                                               ` Christian Brauner
2019-03-18 23:50                                                 ` Joel Fernandes
2019-03-19 22:14                                                   ` Christian Brauner
2019-03-19 22:26                                                     ` Joel Fernandes
2019-03-19 22:48                                                     ` Daniel Colascione
2019-03-19 23:10                                                       ` Christian Brauner
2019-03-20  1:52                                                         ` Joel Fernandes
2019-03-20  2:42                                                           ` pidfd design Daniel Colascione
2019-03-20  3:59                                                             ` Christian Brauner
2019-03-20  7:02                                                               ` Daniel Colascione
2019-03-20 11:33                                                                 ` Joel Fernandes
2019-03-20 11:33                                                                   ` Joel Fernandes
2019-03-20 18:26                                                                   ` Christian Brauner
2019-03-20 18:38                                                                     ` Daniel Colascione
2019-03-20 18:51                                                                       ` Christian Brauner
2019-03-20 18:58                                                                         ` Andy Lutomirski
2019-03-20 19:14                                                                           ` Christian Brauner
2019-03-20 19:40                                                                             ` Daniel Colascione
2019-03-21 17:02                                                                               ` Andy Lutomirski
2019-03-25 20:13                                                                                 ` Jann Horn
2019-03-25 20:13                                                                                   ` Jann Horn
2019-03-25 20:23                                                                                   ` Daniel Colascione
2019-03-25 20:23                                                                                     ` Daniel Colascione
2019-03-25 23:42                                                                                     ` Andy Lutomirski
2019-03-25 23:42                                                                                       ` Andy Lutomirski
2019-03-25 23:45                                                                                       ` Christian Brauner
2019-03-25 23:45                                                                                         ` Christian Brauner
2019-03-26  0:00                                                                                         ` Andy Lutomirski
2019-03-26  0:00                                                                                           ` Andy Lutomirski
2019-03-26  0:12                                                                                           ` Christian Brauner
2019-03-26  0:12                                                                                             ` Christian Brauner
2019-03-26  0:24                                                                                             ` Andy Lutomirski
2019-03-26  0:24                                                                                               ` Andy Lutomirski
2019-03-28  9:21                                                                                               ` Christian Brauner
2019-03-28  9:21                                                                                                 ` Christian Brauner
2019-03-20 19:19                                                                         ` Joel Fernandes
2019-03-20 19:29                                                                         ` Daniel Colascione
2019-03-24 14:44                                                                           ` Serge E. Hallyn
2019-03-24 18:48                                                                             ` Joel Fernandes
2019-03-20 19:11                                                                     ` Joel Fernandes
2019-05-07  2:16                                                           ` [RFC] simple_lmk: Introduce Simple Low Memory Killer for Android Sultan Alsawaf
2019-05-07  2:16                                                             ` Sultan Alsawaf
2019-05-07  7:04                                                             ` Greg Kroah-Hartman
2019-05-07  7:27                                                               ` Sultan Alsawaf
2019-05-07  7:43                                                                 ` Greg Kroah-Hartman
2019-05-07  8:12                                                                   ` Sultan Alsawaf
2019-05-07 10:58                                                                     ` Christian Brauner
2019-05-07 16:28                                                                       ` Suren Baghdasaryan
2019-05-07 16:38                                                                         ` Christian Brauner
2019-05-07 16:53                                                                         ` Sultan Alsawaf
2019-05-07 20:01                                                                           ` Suren Baghdasaryan
2019-05-07 18:46                                                                         ` Joel Fernandes
2019-05-07 17:17                                                                       ` Sultan Alsawaf
2019-05-07 17:29                                                                         ` Greg Kroah-Hartman
2019-05-07 11:09                                                                     ` Greg Kroah-Hartman
2019-05-07 12:26                                                             ` Michal Hocko
2019-05-07 15:31                                                             ` Oleg Nesterov [this message]
2019-05-07 16:35                                                               ` Sultan Alsawaf
2019-05-09 15:56                                                                 ` Oleg Nesterov
2019-05-09 18:33                                                                   ` Sultan Alsawaf
2019-05-10 15:10                                                                     ` Oleg Nesterov
2019-05-13 16:45                                                                       ` Sultan Alsawaf
2019-05-14 16:44                                                                         ` Steven Rostedt
2019-05-14 17:31                                                                           ` Sultan Alsawaf
2019-05-15 14:58                                                                         ` Oleg Nesterov
2019-05-15 17:27                                                                           ` Sultan Alsawaf
2019-05-15 17:27                                                                             ` Sultan Alsawaf
2019-05-15 18:32                                                                             ` Steven Rostedt
2019-05-15 18:52                                                                               ` Sultan Alsawaf
2019-05-15 20:09                                                                                 ` Steven Rostedt
2019-05-16 13:54                                                                             ` Oleg Nesterov
2019-03-17 16:35                                             ` Serge E. Hallyn
2019-03-17 17:11                                               ` Daniel Colascione
2019-03-17 17:16                                                 ` Serge E. Hallyn
2019-03-17 22:02                                                   ` Suren Baghdasaryan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190507153154.GA5750@redhat.com \
    --to=oleg@redhat.com \
    --cc=arve@android.com \
    --cc=christian@brauner.io \
    --cc=dancol@google.com \
    --cc=devel@driverdev.osuosl.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=joel@joelfernandes.org \
    --cc=keescook@chromium.org \
    --cc=kernel-team@android.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=luto@amacapital.net \
    --cc=maco@android.com \
    --cc=mhocko@kernel.org \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=serge@hallyn.com \
    --cc=sultan@kerneltoast.com \
    --cc=surenb@google.com \
    --cc=timmurray@google.com \
    --cc=tkjos@android.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.