[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87a6bxjiyi.ffs@tglx>
Date: Wed, 04 May 2022 19:39:17 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Marcelo Tosatti <mtosatti@...hat.com>
Cc: linux-kernel@...r.kernel.org, Nitesh Lal <nilal@...hat.com>,
Nicolas Saenz Julienne <nsaenzju@...hat.com>,
Frederic Weisbecker <frederic@...nel.org>,
Christoph Lameter <cl@...ux.com>,
Juri Lelli <juri.lelli@...hat.com>,
Peter Zijlstra <peterz@...radead.org>,
Alex Belits <abelits@...its.com>, Peter Xu <peterx@...hat.com>,
Daniel Bristot de Oliveira <bristot@...hat.com>,
Oscar Shiang <oscar0225@...email.tw>
Subject: Re: [patch v12 09/13] task isolation: add preempt notifier to sync
per-CPU vmstat dirty info to thread info
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
Powered by blists - more mailing lists