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.
