Hi!
On Tue 22-10-24 12:33:27, Mohammed Anees wrote:
> > Benchmarks, please. Look at what operations are done on this list.
> > It's not at all obvious to me that what you've done here will improve
> > performance of any operation.
>
> This patch aims to improve this operation in io_cancel() syscall,
> currently this iterates through all the requests in the Linked list,
> checking for a match, which could take a significant time if the
> requests are high and once it finds one it deletes it. Using a hash
> table will significant reduce the search time, which is what the comment
> suggests as well.
>
> /* TODO: use a hash or array, this sucks. */
> list_for_each_entry(kiocb, &ctx->active_reqs, ki_list) {
> if (kiocb->ki_res.obj == obj) {
> ret = kiocb->ki_cancel(&kiocb->rw);
> list_del_init(&kiocb->ki_list);
> break;
> }
> }
>
> I have tested this patch and believe it doesn’t affect the
> other functions. As for the io_cancel() syscall, please let
> me know exactly how you’d like me to test it so I can benchmark
> it accordingly.
Well, I'd say that calling io_cancel() isn't really frequent operation. Or
are you aware of any workload that would be regularly doing that? Hence
optimizing performance for such operation isn't going to bring much benefit
to real users. On the other hand the additional complexity of handling
hashtable for requests in flight (although it isn't big on its own) is
going to impact everybody using AIO. Hence I agree with Matthew that
changes like you propose are not a clear win when looking at the bigger
picture and need good justification.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
