Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] ipc: replace costly bailout check in sysvipc_find_ipc()
To:     Rafael Aquini <aquini@redhat.com>, linux-kernel@vger.kernel.org
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Davidlohr Bueso <dbueso@suse.de>,
        Waiman Long <llong@redhat.com>
References: <20210809203554.1562989-1-aquini@redhat.com>
From:   Manfred Spraul <manfred@colorfullife.com>
Message-ID: <0b80af66-0ff4-5278-37e1-08966157493e@colorfullife.com>
Date:   Tue, 17 Aug 2021 20:34:02 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <20210809203554.1562989-1-aquini@redhat.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

Hello Rafael,

I still try to understand the code. It seems, it is more or less 
unchanged from 2009:

|

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/patch/ipc/util.c?id=7ca7e564e049d8b350ec9d958ff25eaa24226352 
|


On 8/9/21 10:35 PM, Rafael Aquini wrote:
> --- a/ipc/util.c
> +++ b/ipc/util.c
> @@ -788,21 +788,13 @@ struct pid_namespace *ipc_seq_pid_ns(struct seq_file *s)
>   static struct kern_ipc_perm *sysvipc_find_ipc(struct ipc_ids *ids, loff_t pos,
>   					      loff_t *new_pos)
>   {
> -	struct kern_ipc_perm *ipc;
> -	int total, id;
> -
> -	total = 0;
> -	for (id = 0; id < pos && total < ids->in_use; id++) {
> -		ipc = idr_find(&ids->ipcs_idr, id);
> -		if (ipc != NULL)
> -			total++;
> -	}
> +	struct kern_ipc_perm *ipc = NULL;
> +	int max_idx = ipc_get_maxidx(ids);
>   
> -	ipc = NULL;
> -	if (total >= ids->in_use)
> +	if (max_idx == -1 || pos > max_idx)
>   		goto out;
>   
> -	for (; pos < ipc_mni; pos++) {
> +	for (; pos <= max_idx; pos++) {
>   		ipc = idr_find(&ids->ipcs_idr, pos);
>   		if (ipc != NULL) {
>   			rcu_read_lock();

The change looks as correct to me. But I'm still not sure that I really 
understand what the current code does:

- first, loop over index values in the idr, starting from 0, count found 
entries.

- if we found all entries before we are at index=pos: fail

- then search up to ipc_nmi for the next entry with an index >=pos.

- if something is found: use it. otherwise fail.

It seems the code tries to avoid that we loop until ipc_mni after the 
last entry was found, and therefore we loop every time from 0.


 From what I see, the change looks to be correct: You now remove the 
first loop, and instead of searching until ipc_mni, the search ends at 
<= max_idx.

I'll try to find some time to test it.


But: What about using idr_get_next() instead of the idr_find()?

We want to find the next used index, thus idr_get_next() should be even 
better than the for loop, ...


--

     Manfred