Messages in this thread | | | From | Thomas Gleixner <> | Subject | Re: [patch v12 04/13] add prctl task isolation prctl docs and samples | Date | Tue, 26 Apr 2022 02:15:22 +0200 |
| |
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
| |