Re: [PATCH 1/2] introduce for_each_process_thread_break() and for_each_process_thread_continue()

[Andrew Morton]

On Wed, 12 Sep 2018 18:33:35 +0200 Oleg Nesterov <oleg@xxxxxxxxxx> wrote:

> Usage:
> 
> 	rcu_read_lock();
> 	for_each_process_thread(p, t) {
> 		do_something_slow(p, t);
> 
> 		if (SPENT_TOO_MUCH_TIME) {
> 			for_each_process_thread_break(p, t);
> 			rcu_read_unlock();
> 			schedule();
> 			rcu_read_lock();
> 			for_each_process_thread_continue(&p, &t);
> 		}
> 	}
> 	rcu_read_unlock();
> 
> This looks similar to rcu_lock_break(), but much better and the next patch
> changes check_hung_uninterruptible_tasks() to use these new helpers. But my
> real target is show_state_filter() which can trivially lead to lockup.
> 
> Compared to rcu_lock_break(), for_each_process_thread_continue() never gives
> up, it relies on fact that both process and thread lists are sorted by the
> task->start_time key. So, for example, even if both leader/thread are already
> dead we can find the next alive process and continue.
> 
> Strictly speaking, the for_each_process/for_each_thread loops in _continue()
> could be "SPEND_TOO_MUCH_TIME" by themselves, so perhaps we will add another
> "max_scan" argument later or do something else. But at least they can not
> livelock under heavy fork/exit loads, they are bounded by PID_MAX_DEFAULT in
> the worst case.
> 
> NOTE: it seems that, contrary to the comment, task_struct->start_time is not
> really monotonic, and this should be probably fixed. Until then  _continue()
> might skip more threads with the same ->start_time than necessary.
> 
> --- a/include/linux/sched/signal.h
> +++ b/include/linux/sched/signal.h
> @@ -565,6 +565,16 @@ extern bool current_is_single_threaded(void);
>  #define for_each_process_thread(p, t)	\
>  	for_each_process(p) for_each_thread(p, t)
>  
> +static inline void
> +for_each_process_thread_break(struct task_struct *p, struct task_struct *t)
> +{
> +	get_task_struct(p);
> +	get_task_struct(t);
> +}
> +
> +extern void
> +for_each_process_thread_continue(struct task_struct **, struct task_struct **);

These things will need some documentation, please.  What they do, why
they do it, how people should use them, when and why they should use
them.  Etcetera!  This is tricky stuff.


> +void for_each_process_thread_continue(struct task_struct **p_leader,
> +				      struct task_struct **p_thread)
> +{
> +	struct task_struct *leader = *p_leader, *thread = *p_thread;
> +	struct task_struct *prev, *next;
> +	u64 start_time;
> +
> +	if (pid_alive(thread)) {
> +		/* mt exec could change the leader */
> +		*p_leader = thread->group_leader;
> +	} else if (pid_alive(leader)) {
> +		start_time = thread->start_time;
> +		prev = leader;
> +
> +		for_each_thread(leader, next) {
> +			if (next->start_time > start_time)
> +				break;
> +			prev = next;
> +		}
> +
> +		*p_thread = prev;
> +	} else {
> +		start_time = leader->start_time;
> +		prev = &init_task;
> +
> +		for_each_process(next) {
> +			if (next->start_time > start_time)
> +				break;
> +			prev = next;
> +		}
> +
> +		*p_leader = prev;
> +		/* a new thread can come after that, but this is fine */
> +		*p_thread = list_last_entry(&prev->signal->thread_head,
> +						struct task_struct,
> +						thread_node);
> +	}
> +
> +	put_task_struct(leader);
> +	put_task_struct(thread);
> +}

Should these be available to modules, like the rest of these things
appear to be?  Or we could do that later if a need is shown.