Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1396902 > unrolled thread

Re: [PATCH v5] mm: Add memory allocation watchdog kernel thread.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5] mm: Add memory allocation watchdog kernel thread. "Hillf Danton" <hillf.zj@alibaba-inc.com> - 2016-05-09 12:00 +0200
    Re: [PATCH v5] mm: Add memory allocation watchdog kernel thread. Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-05-12 17:10 +0200
      Re: [PATCH v5] mm: Add memory allocation watchdog kernel thread. Michal Hocko <mhocko@kernel.org> - 2016-05-12 18:40 +0200

#1396902 — Re: [PATCH v5] mm: Add memory allocation watchdog kernel thread.

[...]
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1446,6 +1446,32 @@ struct tlbflush_unmap_batch {
>  	bool writable;
>  };
> 
> +struct memalloc_info {
> +	/*
> +	 * 0: not doing __GFP_RECLAIM allocation.
> +	 * 1: doing non-recursive __GFP_RECLAIM allocation.
> +	 * 2: doing recursive __GFP_RECLAIM allocation.
> +	 */
> +	u8 valid;
> +	/*
> +	 * bit 0: Will be reported as OOM victim.
> +	 * bit 1: Will be reported as dying task.
> +	 * bit 2: Will be reported as stalling task.
> +	 * bit 3: Will be reported as exiting task.
> +	 * bit 7: Will be reported unconditionally.
> +	 */
> +	u8 type;
> +	/* Index used for memalloc_in_flight[] counter. */
> +	u8 idx;

	u8 __pad;	is also needed perhaps.

> +	/* For progress monitoring. */
> +	unsigned int sequence;
> +	/* Started time in jiffies as of valid == 1. */
> +	unsigned long start;
> +	/* Requested order and gfp flags as of valid == 1. */
> +	unsigned int order;
> +	gfp_t gfp;
> +};
> +

