Re: [patch v12 13/13] task isolation: only TIF_TASK_ISOL if task isolation is enabled

[Marcelo Tosatti]

On Wed, Apr 27, 2022 at 09:45:54AM +0200, Thomas Gleixner wrote:
> On Tue, Mar 15 2022 at 12:31, Marcelo Tosatti wrote:
> 
> $Subject does not qualify as a parseable sentence.
> 
> > This avoids processing of TIF_TASK_ISOL, when returning to userspace,
> > for tasks which do not have task isolation configured.
> 
> That's how kernel development works, right:
> 
>    1) Add half baken stuff
>    2) Apply duct tape on top
> 
> You know exactly, that this is _not_ the way it works.
> 
> This whole thing is half thought out tinkerware with [ill|un]defined
> semantics.

It seems to be inline with the remaining TIF_ bits:

                if (ti_work & _TIF_NOTIFY_RESUME)
                        tracehook_notify_resume(regs);

+               if (ti_work & _TIF_TASK_ISOL)
+                       task_isol_exit_to_user_mode();
+


And there is even:

--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -83,6 +83,7 @@ struct thread_info {
 #define TIF_NEED_RESCHED       3       /* rescheduling necessary */
 #define TIF_SINGLESTEP         4       /* reenable singlestep on user return*/
 #define TIF_SSBD               5       /* Speculative store bypass disable */
+#define TIF_TASK_ISOL          6       /* task isolation work pending */
 #define TIF_SPEC_IB            9       /* Indirect branch speculation mitigation */
 #define TIF_SPEC_L1D_FLUSH     10      /* Flush L1D on mm switches (processes) */
 #define TIF_USER_RETURN_NOTIFY 11      /* notify kernel of userspace return */

So the purpose of TIF_TASK_ISOL is to condense in a single bit the 
question: "is there task isolation work pending?"

By looking at the code, we see the sites where this bit is set are:

1) Task isolation configuration.

@@ -251,6 +257,11 @@ static int cfg_feat_quiesce_set(unsigned
        info->quiesce_mask = i_qctrl->quiesce_mask;
        info->oneshot_mask = i_qctrl->quiesce_oneshot_mask;
        info->conf_mask |= ISOL_F_QUIESCE;
+
+       if ((info->active_mask & ISOL_F_QUIESCE) &&
+           (info->quiesce_mask & ISOL_F_QUIESCE_VMSTATS))
+               set_thread_flag(TIF_TASK_ISOL);
+
        ret = 0;

2) copy_process (clone / fork):

@@ -303,6 +314,7 @@ int __copy_task_isol(struct task_struct
                new_info->active_mask = info->active_mask;

        tsk->task_isol_info = new_info;
+       set_ti_thread_flag(task_thread_info(tsk), TIF_TASK_ISOL);

        return 0;
 }

3) task isolation activation:

@@ -330,6 +342,10 @@ int prctl_task_isol_activate_set(unsigne
        info->active_mask = active_mask;
        ret = 0;

+       if ((info->active_mask & ISOL_F_QUIESCE) &&
+           (info->quiesce_mask & ISOL_F_QUIESCE_VMSTATS))
+               set_thread_flag(TIF_TASK_ISOL);
+
 out:
        return ret;

Would you prefer an explanation, in words, when these bits are set, when
they are cleared? 

So the meaning is:

+#define TIF_TASK_ISOL          6       /* task isolation work pending */

And the definition is "task isolation work" depends on what task
isolation features are implemented.

(honestly, seem pretty clear to me, but willing to improve...).