lkml.org 
[lkml]   [2021]   [Aug]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] ipc: replace costly bailout check in sysvipc_find_ipc()
From
Date
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

\
 
 \ /
  Last update: 2021-08-17 20:35    [W:0.147 / U:0.084 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site