Re: [patch V3 2/8] add prctl task isolation prctl docs and samples

[Frederic Weisbecker <frederic@kernel.org>]

On Thu, Aug 26, 2021 at 09:11:31AM -0300, Marcelo Tosatti wrote:
> Hi Frederic,
> 
> On Thu, Aug 26, 2021 at 11:59:58AM +0200, Frederic Weisbecker wrote:
> > > +Note: the prctl interface is independent of nohz_full=.
> > > +
> > > +The prctl options are:
> > > +
> > > +
> > > +        - PR_ISOL_FEAT: Retrieve supported features.
> > > +        - PR_ISOL_GET: Retrieve task isolation parameters.
> > > +        - PR_ISOL_SET: Set task isolation parameters.
> > > +        - PR_ISOL_CTRL_GET: Retrieve task isolation state.
> > > +        - PR_ISOL_CTRL_SET: Set task isolation state.
> > > +        - PR_ISOL_GET_INT: Retrieve internal parameters.
> > > +        - PR_ISOL_SET_INT: Retrieve internal parameters.
> > 
> > There should be some short summary here to explain the difference
> > between parameter, state, and internal parameter. I personally have
> > no clue so far.
> 
> Yes, those have been written without clear definitions and can be
> improved (it makes sense to me, so please indicate what is not 
> clear to you). So:
> 
> * "Feature": a generic name for a task isolation feature.
> Examples of features could be logging, new operating modes (syscalls
> disallowed), userspace notifications, etc. One feature is quiescing.
> 
> * "Parameter": a specific choice from a given set of possible choices
> that dictate how the particular feature in question should act.
> 
> * "Internal parameter": A parameter (as in above) but not related to
> task isolation features themselves, but to "internal characteristics"
> (well, there is only one example of internal parameter so far
> and that is inheritance across clone/fork).
> 
> Maybe "internal parameter" is a bad name and something different should
> be used instead ?
> 
> Should i add the description aboves to the document file?

Ok so to make things clearer, may I suggest:

   s/PR_ISOL_FEAT/PR_ISOL_GET_FEAT

to make it more obvious that we are not going to write or configure something.

Also:

  s/PR_ISOL_SET/PR_ISOL_CFG_SET  or  s/PR_ISOL_SET/PR_ISOL_PARAM_SET
  s/PR_ISOL_GET/PR_ISOL_CFG_GET  or  s/PR_ISOL_GET/PR_ISOL_PARAM_GET

because SET or GET alone are too general. I first thought they were the
activation interface whereas they are only the configuration stage.

And then PR_ISOL_CTRL_GET/SET look good. Although perhaps
PR_ISOL_ACTIVATE_SET/GET would probably be clearer. Or even this is where
the trimmed name PR_ISOL_SET/GET would make sense.

> > > +---------------------
> > > +Interface description
> > > +---------------------
> > > +
> > > +**PR_ISOL_FEAT**:
> > > +
> > > +        Returns the supported features and feature
> > > +        capabilities, as a bitmask::
> > > +
> > > +                prctl(PR_ISOL_FEAT, feat, arg3, arg4, arg5);
> > > +
> > > +        The 'feat' argument specifies whether to return
> > > +        supported features (if zero), or feature capabilities
> > > +        (if not zero). Possible non-zero values for 'feat' are:
> > > +
> > > +        - ``ISOL_F_QUIESCE``:
> > > +
> > > +                Returns a bitmask containing which kernel
> > > +                activities are supported for quiescing.
> > > +
> > > +        Features and its capabilities are defined at
> > > include/uapi/linux/task_isolation.h.
> > 
> > Preferably have feat a parameter name. We never know if we want
> > to extend it in the future.
> 
> It is a parameter name:
> 
> prctl(PR_ISOL_FEAT, feat-A, arg3, arg4, arg5);
> 
> prctl(PR_ISOL_FEAT, feat-B, arg3, arg4, arg5);
> 
> And yes, the idea is that new features can be added.
> 
> So unless i misunderstood you, there are no changes necessary here.

Ok, indeed that was my misunderstanding.

> > > +**PR_ISOL_GET**:
> > > +
> > > +        Retrieve task isolation feature configuration.
> > > +        The general format is::
> > > +
> > > +                prctl(PR_ISOL_GET, feat, arg3, arg4, arg5);
> > > +
> > > +        The 'feat' argument specifies whether to return
> > > +        configured features (if zero), or individual feature
> > > +        configuration (if not zero).
> > 
> > You might need to elaborate a bit on the "feat" behaviour difference.
> 
> Not sure what you mean? There is only one "feat" yet, which is
> ISOL_F_QUIESCE:

