Re: [patch v12 09/13] task isolation: add preempt notifier to sync per-CPU vmstat dirty info to thread info

Thomas Gleixner
2022-05-04T18:15:39Z

On Wed, May 04 2022 at 13:32, Marcelo Tosatti wrote:
> On Wed, Apr 27, 2022 at 02:09:16PM +0200, Thomas Gleixner wrote:
>> Aside of that, the existance of this preempt notifier alone tells me
>> that this is either a design fail or has no design in the first place.
>>
>> The state of vmstat does not matter at all at the point where a task is
>> scheduled in. It matters when an isolated task goes out to user space or
>> enters a VM.
>
> If the following happens, with two threads with names that mean whether
> a thread has task isolation enabled or not:
>
> Thread-no-task-isol, Thread-task-isol.
>
> Events:
>
> not-runnable Thread-task-isol
> runnable Thread-task-no-isol
> marks vmstat dirty Thread-task-no-isol (writes to some per-CPU vmstat
> counter)
> not-runnable Thread-task-no-isol
> runnable Thread-task-isol
>
> Then we have to transfer the "vmstat dirty" information from per-CPU
> bool to per-thread TIF_TASK_ISOL bit (so that the
> task_isolation_process_work thing executes on return to userspace).

That's absolute nonsense.

sched_out() isolated task
vmstat_dirty()
this_cpu_or(isolwork, VMSTAT);
sched_in() isolated task

return_to_user()
local_irq_disable();
exit_to_user_update_work()
task_isol_exit_to_user_prepare()
if (!isolated_task())
return;
if (this_cpu_read(isolwork) & current->isol_work_mask)
set_thread_flag(TIF_ISOL);

exit_to_user_mode_loop()
do {
local_irq_enable();
handle_TIF_bits();
local_irq_disable();
exit_to_user_update_work();
work = read_thread_flags();
} while (work & EXIT_WORK);

Solves the problem nicely with a minimal overhead for non-isolated
tasks.

Plus some of these isolwork bits could even be handled _after_ returning
from exit_do_user_mode_loop() if they are good to be done in irq
diasbled context.

> Sure, but who sets SYSCALL_TASK_ISOL_EXIT or SYSCALL_TASK_ISOL_EXIT ?

It's set once by the prctl() when an isolation feature is enabled for a
task and it's cleared by the prctl() when the last isolation feature is
disabled for the task.

That's then used in:

static inline bool isolated_task()
{
return current->XXXX_work & TASK_ISOL_EXIT;
}

IOW, the return to user path has

- _ONE_ extra cache hot conditional for non-isolated tasks.

- _ONE_ central place to transform the per cpu isolation muck into
the TIF flag.

See? No sprinkling of TIF bits, no preempt notifiers, nothing.

> Use TIF_TASK_ISOL for "task isolation configured and activated,
> quiesce vmstat work on return to userspace" only, and then have
> the "is vmstat per-CPU data dirty?" information held on
> task->syscall_work or task->isol_work ? (that will be probably be two
> cachelines).

See above.

> You'd still need the preempt notifier, though (unless i am missing
> something).

Yes, see above.

Using a preempt notifier isa design fail because it tags information at
a place where this information is absolutely irrelevant and subject to
change.

Aside of that this information is not a task property. vmmstat_is_dirty
is a per CPU property. The only point where this per CPU property is
relevant for a task is when the task is isolated and goes out to user
space or enters a VM.

Trying to carry this information in a task flag is fundamentaly wrong
for obvious reasons and causes pointless overhead and complexity for
absolutely no value.

Thanks,

tglx