Hi Oleg,
Okay, get your point clearly, thanks for the explanation. Abort this patch.
Honglei
On 9/18/19 7:47 PM, Oleg Nesterov wrote:
On 09/18, Honglei Wang wrote:
On 9/17/19 9:47 PM, Oleg Nesterov wrote:
int main(int argc, char *argv[])
{
sigset_t waitset;
int signo;
sigemptyset(&waitset);
sigaddset(&waitset, SIGINT);
sigaddset(&waitset, SIGUSR1);
pthread_sigmask(SIG_BLOCK, &waitset, NULL);
for (;;) {
signo = sigwaitinfo(&waitset, NULL);
if (signo < 0)
err(1, "sigwaitinfo() failed");
if (signo == SIGUSR1)
printf("Receive SIGUSR1\n");
else
break;
}
return 0;
}
Abort
Well, I think we do not care. Userspace should handle -EINTR and restart
if necessary. And afaics this test case can equally "fail" if it races
with the system freezer, freeze_task() can be called between
set_current_state(TASK_INTERRUPTIBLE) and freezable_schedule() in
do_sigtimedwait().
OTOH, this is another indication that kernel/cgroup/freezer.c should
interact with freezer_do_not_count/freezer_count somehow.
It looks weird if a task is happily waiting for the desired signal, but is
woke up by an move from one cgroup to another (which is really not related
to any signal action from the point of userspace), doesn't it?
Again, imo a sane application should handle -EINTR. Nothing really weird.
There are other reasons why sigwaitinfo() can return -EINTR. Say, another
thread can steal a signal. Or, simply start you test-case and run
strace -p.
If you want to "fix" sigwaitinfo() - just make it restartable, that is all.
But see above, I don't think this makes any sense.
At this point, seems we need to be sure if it's necessary for cgroup to fix
this problem or not.
Well, to me this particular problem simply doesn't exist ;) However, cgroup
freezer has other, more serious problems.
diff --git a/kernel/cgroup/freezer.c b/kernel/cgroup/freezer.c
index 8cf010680678..08f6abacaa75 100644
--- a/kernel/cgroup/freezer.c
+++ b/kernel/cgroup/freezer.c
@@ -162,10 +162,14 @@ static void cgroup_freeze_task(struct task_struct *task, bool freeze)
if (freeze) {
task->jobctl |= JOBCTL_TRAP_FREEZE;
- signal_wake_up(task, false);
+
+ if (sigisemptyset(&task->real_blocked))
+ signal_wake_up(task, false);
This can't really help, ->real_blocked can be empty even if this task
sleeps in sigwaitinfo().
Sorry, I'm not quit clear about the 'sleep'. sigwaitinfo() sets
->real_blocked in do_sigtimedwait() and it goes to
freezable_schedule_hrtimeout_range() soon. After the task is woke up,
->real_blocked is cleared. Seems there is no chance to set ->real_blocked to
empty in this path. Do I miss something important here? Please correct me.
Hmm. do_sigtimedwait() does
tsk->real_blocked = tsk->blocked;
Now, if tsk->blocked was empty (sigisemptyset() == T), then why do you think
the sigisemptyset(real_blocked) check in cgroup_freeze_task() won't be true?
But in fact this doesn't really matter. This patch is wrong in any case.
sigwaitinfo() must not block cgroup freezer.
Oleg.