Sorry my misunderstanding again. So if I understand correctly prctl(PR_ISOL_GET,
0, ...) returns a mask of all features that have been configured through
PR_ISOL_SET(), right?

> > > +**PR_ISOL_SET**:
> > > +
> > > +        Configures task isolation features. The general format is::
> > > +
> > > +                prctl(PR_ISOL_SET, feat, arg3, arg4, arg5);
> > > +
> > > +        The 'feat' argument specifies which feature to configure.
> > > +        Possible values for feat are:
> > > +
> > > +        - ``ISOL_F_QUIESCE``:
> > > +
> > > +                The 'arg3' argument is a bitmask specifying which
> > > +                kernel activities to quiesce. Possible bit sets are:
> > > +
> > > +                - ``ISOL_F_QUIESCE_VMSTATS``
> > > +
> > > +                  VM statistics are maintained in per-CPU counters to
> > > +                  improve performance. When a CPU modifies a VM statistic,
> > > +                  this modification is kept in the per-CPU counter.
> > > +                  Certain activities require a global count, which
> > > +                  involves requesting each CPU to flush its local counters
> > > +                  to the global VM counters.
> > > +
> > > +                  This flush is implemented via a workqueue item, which
> > > +                  might schedule a workqueue on isolated CPUs.
> > > +
> > > +                  To avoid this interruption, task isolation can be
> > > +                  configured to, upon return from system calls, synchronize
> > > +                  the per-CPU counters to global counters, thus avoiding
> > > +                  the interruption.
> > > +
> > > +                  To ensure the application returns to userspace
> > > +                  with no modified per-CPU counters, its necessary to
> > > +                  use mlockall() in addition to this isolcpus flag.
> > 
> > So prctl(PR_ISOL_SET, ISOL_F_QUIESCE, ISOL_F_QUIESCE_VMSTATS, ...) will quiesce
> > on all subsequent return to userspace, right?
> 
> Yes. Hum, i think i dropped that clarification (by mistake). Will re-add
> it.

So how are we going to implement oneshot quiescing? As in quiescing only upon
the return of a given prctl().

Maybe using a feature something like ISOL_F_QUIESCE_ONCE?

But then suppose I do this:

prctl(PR_ISOL_SET, ISOL_F_QUIESCE_ONCE, ISOL_F_QUIESCE_VMSTATS, ...)
prctl(PR_ISOL_CTRL_SET, ISOL_F_QUIESCE_ONCE, ...) //will quiesce on this return only
prctl(PR_ISOL_CTRL_GET,  ...)

What should PR_ISOL_CTRL_GET return above? Probably nothing.

> 
> > > +
> > > +**PR_ISOL_CTRL_GET**:
> > > +
> > > +        Retrieve task isolation control.
> > > +
> > > +                prctl(PR_ISOL_CTRL_GET, 0, 0, 0, 0);
> > > +
> > > +        Returns which isolation features are active.
> > > +
> > > +**PR_ISOL_CTRL_SET**:
> > > +
> > > +        Activates/deactivates task isolation control.
> > > +
> > > +                prctl(PR_ISOL_CTRL_SET, mask, 0, 0, 0);
> > > +
> > > +        The 'mask' argument specifies which features
> > > +        to activate (bit set) or deactivate (bit clear).
> > > +
> > > +        For ISOL_F_QUIESCE, quiescing of background activities
> > > +        happens on return to userspace from the
> > > +        prctl(PR_ISOL_CTRL_SET) call, and on return from
> > > +        subsequent system calls.
> > 
> > Now I'm lost again on the difference with PR_ISOL_SET
> 
> PR_ISOL_SET configures the features parameters.
> 
> PR_ISOL_CTRL_SET _activates_ task isolation.
> 
> You earlier wrote:
> 
> "I would rather decouple the above with, for modes:
> 
>   PR_TASK_ISOLATION_SET
>   PR_TASK_ISOLATION_GET
> 
> And for oneshot requests:
> 
>   PR_TASK_ISOLATION_REQUEST"
> 
> Now we have PR_ISOL_SET/PR_ISOL_GET (to configure the parameters of 
> task isolation features), and PR_ISOL_CTRL_SET to activate that
> isolation (and we pass a bitmask to PR_ISOL_CTRL_SET indicating what
> features should be active). How the particular features behave 
> is determined at PR_ISOL_SET time.

I guess that makes sense. This way we can quiesce everything in one go
instead of issuing a prctl() for each features, which adds further noise.
Sounds proper.

