linux-kernel - RE: [PATCH] iov_iter: optimise iter type checking

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: <6d5bd939aeaf425bbecf36a95c307f94@AcuMS.aculab.com>
Date:   Sun, 17 Jan 2021 12:12:47 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Al Viro' <viro@...iv.linux.org.uk>,
        Pavel Begunkov <asml.silence@...il.com>
CC:     "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "Jens Axboe" <axboe@...nel.dk>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] iov_iter: optimise iter type checking

From: Al Viro
> Sent: 16 January 2021 05:18
> 
> On Sat, Jan 09, 2021 at 10:11:09PM +0000, Pavel Begunkov wrote:
> 
> > > Does any code actually look at the fields as a pair?
> > > Would it even be better to use separate bytes?
> > > Even growing the on-stack structure by a word won't really matter.
> >
> > u8 type, rw;
> >
> > That won't bloat the struct. I like the idea. If used together compilers
> > can treat it as u16.
> 
> Reasonable, and from what I remember from looking through the users,
> no readers will bother with looking at both at the same time.

I couldn't find any.
...
> So... something like (completely untested) variant below, perhaps?
> 
> diff --git a/include/linux/uio.h b/include/linux/uio.h
> index 72d88566694e..ed8ad2c5d384 100644
> --- a/include/linux/uio.h
> +++ b/include/linux/uio.h
> @@ -19,20 +19,16 @@ struct kvec {
> 
>  enum iter_type {
>  	/* iter types */
> -	ITER_IOVEC = 4,
> -	ITER_KVEC = 8,
> -	ITER_BVEC = 16,
> -	ITER_PIPE = 32,
> -	ITER_DISCARD = 64,
> +	ITER_IOVEC,
> +	ITER_KVEC,
> +	ITER_BVEC,
> +	ITER_PIPE,
> +	ITER_DISCARD
>  };
> 
>  struct iov_iter {
> -	/*
> -	 * Bit 0 is the read/write bit, set if we're writing.
> -	 * Bit 1 is the BVEC_FLAG_NO_REF bit, set if type is a bvec and
> -	 * the caller isn't expecting to drop a page reference when done.
> -	 */
> -	unsigned int type;
> +	u8 iter_type;
> +	bool data_source;

I'd leave it as 'u8 direction' and assign READ (0) or WRITE (1) to it.
It will always be confusing whether WRITE means a 'write' system call
or a transfer that will write into the buffer (eg a read system call).

I'm pretty sure I can detect the performance change from forcing
the compiler to convert values to 'bool'.

...

Since you've still got tests like:

> +	if (i->iter_type == ITER_BVEC || i->iter_type == ITER_KVEC) {

It is probably still worth using bit values.
After all, the only thing that benefits from small dense integers
is a case statement lookup table - and we don't do those any more.

Otherwise you might as well use 'i', 'k', 'b', 'p' and 'd' so that
anyone hexdumping the structure (or reading the asm decode) knows
the type without having to go and look it up.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)