[CRIU] Re: [RFC PATCH] posix timers: allocate timer id per task

[Pavel Emelyanov]

On 10/15/2012 07:30 PM, Stanislav Kinsbursky wrote:
> 15.10.2012 19:26, Pavel Emelyanov пишет:
>> On 10/15/2012 07:24 PM, Stanislav Kinsbursky wrote:
>>> 15.10.2012 19:16, Pavel Emelyanov пишет:
>>>> On 10/15/2012 06:01 PM, Stanislav Kinsbursky wrote:
>>>>> This patch replaces global idr with global hash table for posix timers and
>>>>> makes timer ids unique not globally, but per task. Next free timer id is type
>>>>> of integer and stored on signal struct (posix_timer_id). If free timer id
>>>>> reaches negative value on timer creation, it will be dropped to zero and
>>>>> -EAGAIN will be returned to user.
>>>>>
>>>>> Hash table is size of page (4KB).
>>>>>
>>>>> Key is constructed as follows:
>>>>> key = hash_ptr(current->signal) ^ hash_32(posix_timer_id);
>>>>>
>>>>> ---
>>>>>    include/linux/posix-timers.h |    1
>>>>>    include/linux/sched.h        |    4 +-
>>>>>    kernel/posix-timers.c        |  112 ++++++++++++++++++++++++++++--------------
>>>>>    3 files changed, 78 insertions(+), 39 deletions(-)
>>>>>
>>>>
>>>>
>>>>> +{
>>>>> +	struct signal_struct *sig = current->signal;
>>>>> +	struct hlist_head *head;
>>>>> +	int ret = -ENOENT;
>>>>> +
>>>>> +	do {
>>>>> +		spin_lock(&hash_lock);
>>>>> +		head = &posix_timers_hashtable[hash(sig, sig->posix_timer_id)];
>>>>> +		if (__posix_timers_find(head, sig, sig->posix_timer_id) == NULL) {
>>>>> +			hlist_add_head(&timer->t_hash, head);
>>>>> +			ret = sig->posix_timer_id++;
>>>>> +		} else if (++sig->posix_timer_id < 0) {
>>>>> +			sig->posix_timer_id = 0;
>>>>> +			ret = -EAGAIN;
>>>>
>>>> What for? Overflow is OK, no need in restart.
>>>>
>>>
>>> I was thinking, that we need some way to stop here in case of corner case, when
>>> all id's are busy.
>>> I'm don't think that anyone can reach even INT_MAX timers in one task, so
>>> INT_MAX looks enough. This "EGAIN" will be returned to user.
>>
>> while (1) {
>> 	add_timer()
>> 	del_timer()
>> }
>>
>> will overflow it very fast.
>>
> 
> Sure. And add_timer() caller will receive -EAGAIN.

It didn't behave _that_ _before_ this patch.

> This is already a part of timer_create() API.

It doesn't mean you can report "I'm confused, try one more time please".

> There should be some way to return from syscall in case of all id's are busy.

+1

> Actually, I can check for 0 after add and return -EAGAIN in this case.

-1

> Is it better from your pow?
> 
>>>>> +		}
>>>>> +		spin_unlock(&hash_lock);
>>>>> +	} while (ret == -ENOENT);
>>>>> +	return ret;
>>>>> +}
>>>>
>>>
>>>
>>
>>
> 
>