linux-kernel - Re: [PATCH] aio: using hash table for active requests

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

Message-ID: <x49lj3qd79m.fsf@segfault.boston.devel.redhat.com>
Date:	Wed, 15 Dec 2010 17:02:29 -0500
From:	Jeff Moyer <jmoyer@...hat.com>
To:	Li Yu <raise.sail@...il.com>
Cc:	bcrl@...ck.org, viro@...iv.linux.org.uk, linux-aio@...ck.org,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] aio: using hash table for active requests

Li Yu <raise.sail@...il.com> writes:

> This patch remove a TODO in fs/aio.c, that is to use hash table for
> active requests.

It's remained a TODO item for this long because it hasn't really
mattered in the past.  Was there other motivation for this patch besides
the TODO comment?

> I prefer add an iocb at tail of collision chain, so I do not use hlist
> here.

Why do you prefer to add to the tail?  It makes more sense to add
collisions at the head, since if you're going to cancel everything, you
have a better chance of cancelling stuff that was queued later than
earlier (assuming you can cancel anything at all, which for most cases,
you can't).  Also, you're halving the number of buckets, so you should
have a good reason.

What sort of testing did you do?

I've made some cursory comments below.  I'll more fully review this if
you can provide a bit more justification and some proof of testing.  I'd
be really surprised if this helped any real workloads, though.

Cheers,
Jeff

> +static int ioctx_active_reqs_init(struct kioctx *ctx)
> +{
> +	int i;
> +
> +	ctx->active_reqs_table = kmalloc(AIO_ACTREQ_BUCKETS*sizeof(struct list_head), GFP_KERNEL);

Fit this into 80 columns, please.  You may just want to run checkpatch
over the whole thing.

> +static inline void aio_cancel_one(struct kioctx *ctx, struct kiocb *iocb)

I see no good reason to inline this.

> @@ -465,10 +506,12 @@ static struct kiocb *__aio_get_req(struct kioctx *ctx)
>  	/* Check if the completion queue has enough free space to
>  	 * accept an event from this io.
>  	 */
> +	bucket = hash_long((unsigned long)tohash, AIO_ACTREQ_BUCKETS_SHIFT);

hash_ptr?

> -	struct list_head	active_reqs;	/* used for cancellation */
> +	struct list_head*	active_reqs_table;	/* used for cancellation */

Coding Style
--
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/