linux-kernel - Re: [PATCH] aio: convert the ioctx list to radix tree

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] [day] [month] [year] [list]

Message-ID: <CAE1zotLg1RK74LJ3R6HjzaKu=y+g+VZTOZqyxCUFxJpEuvL-xQ@mail.gmail.com>
Date:	Fri, 22 Mar 2013 17:21:47 -0700
From:	Octavian Purdila <octavian.purdila@...el.com>
To:	Zach Brown <zab@...hat.com>
Cc:	linux-kernel@...r.kernel.org, linux-aio@...ck.org,
	linux-s390@...r.kernel.org, koverstreet@...gle.com, bcrl@...ck.org,
	schwidefsky@...ibm.com, kirill.shutemov@...ux.intel.com,
	Andi Kleen <ak@...ux.intel.com>
Subject: Re: [PATCH] aio: convert the ioctx list to radix tree

On Fri, Mar 22, 2013 at 3:05 PM, Zach Brown <zab@...hat.com> wrote:
> On Fri, Mar 22, 2013 at 08:33:19PM +0200, Octavian Purdila wrote:
>> When using a large number of threads performing AIO operations the
>> IOCTX list may get a significant number of entries which will cause
>> significant overhead. For example, when running this fio script:
>
> Indeed.  But you also need to consider the impact this change has on the
> typical case of only having one ctx in the mm.  Please include
> measurements of that case in the commit message.
>

Hi Zach,

Good point, I will add those numbers.

>> --- a/arch/s390/mm/pgtable.c
>> +++ b/arch/s390/mm/pgtable.c
>> @@ -831,7 +831,7 @@ int s390_enable_sie(void)
>>       task_lock(tsk);
>>       if (!tsk->mm || atomic_read(&tsk->mm->mm_users) > 1 ||
>>  #ifdef CONFIG_AIO
>> -         !hlist_empty(&tsk->mm->ioctx_list) ||
>> +         tsk->mm->ioctx_rtree.rnode ||
>>  #endif
>
> Boy, what a curious thing.  I wonder if this is still needed if we're no
> longer storing the mm in the ctx after retry support is removed.
>
>> +     err = radix_tree_insert(&mm->ioctx_rtree, ctx->user_id, ctx);
>
> Hmm.  Is there anything stopping an exceptionally jerky app from racing
> io_setup() and munmap() and having two contexts be mapped to the same
> address and get the same user_id?  I guess this would just return
> -EEXIST, then, not do anything terrible.  I guess that's OK?
>

Ah, interesting, didn't thought of that. But I guess it is OK to keep
the WARN_ONCE there, as the application is not supposed to do the
munmap.

>> +                                            idx, sizeof(ctx)/sizeof(void *));
>
> ARRAY_SIZE(ctx)
>
> And why bother tracking the starting idx?  If you're completely draining
> it simply always start from 0?
>

I will fix these in the next version, thanks for reviewing !
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/