On 11/11, Cyrill Gorcunov wrote:
Anyway while looking into patch I got wonder why
+static int wait_for_notify_count(struct task_struct *tsk) +{
- for (;;) {
return -EINTR;set_current_state(TASK_KILLABLE);if (!tsk->signal->notify_count)break;We have no any barrier here in fetching @notify_count? I mean updating this value is done under locks (spin or read/write) in turn condition test is a raw one. Not a big deal since set_current_state() and schedule()
Yes, so I think that, correctness-wise, this doesn't need additional barriers.
but I've been a bit confused that we don't use some read_once here or something.
Yes, this needs READ_ONCE() to avoid the warnings from KCSAN. And in fact this code was written with READ_ONCE() but I removed it before sending this RFC.
I was going to do this later. I always forget how KCSAN works, IIUC I also need to add WRITE_ONCE() into exit_notify() and __exit_signal() to make KCSAN happy, even if ->notify_count is always updated under the lock.
Same for the "if (me->signal->group_exec_task == me)" check in begin_new_exec().
Right now I would like to know if this RFC (approach) makes any sense, especially because 3/3 adds a user-visible change.
Oleg.