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 for Android: free password hash cracker in your pocket

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

Message-ID: <m1sg50f4ij.fsf@fess.ebiederm.org>
Date:   Fri, 12 Mar 2021 15:25:56 -0600
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Jim Newsome <jnewsome@...project.org>
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)

Jim Newsome <jnewsome@...project.org> writes:

> 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 :).

Oh absolutely.  The two lookups are independent.

> 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.

pid_task has a lot of verbiage but it is only reading a pointer,
verifying the pointer is not NULL and calling container_of on the result
of the pointer read.

Eric