On 7/9/24 11:36, Oleg Nesterov wrote:
On 07/08, Pavel Begunkov wrote:
On 7/8/24 11:42, Oleg Nesterov wrote:
I don't think we should blame io_uring even if so far it is the only user
of TWA_SIGNAL.
And it's not entirely correct even for backporting purposes,
I'll pin it to when freezing was introduced then.
This is another problem introduced by 12db8b690010 ("entry: Add support for
TIF_NOTIFY_SIGNAL")
Ah, yes, I forgot NOTIFY_SIGNAL was split out of SIGPENDING
We need much more changes. Say, zap_threads() does the same and assumes
that only SIGKILL or freezeing can make dump_interrupted() true.
There are more similar problems. I'll try to think, so far I do not see
a simple solution...
Thanks. And there was some patching done before against dumping
being interrupted by task_work, indeed a reoccurring issue.
As for this particular problem, I agree it needs a simple/backportable fix.
relock:
+ clear_notify_signal();
+ if (unlikely(task_work_pending(current)))
+ task_work_run();
+
spin_lock_irq(&sighand->siglock);
Well, but can't we kill the same code at the start of get_signal() then?
Of course, in this case get_signal() should check signal_pending(), not
task_sigpending().
Should be fine,
Well, not really at least performance-wise... get_signal() should return
asap if TIF_NOTIFY_SIGNAL was the only reason to call get_signal().
but I didn't want to change the
try_to_freeze() -> __refrigerator() path, which also reschedules.
Could you spell please?
Let's say it calls get_signal() for freezing with a task_work pending.
Currently, it executes task_work and calls try_to_freeze(), which
puts the task to sleep. If we remove that task_work_run() before
try_to_freeze(), it would not be able to sleep. Sounds like it should
be fine, it races anyway, but I'm trying to avoid side effect for fixes.
Or perhaps something like the patch below makes more sense? I dunno...
It needs a far backporting, I'd really prefer to keep it
lean and without more side effects if possible, unless
there is a strong opinion on that.
Well, I don't think my patch is really worse in this sense. Just it
is buggy ;) it needs another recalc_sigpending() before goto start,
so lets forget it.
So I am starting to agree with your change as a workaround until we
find a clean solution (if ever ;).
But can I ask you to add this additional clear_notify_signal() +
task_work_run() to the end of do_freezer_trap() ? get_signal() is
already a mess...
Will change
-----------------------------------------------------------------------
Either way I have no idea whether a cgroup_task_frozen() task should
react to task_work_add(TWA_SIGNAL) or not.
Documentation/admin-guide/cgroup-v2.rst says
Writing "1" to the file causes freezing of the cgroup and all
descendant cgroups. This means that all belonging processes will
be stopped and will not run until the cgroup will be explicitly
unfrozen.
AFAICS this is not accurate, they can run but can't return to user-mode.
So I guess task_work_run() is fine.
IIUC it's a user facing doc, so maybe it's accurate enough from that
perspective. But I do agree that the semantics around task_work is
not exactly clear.
--
Pavel Begunkov
