Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Marcelo Tosatti <mtosatti@redhat.com>, linux-kernel@vger.kernel.org
Cc:     Nitesh Lal <nilal@redhat.com>,
        Nicolas Saenz Julienne <nsaenzju@redhat.com>,
        Frederic Weisbecker <frederic@kernel.org>,
        Christoph Lameter <cl@linux.com>,
        Juri Lelli <juri.lelli@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Alex Belits <abelits@belits.com>, Peter Xu <peterx@redhat.com>,
        Daniel Bristot de Oliveira <bristot@redhat.com>,
        Oscar Shiang <oscar0225@livemail.tw>,
        Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: [patch v12 04/13] add prctl task isolation prctl docs and samples
In-Reply-To: <20220315153313.908516477@fedora.localdomain>
Date:   Tue, 26 Apr 2022 02:15:22 +0200
Message-ID: <87r15kzomd.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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