Re: [patch V2 3/5] posix-cpu-timers: Provide mechanisms to defer timer handling to task_work

[Thomas Gleixner]

Oleg,

Oleg Nesterov <oleg@xxxxxxxxxx> writes:
> Looks correct to me, but I forgot everything about posix-timers.c

that's not a problem because this is about posix-cpu-timers.c :)

> this obviously means that the expired timer won't fire until the
> task returns to user-mode but probably we don't care.

If the signal goes to the task itself it does not matter at all because
it's going to be delivered when the task goes out to user space.

If the signal goes to a supervisor process, then it will be slightly
delayed but I could not find a problem with that at all.

I'll add more reasoning to the changelog on V3.

>> +#ifdef CONFIG_POSIX_CPU_TIMERS_TASK_WORK
>> +void posix_cpu_timers_work(struct callback_head *work);
>> +
>> +static inline void posix_cputimer_init_work(struct posix_cputimers *pct)
>> +{
>> +	pct->task_work.func = posix_cpu_timers_work;
>
> init_task_work() ?

Yeah.

>> +}
>> +#else
>> +static inline void posix_cputimer_init_work(struct posix_cputimers *pct) { }
>> +#endif
>> +
>>  static inline void posix_cputimers_init(struct posix_cputimers *pct)
>>  {
>>  	memset(pct, 0, sizeof(*pct));
>>  	pct->bases[0].nextevt = U64_MAX;
>>  	pct->bases[1].nextevt = U64_MAX;
>>  	pct->bases[2].nextevt = U64_MAX;
>> +	posix_cputimer_init_work(pct);
>>  }
>
> And I can't resist. I know this is a common practice, please ignore, but to me
>
> 	static inline void posix_cputimers_init(struct posix_cputimers *pct)
> 	{
> 		memset(pct, 0, sizeof(*pct));
> 		pct->bases[0].nextevt = U64_MAX;
> 		pct->bases[1].nextevt = U64_MAX;
> 		pct->bases[2].nextevt = U64_MAX;
> 	#ifdef CONFIG_POSIX_CPU_TIMERS_TASK_WORK
> 		init_task_work(&pct->task_work, posix_cpu_timers_work);
> 	#endif
> 	}
>
> looks better than 2 posix_cputimer_init_work() definitions above.

Gah, I hate ifdefs in the middle of the code :)

> Note also that signal_struct->posix_cputimers.task_work is never used, perhaps
> it would be better to move this task_work into task_struct? This way we do not
> even need to change posix_cputimers_init(), we call simply initialize
> init_task.posix_task_work.

Let me look into that.

Thanks,

        tglx