linux-kernel - Re: [PATCH v2] epoll: add nsec timeout support

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: <20201116120445.7359b0053778c1a4492d1057@linux-foundation.org>
Date:   Mon, 16 Nov 2020 12:04:45 -0800
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        viro@...iv.linux.org.uk, soheil.kdev@...il.com, arnd@...db.de,
        shuochen@...gle.com, Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH v2] epoll: add nsec timeout support

On Mon, 16 Nov 2020 11:10:01 -0500 Willem de Bruijn <willemdebruijn.kernel@...il.com> wrote:

> From: Willem de Bruijn <willemb@...gle.com>
> 
> Add epoll_create1 flag EPOLL_NSTIMEO. When passed, this changes the
> interpretation of argument timeout in epoll_wait from msec to nsec.
> 
> Use cases such as datacenter networking operate on timescales well
> below milliseconds. Shorter timeouts bounds their tail latency.
> The underlying hrtimer is already programmed with nsec resolution.

hm, maybe.  It's not very nice to be using one syscall to alter the
interpretation of another syscall's argument in this fashion.  For
example, one wonders how strace(1) is to properly interpret & display
this argument?

Did you consider adding epoll_wait2()/epoll_pwait2() syscalls which
take a nsec timeout?  Seems simpler.

Either way, we'd be interested in seeing the proposed manpage updates
alongside this change.

> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -225,6 +225,9 @@ struct eventpoll {
>  	unsigned int napi_id;
>  #endif
>  
> +	/* Accept timeout in ns resolution (EPOLL_NSTIMEO) */
> +	unsigned int nstimeout:1;
> +

Why a bitfield?  This invites other developers to add new bitfields to
the same word.  And if that happens, we'll need to consider the locking
rules for that word - I don't think the compiler provides any
protection against concurrent modifications to the bitfields which
share a machine word.  If we add a rule

/*
 * per eventpoll flags.  Initialized at creation time, never changes
 * thereafter
 */

then that would cover it.  Or just make the thing a `bool'?