On 10/2/20 9:14 AM, Oleg Nesterov wrote:
> Heh. To be honest I don't really like 1-2 ;)
>
> Unfortunately, I do not see a better approach right now. Let me think
> until Monday, it is not that I think I will find a better solution, but
> I'd like to try anyway.
>
> Let me comment 3/3 for now.
Thanks, appreciate your time on this!
>> +static void task_work_signal(struct task_struct *task)
>> +{
>> +#ifndef TIF_TASKWORK
>> + unsigned long flags;
>> +
>> + /*
>> + * Only grab the sighand lock if we don't already have some
>> + * task_work pending. This pairs with the smp_store_mb()
>> + * in get_signal(), see comment there.
>> + */
>> + if (!(READ_ONCE(task->jobctl) & JOBCTL_TASK_WORK) &&
>> + lock_task_sighand(task, &flags)) {
>> + task->jobctl |= JOBCTL_TASK_WORK;
>> + signal_wake_up(task, 0);
>> + unlock_task_sighand(task, &flags);
>> + }
>> +#else
>> + set_tsk_thread_flag(task, TIF_TASKWORK);
>> + set_notify_resume(task);
>> +#endif
>
> Again, I can't understand. task_work_signal(task) should set TIF_TASKWORK
> to make signal_pending() = T _and_ wake/kick the target up, just like
> signal_wake_up() does. Why do we set TIF_NOTIFY_RESUME ?
>
> So I think that if we are going to add TIF_TASKWORK we should generalize
> this logic and turn it into TIF_NOTIFY_SIGNAL. Similar to TIF_NOTIFY_RESUME
> but implies signal_pending().
>
> IOW, something like
>
> void set_notify_signal(task)
> {
> if (!test_and_set_tsk_thread_flag(task, TIF_NOTIFY_SIGNAL)) {
> if (!wake_up_state(task, TASK_INTERRUPTIBLE))
> kick_process(t);
> }
> }
>
> // called by exit_to_user_mode_loop() if ti_work & _TIF_NOTIFY_SIGNAL
> void tracehook_notify_signal(regs)
> {
> clear_thread_flag(TIF_NOTIFY_SIGNAL);
> smp_mb__after_atomic();
> if (unlikely(current->task_works))
> task_work_run();
> }
>
> This way task_work_run() doesn't need to clear TIF_NOTIFY_SIGNAL and it can
> have more users.
>
> What do you think?
I like that. It'll achieve the same thing as far as I'm concerned, but not
tie the functionality to task_work. Not that we have anything that'd use
it right now, but it still seems like a better base.
I'll adapt patch 2+3 for this, thanks Oleg.
--
Jens Axboe
