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

[Sha Zhengju <handai.szj@gmail.com>]

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