linux-kernel - Re: [PATCH v5] do_wait: make PIDTYPE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 12 Mar 2021 15:05:16 -0600
From:   Jim Newsome <jnewsome@...project.org>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Oleg Nesterov <oleg@...hat.com>,
        Christian Brauner <christian@...uner.io>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5] do_wait: make PIDTYPE_PID case O(1) instead of O(n)

On 3/12/21 14:29, Eric W. Biederman wrote:
> When I looked at this a second time it became apparent that using
> pid_task twice should actually be faster as it removes a dependent load
> caused by thread_group_leader, and replaces it by accessing two adjacent
> pointers in the same cache line.
> 
> I know the algorithmic improvement is the main advantage, but removing
> 60ns or so for a dependent load can't hurt.
> 
> Plus I think using the two pid types really makes it clear that one
> is always a process and the other is always potentially a thread.
> 
> /*
>  * Optimization for waiting on PIDTYPE_PID. No need to iterate through child
>  * and tracee lists to find the target task.
>  */
> static int do_wait_pid(struct wait_opts *wo)
> {
> 	bool ptrace;
> 	struct task_struct *target;
> 	int retval;
> 
> 	ptrace = false;
> 	target = pid_task(wo->wo_pid, PIDTYPE_TGID);
> 	if (target && is_effectively_child(wo, ptrace, target)) {
> 		retval = wait_consider_task(wo, ptrace, target);
> 		if (retval)
> 			return retval;
> 	}
> 
> 	ptrace = true;
> 	target = pid_task(wo->wo_pid, PIDTYPE_PID);
> 	if (target && target->ptrace &&
>             is_effectively_child(wo, ptrace, target)) {
> 		retval = wait_consider_task(wo, ptrace, target);
> 		if (retval)
> 			return retval;
> 	}
> 
> 	return 0;
> }

I'm fine with either way.

Part of what made my earlier version with the double-lookup a bit
awkward was only doing the second lookup if the first lookup failed. I'm
happy to take your word though that making the second lookup conditional
is unnecessary or even detrimental :). It did cross my mind that it
might not be a very consistent branch for a branch-predictor, but I also
figured pid_task's synchronization might outweigh that.