Re: [patch v12 04/13] add prctl task isolation prctl docs and samples

Thomas Gleixner
2022-04-26T00:15:28Z

On Tue, Mar 15 2022 at 12:31, Marcelo Tosatti wrote:
> +++ linux-2.6/samples/task_isolation/task_isol.c

> +#ifdef PR_ISOL_FEAT_GET

This ifdef is there because the kernel on which this sample is compiled
does not support PR_ISOL_FEAT_GET? Try again...

> +int task_isol_setup(int oneshot)
> +{
> + int ret;
> + int errnosv;
> + unsigned long long fmask;
> + struct task_isol_quiesce_extensions qext;
> + struct task_isol_quiesce_control qctrl;
> +
> + /* Retrieve supported task isolation features */
> + ret = prctl(PR_ISOL_FEAT_GET, 0, &fmask, 0, 0);
> + if (ret == -1) {
> + perror("prctl PR_ISOL_FEAT");
> + return ret;
> + }
> + printf("supported features bitmask: 0x%llx\n", fmask);
> +
> + /* Retrieve supported ISOL_F_QUIESCE bits */
> + ret = prctl(PR_ISOL_FEAT_GET, ISOL_F_QUIESCE, &qext, 0, 0);

It makes a lot of sense to query ISOL_F_QUIESCE if the supported
features bitmask has not set it, right?

> + if (ret == -1) {
> + perror("prctl PR_ISOL_FEAT (ISOL_F_QUIESCE)");
> + return ret;
> + }
> + printf("supported ISOL_F_QUIESCE bits: 0x%llx\n",
> + qext.supported_quiesce_bits);
> +
> + fmask = 0;
> + ret = prctl(PR_ISOL_CFG_GET, I_CFG_FEAT, 0, &fmask, 0);
> + errnosv = errno;
> + if (ret != -1 && fmask != 0) {
> + printf("Task isolation parameters already configured!\n");
> + return ret;
> + }

Really useful because if that code is executed after a fork/clone then
it fails, not in that particular case, but this is _NOT_ a test case,
this is a sample to demonstrate usage.

> + if (ret == -1 && errnosv != ENODATA) {

How exactly ends this prctl() up returning ENODATA?

> + perror("prctl PR_ISOL_GET");
> + return ret;
> + }
> + memset(&qctrl, 0, sizeof(struct task_isol_quiesce_control));
> + qctrl.quiesce_mask = ISOL_F_QUIESCE_VMSTATS;
> + if (oneshot)
> + qctrl.quiesce_oneshot_mask = ISOL_F_QUIESCE_VMSTATS;
> +
> + ret = prctl(PR_ISOL_CFG_SET, I_CFG_FEAT, ISOL_F_QUIESCE,
> + QUIESCE_CONTROL, &qctrl);
> + if (ret == -1) {
> + perror("prctl PR_ISOL_CFG_SET");
> + return ret;
> + }
> + return ISOL_F_QUIESCE;

Very consistent return value:

int task_isol_setup(int oneshot)

which just works because the whole definition of the ISOL_F_* feature
space is bogus and inconsistent hackery, but if that ever goes up to bit
31bit+ then all of this is just crap.

> +}
> +
> +int task_isol_activate_set(unsigned long long mask)

While you here make sure that @mask is properly sized. Btw. uint64_t
exists for a reason...

> +int main(void)
> +{
> + int ret;
> + void *buf = malloc(4096);
> + unsigned long mask;

Works by chance...

> + memset(buf, 1, 4096);
> + ret = mlock(buf, 4096);
> + if (ret) {
> + perror("mlock");
> + return EXIT_FAILURE;
> + }
> +
> + ret = task_isol_setup(0);
> + if (ret == -1)
> + return EXIT_FAILURE;
> +
> + mask = ret;
> + /* enable quiescing on system call return, oneshot */
> + ret = task_isol_activate_set(mask);
> + if (ret)
> + return EXIT_FAILURE;
> +
> +#define NR_LOOPS 999999999
> +#define NR_PRINT 100000000
> + /* busy loop */

Really readable code.... Not.

> + while (ret < NR_LOOPS) {
> + memset(buf, 0, 4096);
> + ret = ret+1;

The kernel has a well define coding style which is not optional for
samples.

> +int main(void)
> +{
> + write_loops = 0;
> + do {
> +#define NR_LOOPS 999999999
> +#define NR_PRINT 100000000

Groan.

> + /* enable quiescing on system call return */
> + ret = task_isol_activate_set(mask);
> + if (ret)
> + return EXIT_FAILURE;
> +
> + /* busy loop */
> + while (ret < NR_LOOPS) {
> + memset(buf, 0xf, 4096);
> + ret = ret+1;
> + if (!(ret % NR_PRINT))
> + printf("wloop=%d loops=%d of %d\n", write_loops,
> + ret, NR_LOOPS);

This is really a brilliant example of design fail at the conceptual level:

task_isol_activate_set()
set_thread_flag(TIF_TASK_ISOL);
exit_to_user_mode()
if (thread_flag(TIF_TASK_ISOL)) {
handle_isol_muck() {
clear_thread_flag(TIF_TASK_ISOL);
....
}
printf()
sys_write()....
exit_to_user_mode()
....

---> which might coincidentaly quiesce stuff or not just
because something might have set TIF_TASK_ISOL or not.

Are you serious that setting TIF_TASK_ISOL from each of these envisioned
facilities which need quiescing is a maintainable approach?

That's a recipe for disaster and a guarantee for hard to diagnose
problems which ends up with a flood of non-sensical patches sprinkling
set_thread_flag(TIF_TASK_ISOL) all over the place just to cure the
symptoms.

Sure you can claim that this did not blow up in your face so far, but
that's a useless argument because _one_ out of the proposed 64 x 64 is
perhaps maintainable, but not anything beyond that.

Thanks,

tglx