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

[Pavel Emelyanov]

On 10/15/2012 07:36 PM, Stanislav Kinsbursky wrote:
> 15.10.2012 19:33, 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.
>>
> 
> Yes, it did.
> 
> # man timer_create
> 
> ERRORS
>         EAGAIN Temporary error during kernel allocation of timer structures.

Rule #1 in kernel programming -- never ever thing kernel works it is documented
to work (c).

Stas, it did _NOT_ behave like that in the test I provided, it should _NOT_ start
doing so.

> 
>>> This is already a part of timer_create() API.
>>
>> It doesn't mean you can report "I'm confused, try one more time please".
>>
> 
> O_o
> 
>>> Actually, I can check for 0 after add and return -EAGAIN in this case.
>>
>> -1
>>
> 
> Please, share the better idea, if you have it.

Remember ID we had on signal-> when started, then report error in case we
meet it again.