> 
> This allows the administrator to, via chisol, configure task isolation:
> 
> +
> +       if (argc - optind < 1) {
> +	       warnx(_("bad usage"));
> +               errtryhelp(EXIT_FAILURE);
> + 	}
> +
> +       if (quiesce_act_mask) {
> + 	        ret = prctl(PR_ISOL_SET, ISOL_F_QUIESCE, quiesce_act_mask, 0, 0);
> +	       if (ret == -1) {
> +	                perror("prctl PR_ISOL_SET");
> +                       exit(EXIT_FAILURE);
> +	       }
> +       }
> 
> And the application, which has to be modified only once with:
> 
> +#ifdef PR_ISOL_GET
> +       ret = prctl(PR_ISOL_GET, 0, 0, 0, 0);
> +	if (ret != -1) {
> +               unsigned long mask = ret;
> +
> +               TEST0(prctl(PR_ISOL_CTRL_SET, mask, 0, 0, 0));
> +	}
> +#endif
> +
>         frc(&ts2);
>         do {
>                 workload_fn(t->dst_buf, t->src_buf, g.workload_mem_size);
> 
> Makes sense?

Yes! Btw you might want to fetch the mask of PR_ISOL_GET into the
second parameter instead of using the return value which is only
32 bits or prctl() and the highest significant bit is even reserved
for the error.

> > > +**PR_ISOL_GET_INT**:
> > > +
> > > +        Retrieves task isolation internal parameters.
> > > +
> > > +        The general format is::
> > > +
> > > +                prctl(PR_ISOL_GET_INT, cmd, arg3, arg4, arg5);
> > > +
> > > +        The 'cmd' argument specifies which parameter to configure.
> > > +        Possible values for cmd are:
> > > +
> > > +        - ``INHERIT_CFG``:
> > > +
> > > +                Retrieve inheritance configuration.
> > > +
> > > +                The 'arg3' argument is a pointer to a struct
> > > +                inherit_control::
> > > +
> > > +                        struct task_isol_inherit_control {
> > > +                                __u8    inherit_mask;
> > > +                                __u8    pad[7];
> > > +                        };
> > > +
> > > +                See PR_ISOL_SET_INT description below for meaning
> > > +                of structure fields.
> > > +
> > > +**PR_ISOL_SET_INT**:
> > > +
> > > +        Sets task isolation internal parameters.
> > > +
> > > +        The general format is::
> > > +
> > > +                prctl(PR_ISOL_SET_INT, cmd, arg3, arg4, arg5);
> > > +
> > > +        The 'cmd' argument specifies which parameter to configure.
> > > +        Possible values for cmd are:
> > > +
> > > +        - ``INHERIT_CFG``:
> > > +
> > > +                Set inheritance configuration when a new task
> > > +                is created via fork and clone.
> > > +
> > > +                The 'arg3' argument is a pointer to a struct
> > > +                inherit_control::
> > > +
> > > +                        struct task_isol_inherit_control {
> > > +                                __u8    inherit_mask;
> > > +                                __u8    pad[7];
> > > +                        };
> > > +
> > > +                inherit_mask is a bitmask that specifies which part
> > > +                of task isolation to be inherited:
> > > +
> > > +                - Bit ISOL_INHERIT_CONF: Inherit task isolation configuration.
> > > +                  This is the stated written via prctl(PR_ISOL_SET, ...).
> > > +
> > > +                - Bit ISOL_INHERIT_ACTIVE: Inherit task isolation activation
> > > +                  (requires ISOL_INHERIT_CONF to be set). The new task
> > > +                  should behave, right after fork/clone, in the same manner
> > > +                  as the parent task _after_ it executed:
> > > +
> > > +                        prctl(PR_ISOL_CTRL_SET, mask, 0, 0, 0);
> > > +
> > > +                  with a valid mask.
> > 
> > I'm wondering if those things shouldn't be set on arg4 for PR_ISOL_SET instead?
> > Arguably having a whole prctl for that makes it easier to extend. But then
> > PR_ISOL_SET_INT must always be called before PR_ISOL_SET, otherwise we create
> > noise, right?
> 
> It has to be called before PR_ISOL_CTRL_SET, yes.
> 
> Decided on a separate prctl because the inheritance control
> is not a feature itself: it acts on all features (or how task isolation
> features are inherited across fork/clone).
> 
> So yes, first idea was to "lets add this to PR_ISOL_SET", but then it
> became weird to have something that controls the features as a feature
> itself... It would be ISOL_F_INHERIT_CONTROL. Can change to that, if 
> you prefer.

Funny but that would work. Either way, let's keep things that way for now.
Just the naming is unfortunate.

Well that could be a clone flag after all... But what about exec()? Should we
make its inheritance tunable? Well we can still extend the interface later if
necessary for that.

Thanks.