[...]
> +
> +/*
> + * check_memalloc_stalling_tasks - Check for memory allocation stalls.
> + *
> + * @timeout: Timeout in jiffies.
> + *
> + * Returns number of stalling tasks.
> + *
> + * This function is marked as "noinline" in order to allow inserting dynamic
> + * probes (e.g. printing more information as needed using SystemTap, calling
> + * panic() if this function returned non 0 value).
> + */
> +static noinline int check_memalloc_stalling_tasks(unsigned long timeout)
> +{
> +	char buf[256];
> +	struct task_struct *g, *p;
> +	unsigned long now;
> +	unsigned long expire;
> +	unsigned int sigkill_pending = 0;
> +	unsigned int exiting_tasks = 0;
> +	unsigned int memdie_pending = 0;
> +	unsigned int stalling_tasks = 0;
> +
> +	cond_resched();
> +	now = jiffies;
> +	/*
> +	 * Report tasks that stalled for more than half of timeout duration
> +	 * because such tasks might be correlated with tasks that already
> +	 * stalled for full timeout duration.
> +	 */
> +	expire = now - timeout * (HZ / 2);
> +	/* Count stalling tasks, dying and victim tasks. */
> +	preempt_disable();
> +	rcu_read_lock();
> +	for_each_process_thread(g, p) {
> +		u8 type = 0;
> +
> +		if (test_tsk_thread_flag(p, TIF_MEMDIE)) {
> +			type |= 1;
> +			memdie_pending++;
> +		}
> +		if (fatal_signal_pending(p)) {
> +			type |= 2;
> +			sigkill_pending++;
> +		}
> +		if ((p->flags & PF_EXITING) && p->state != TASK_DEAD) {
> +			type |= 8;
> +			exiting_tasks++;
> +		}
> +		if (is_stalling_task(p, expire)) {
> +			type |= 4;
> +			stalling_tasks++;
> +		}
> +		if (p->flags & PF_KSWAPD)
> +			type |= 128;
> +		p->memalloc.type = type;
> +	}

The numbers assigned to type may be replaced with texts, 
for instance,
	MEMALLOC_TYPE_VICTIM 
	MEMALLOC_TYPE_DYING 
	MEMALLOC_TYPE_STALLING
	MEMALLOC_TYPE_EXITING
	MEMALLOC_TYPE_REPORT

> +	rcu_read_unlock();
> +	preempt_enable();
> +	if (!stalling_tasks)
> +		return 0;
> +	cond_resched();
> +	/* Report stalling tasks, dying and victim tasks. */
> +	pr_warn("MemAlloc-Info: stalling=%u dying=%u exiting=%u victim=%u oom_count=%u\n",
> +		stalling_tasks, sigkill_pending, exiting_tasks, memdie_pending,
> +		out_of_memory_count);
> +	cond_resched();
> +	sigkill_pending = 0;
> +	exiting_tasks = 0;
> +	memdie_pending = 0;
> +	stalling_tasks = 0;
> +	preempt_disable();
> +	rcu_read_lock();
> + restart_report:
> +	for_each_process_thread(g, p) {
> +		bool can_cont;
> +		u8 type;
> +
> +		if (likely(!p->memalloc.type))
> +			continue;
> +		p->memalloc.type = 0;
> +		/* Recheck in case state changed meanwhile. */
> +		type = 0;
> +		if (test_tsk_thread_flag(p, TIF_MEMDIE)) {
> +			type |= 1;
> +			memdie_pending++;
> +		}
> +		if (fatal_signal_pending(p)) {
> +			type |= 2;
> +			sigkill_pending++;
> +		}
> +		if ((p->flags & PF_EXITING) && p->state != TASK_DEAD) {
> +			type |= 8;
> +			exiting_tasks++;
> +		}
> +		if (is_stalling_task(p, expire)) {
> +			type |= 4;
> +			stalling_tasks++;
> +			snprintf(buf, sizeof(buf),
> +				 " seq=%u gfp=0x%x(%pGg) order=%u delay=%lu",
> +				 memalloc.sequence, memalloc.gfp,
> +				 &memalloc.gfp,
> +				 memalloc.order, now - memalloc.start);
> +		} else {
> +			buf[0] = '\0';
> +		}
> +		if (p->flags & PF_KSWAPD)
> +			type |= 128;

ditto

thanks
Hillf

[toc] | [next] | [standalone]

#1400131

Hillf Danton wrote:
> > +struct memalloc_info {
> > +	/*
> > +	 * 0: not doing __GFP_RECLAIM allocation.
> > +	 * 1: doing non-recursive __GFP_RECLAIM allocation.
> > +	 * 2: doing recursive __GFP_RECLAIM allocation.
> > +	 */
> > +	u8 valid;
> > +	/*
> > +	 * bit 0: Will be reported as OOM victim.
> > +	 * bit 1: Will be reported as dying task.
> > +	 * bit 2: Will be reported as stalling task.
> > +	 * bit 3: Will be reported as exiting task.
> > +	 * bit 7: Will be reported unconditionally.
> > +	 */
> > +	u8 type;
> > +	/* Index used for memalloc_in_flight[] counter. */
> > +	u8 idx;
> 
> 	u8 __pad;	is also needed perhaps.
> 

Since this structure is not marked as __packed, I think that
the compiler will automatically pad it.

> The numbers assigned to type may be replaced with texts, 
> for instance,
> 	MEMALLOC_TYPE_VICTIM 
> 	MEMALLOC_TYPE_DYING 
> 	MEMALLOC_TYPE_STALLING
> 	MEMALLOC_TYPE_EXITING
> 	MEMALLOC_TYPE_REPORT
> 

I can define them as bit shift numbers. Thanks.



Michal, this version eliminated overhead of walking the process list
when nothing is wrong. You are aware of the possibility of
debug_show_all_locks() failing to report the culprit, aren't you?
So, what are unacceptable major problems for you?

[toc] | [prev] | [next] | [standalone]

#1400228

On Fri 13-05-16 00:09:07, Tetsuo Handa wrote:
[...]
> Michal, this version eliminated overhead of walking the process list
> when nothing is wrong. You are aware of the possibility of
> debug_show_all_locks() failing to report the culprit, aren't you?
> So, what are unacceptable major problems for you?

I do not remember complaining about anything unacceptable for this
patch. I just thought it was way too large the last time I have looked
at it. Johannes was suggesting to simply extend warn_alloc_failed to
also report too many retries with some extended information as a more
lightweight approach.  It wouldn't give as much information as your
watchdog but maybe it would be sufficient to figure out that something
really bad is going on. Dunno but I would rather see a more lightweight
debugging aid than a lot of code which sit basically unused most of the
time.

That being said I cannot comment on this particular version as I haven't
checked it properly yet.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web