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

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

[Sha Zhengju]

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.  

changelog v2..v1
1. Changing 'n' to an unsigned type makes the "if (n < 0)" test a no-op.

Signed-off-by: Sha Zhengju <handai.szj@taobao.com>

---
 fs/eventfd.c            |    6 ++----
 include/linux/eventfd.h |    2 +-
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/eventfd.c b/fs/eventfd.c
index dba15fe..f570e7a 100644
--- 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);
diff --git a/include/linux/eventfd.h b/include/linux/eventfd.h
index 91bb4f2..3c3ef19 100644
--- a/include/linux/eventfd.h
+++ b/include/linux/eventfd.h
@@ -34,7 +34,7 @@ void eventfd_ctx_put(struct eventfd_ctx *ctx);
 struct file *eventfd_fget(int fd);
 struct eventfd_ctx *eventfd_ctx_fdget(int fd);
 struct eventfd_ctx *eventfd_ctx_fileget(struct file *file);
-int eventfd_signal(struct eventfd_ctx *ctx, int n);
+__u64 eventfd_signal(struct eventfd_ctx *ctx, __u64 n);
 ssize_t eventfd_ctx_read(struct eventfd_ctx *ctx, int no_wait, __u64 *cnt);
 int eventfd_ctx_remove_wait_queue(struct eventfd_ctx *ctx, wait_queue_t *wait,
 				  __u64 *cnt);
-- 
1.7.4.1

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

[Andrew Morton]

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)

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

[Sha Zhengju]

On 04/19/2012 07:53 AM, Andrew Morton wrote:
> 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?
>

yeah... If callers check the return value, there is no difference to
return a subtracted value or an error code in case of overflow.
But even if the counter overflows, someone still will be wakeup
but get the incorrect number(depending on whether it cares the
value or not).
We can only inform about overflow but don't wake up waiters just
as eventfd_write() does.

> (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)
> .
>