Re: [PATCH V2] eventfd: change int to __u64 in eventfd_signal()

[Andrew Morton <akpm@linux-foundation.org>]

On Tue, 17 Apr 2012 11:44:36 +0800
Sha Zhengju <handai.szj@gmail.com> wrote:

> From: Sha Zhengju <handai.szj@taobao.com>
> 
> eventfd_ctx->count is an __u64 counter which is allowed to reach ULLONG_MAX.
> Now eventfd_write() add an __u64 value to "count", but kernel side 
> eventfd_signal() only add an int value to it. So make them consistent.  
> 
> ...
>
> --- a/fs/eventfd.c
> +++ b/fs/eventfd.c
> @@ -51,15 +51,13 @@ struct eventfd_ctx {
>   *
>   * -EINVAL    : The value of @n is negative.
>   */
> -int eventfd_signal(struct eventfd_ctx *ctx, int n)
> +__u64 eventfd_signal(struct eventfd_ctx *ctx, __u64 n)
>  {
>  	unsigned long flags;
>  
> -	if (n < 0)
> -		return -EINVAL;
>  	spin_lock_irqsave(&ctx->wqh.lock, flags);
>  	if (ULLONG_MAX - ctx->count < n)
> -		n = (int) (ULLONG_MAX - ctx->count);
> +		n = ULLONG_MAX - ctx->count;
>  	ctx->count += n;
>  	if (waitqueue_active(&ctx->wqh))
>  		wake_up_locked_poll(&ctx->wqh, POLLIN);

The comment needs updating:

--- a/fs/eventfd.c~eventfd-change-int-to-__u64-in-eventfd_signal-fix
+++ a/fs/eventfd.c
@@ -46,10 +46,8 @@ struct eventfd_ctx {
  * value, and we signal this as overflow condition by returining a POLLERR
  * to poll(2).
  *
- * Returns @n in case of success, a non-negative number lower than @n in case
- * of overflow, or the following error codes:
- *
- * -EINVAL    : The value of @n is negative.
+ * Returns the amount by which the counter was incrememnted.  This will be less
+ * than @n if the counter has overflowed.
  */
 __u64 eventfd_signal(struct eventfd_ctx *ctx, __u64 n)
 {

This doesn't seem a very useful return value.  Shouldn't it inform the
user about overflow?  I guess the caller compares the return value to
`n'.  Of course, no callers bother doing this :(

What happens if the counter overflows?  It stops being updated.  What
is the user-visible effect of that?

(It's presumably not an issue at present with a 64-bit counter, but
might be a problem with your unexplained proposal of permitting
userspace to add to the counter)