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;
From:   Waiman Long <llong@redhat.com>
Subject: Re: [PATCH 2/2] ipc/util.c: Cleanup and improve sysvipc_find_ipc(),
 V2
To:     Manfred Spraul <manfred@colorfullife.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Davidlohr Bueso <dbueso@suse.de>,
        Waiman Long <llong@redhat.com>,
        Rafael Aquini <aquini@redhat.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>, 1vier1@web.de
References: <20210903052020.3265-1-manfred@colorfullife.com>
 <20210903052020.3265-3-manfred@colorfullife.com>
Message-ID: <cb984dbc-27ee-e0c5-1b2a-95d64f0f3598@redhat.com>
Date:   Fri, 3 Sep 2021 14:02:47 -0400
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: <20210903052020.3265-3-manfred@colorfullife.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk

On 9/3/21 1:20 AM, Manfred Spraul wrote:
> sysvipc_find_ipc() can be simplified further:
>
> - It uses a for() loop to locate the next entry in the idr.
>    This can be replaced with idr_get_next().
>
> - It receives two parameters (pos - which is actually
>    and idr index and not a position, and new_pos, which
Typo: "and" => "an"
>    is really a position).
>    One parameter is sufficient.
>
> Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
> ---
>   ipc/util.c | 53 ++++++++++++++++++++++++++++++++---------------------
>   1 file changed, 32 insertions(+), 21 deletions(-)
>
> diff --git a/ipc/util.c b/ipc/util.c
> index d48d8cfa1f3f..bc5000b2457a 100644
> --- a/ipc/util.c
> +++ b/ipc/util.c
> @@ -782,28 +782,37 @@ struct pid_namespace *ipc_seq_pid_ns(struct seq_file *s)
>   	return iter->pid_ns;
>   }
>   
> -/*
> - * This routine locks the ipc structure found at least at position pos.
> +/**
> + * sysvipc_find_ipc - Find and lock the ipc structure based on seq pos
> + * @ids: ipc identifier set
> + * @pos: expected position
> + *
> + * The function finds an ipc structure, based on the sequence file
> + * position @pos. If there is no ipc structure at position @pos, then
> + * the successor is selected.
> + * If a structure is found, then it is locked (both rcu_read_lock() and
> + * ipc_lock_object()) and  @pos is set to the position needed to locate
> + * the found ipc structure.
> + * If nothing is found (i.e. EOF), @pos is not modified.
> + *
> + * The function returns the found ipc structure, or NULL at EOF.
>    */
> -static struct kern_ipc_perm *sysvipc_find_ipc(struct ipc_ids *ids, loff_t pos,
> -					      loff_t *new_pos)
> +static struct kern_ipc_perm *sysvipc_find_ipc(struct ipc_ids *ids, loff_t *pos)
>   {
> -	struct kern_ipc_perm *ipc = NULL;
> -	int max_idx = ipc_get_maxidx(ids);
> +	int tmpidx;
> +	struct kern_ipc_perm *ipc;
>   
> -	if (max_idx == -1 || pos > max_idx)
> -		goto out;
> +	/* convert from position to idr index -> "-1" */
> +	tmpidx = *pos - 1;
>   
> -	for (; pos <= max_idx; pos++) {
> -		ipc = idr_find(&ids->ipcs_idr, pos);
> -		if (ipc != NULL) {
> -			rcu_read_lock();
> -			ipc_lock_object(ipc);
> -			break;
> -		}
> +	ipc = idr_get_next(&ids->ipcs_idr, &tmpidx);
> +	if (ipc != NULL) {
> +		rcu_read_lock();
> +		ipc_lock_object(ipc);
> +
> +		/* convert from idr index to position  -> "+1" */
> +		*pos = tmpidx + 1;
>   	}
> -out:
> -	*new_pos = pos + 1;
>   	return ipc;
>   }
>   
> @@ -817,11 +826,13 @@ static void *sysvipc_proc_next(struct seq_file *s, void *it, loff_t *pos)
>   	if (ipc && ipc != SEQ_START_TOKEN)
>   		ipc_unlock(ipc);
>   
> -	return sysvipc_find_ipc(&iter->ns->ids[iface->ids], *pos, pos);
> +	/* Next -> search for *pos+1 */
> +	(*pos)++;
> +	return sysvipc_find_ipc(&iter->ns->ids[iface->ids], pos);
>   }
>   
>   /*
> - * File positions: pos 0 -> header, pos n -> ipc id = n - 1.
> + * File positions: pos 0 -> header, pos n -> ipc idx = n - 1.
>    * SeqFile iterator: iterator value locked ipc pointer or SEQ_TOKEN_START.
>    */
>   static void *sysvipc_proc_start(struct seq_file *s, loff_t *pos)
> @@ -846,8 +857,8 @@ static void *sysvipc_proc_start(struct seq_file *s, loff_t *pos)
>   	if (*pos == 0)
>   		return SEQ_START_TOKEN;
>   
> -	/* Find the (pos-1)th ipc */
> -	return sysvipc_find_ipc(ids, *pos - 1, pos);
> +	/* Otherwise return the correct ipc structure */
> +	return sysvipc_find_ipc(ids, pos);
>   }
>   
>   static void sysvipc_proc_stop(struct seq_file *s, void *it)

Other than the typo, the patch looks good to me.

Acked-by: Waiman Long <longman@redhat.com>