LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Frederic Weisbecker <frederic@kernel.org>
To: Marcelo Tosatti <mtosatti@redhat.com>
Cc: linux-kernel@vger.kernel.org, Nitesh Lal <nilal@redhat.com>,
	Nicolas Saenz Julienne <nsaenzju@redhat.com>,
	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>
Subject: Re: [patch V3 2/8] add prctl task isolation prctl docs and samples
Date: Mon, 30 Aug 2021 13:38:46 +0200	[thread overview]
Message-ID: <20210830113846.GA17720@lothringen> (raw)
In-Reply-To: <20210827144416.GA186908@fuller.cnet>

On Fri, Aug 27, 2021 at 11:44:16AM -0300, Marcelo Tosatti wrote:
> On Fri, Aug 27, 2021 at 03:08:20PM +0200, Frederic Weisbecker wrote:
> > Ok so to make things clearer, may I suggest:
> > 
> >    s/PR_ISOL_FEAT/PR_ISOL_GET_FEAT

<nit>
In fact PR_ISOL_FEAT_GET so that it follows the same naming convention
than PR_ISOL_CFG_GET/PR_ISOL_PARAM_GET and PR_ISOL_ACTIVATE_GET


> > 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.
> 
> Yeah, nothing.
> 
> So the "quiesce once" feature, as i understand, was suggested by
> Christoph for the following type of application:
> 
> lat_loop:
> 
> 	do {
> 		events = pending_events();
> 		if (events & DATAPATH_EVENT)
> 			process_data()
> 	} while (!(events & UNFREQUENT_ERROR_EVENT))
> 
> 	syscall1()
> 	syscall2()
> 	...
> 	syscallN()
> 	goto lat_loop;
> 
> With the V3 patchset, one would have to:
> 
> 	prctl(PR_ISOL_SET, ISOL_F_OTHER, ...);
> 	prctl(PR_ISOL_SET, ISOL_F_QUIESCE, ISOL_F_QUIESCE_VMSTATS, ...);
> 	...
> 	prctl(PR_ISOL_CTRL_SET, ISOL_F_QUIESCE|ISOL_F_OTHER);
> lat_loop:
> 	do {
> 		events = pending_events();
> 		if (events & DATAPATH_EVENT)
> 			process_data()
> 	} while (!(events & UNFREQUENT_ERROR_EVENT))
> 
> 	/* disables quiescing while executing system calls */
> 	prctl(PR_ISOL_CTRL_SET, ISOL_F_OTHER);
> 	syscall1()
> 	syscall2()
> 	...
> 	syscallN()
> 
> 	/* no more system calls, enables quiescing */
> 	prctl(PR_ISOL_CTRL_SET, ISOL_F_QUIESCE|ISOL_F_OTHER);
> 	goto lat_loop;
> 
> But for an interface with less system calls (true "quiesce once") one could do:
> 
> 	prctl(PR_ISOL_SET, ISOL_F_OTHER, ...);
> 	/* rather than do it at _CTRL_SET as you suggest, enable it at
> 	 * configuration time.
> 	 */
> 	prctl(PR_ISOL_SET, ISOL_F_QUIESCE, ISOL_F_QUIESCE_VMSTATS|ISOL_F_QUIESCE_ONCE, ...);
> 	...
> 
> lat_loop:
> 	prctl(PR_ISOL_CTRL_SET, ISOL_F_QUIESCE|ISOL_F_OTHER);
> 	do {
> 		events = pending_events();
> 		if (events & DATAPATH_EVENT)
> 			process_data()
> 	} while (!(events & UNFREQUENT_ERROR_EVENT))
> 
> 	/* disables quiescing while executing system calls */
> 	syscall1()
> 	syscall2()
> 	...
> 	syscallN()
> 
> 	goto lat_loop;
> 
> But see how it starts to get weird: both versions (new feature, 
> ISOL_F_QUIESCE_ONCE, or new "quiesce feature', ISOL_F_QUIESCE_ONCE)
> are using space reserved to
> 
> "a list of different features" 
> or
> "a list of different quiesce features".
> 
> To add something which is not either a new task isolation 
> feature or quiesce feature, but a separate control
> (which could apply to all of features, or which one might want
> to apply only to certain features, and in that case a bitmap
> might be specified).
> 
> So i think adding a new parameter such as:
> 
> prctl(PR_ISOL_SET, ISOL_F_QUIESCE, CMD, arg, ...);
> 
> is a good idea. So one can have (with two commands, SET_QUIESCE 
> and SET_ONESHOT).
> 
> prctl(PR_ISOL_SET, ISOL_F_QUIESCE, SET_QUIESCE, ISOL_F_QUIESCE_VMSTAT);
> prctl(PR_ISOL_SET, ISOL_F_QUIESCE, SET_ONESHOT, ISOL_F_QUIESCE_VMSTAT);
> 
> Then its possible to add random commands with random parameters
> (rather than be limited by a single bitmask to control quiescing).
> 
> Does that make sense?

We can but it means that the ONESHOT property applies to all ISOL_F_QUIESCE
features. So you can't, for example, quiesce ISOL_F_QUIESCE_VMSTAT only once
and quiesce ISOL_F_QUIESCE_FOO all the time.

I have no idea if it matters or not but be aware of limitations.

> > > +#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.
> 
> Would be good to do this for all cases, so you can extend the
> struct (or pad it).

Yep.

> > 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... 
> 
> Yes, it could as well. But there are no more bits for older clone
> interfaces, and clone3 seems to be problematic (moreover, this
> interface would need backporting to older kernels).

Another way to go is to use all the features as a mask in PR_ISOL_CFG_SET:

prctl(PR_ISOL_FEAT_GET, 0, &all_features, ...)
prctl(PR_ISOL_CFG_SET, &all_features, PR_ISOL_INHERIT...)

or simply:

prctl(PR_ISOL_CFG_SET, -1, PR_ISOL_INHERIT...)

or even:

prctl(PR_ISOL_CFG_SET, 0, PR_ISOL_INHERIT...)

Thanks.

  reply	other threads:[~2021-08-30 11:38 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-24 15:24 [patch V3 0/8] extensible prctl task isolation interface and vmstat sync Marcelo Tosatti
2021-08-24 15:24 ` [patch V3 1/8] add basic task isolation prctl interface Marcelo Tosatti
2021-08-24 15:24 ` [patch V3 2/8] add prctl task isolation prctl docs and samples Marcelo Tosatti
2021-08-26  9:59   ` Frederic Weisbecker
2021-08-26 12:11     ` Marcelo Tosatti
2021-08-26 19:15       ` Christoph Lameter
2021-08-26 20:37         ` Marcelo Tosatti
2021-08-27 13:08       ` Frederic Weisbecker
2021-08-27 14:44         ` Marcelo Tosatti
2021-08-30 11:38           ` Frederic Weisbecker [this message]
2021-09-01 13:11   ` Nitesh Lal
2021-09-01 17:34     ` Marcelo Tosatti
2021-09-01 17:49       ` Nitesh Lal
2021-08-24 15:24 ` [patch V3 3/8] task isolation: sync vmstats on return to userspace Marcelo Tosatti
2021-09-10 13:49   ` nsaenzju
2021-08-24 15:24 ` [patch V3 4/8] procfs: add per-pid task isolation state Marcelo Tosatti
2021-08-24 15:24 ` [patch V3 5/8] task isolation: sync vmstats conditional on changes Marcelo Tosatti
2021-08-25  9:46   ` Christoph Lameter
2021-08-24 15:24 ` [patch V3 6/8] KVM: x86: call isolation prepare from VM-entry code path Marcelo Tosatti
2021-08-24 15:24 ` [patch V3 7/8] mm: vmstat: move need_update Marcelo Tosatti
2021-08-24 15:24 ` [patch V3 8/8] mm: vmstat_refresh: avoid queueing work item if cpu stats are clean Marcelo Tosatti
2021-08-25  9:30   ` Christoph Lameter
2021-09-01 13:05   ` Nitesh Lal
2021-09-01 17:32     ` Marcelo Tosatti
2021-09-01 18:33       ` Marcelo Tosatti
2021-09-03 17:38         ` Nitesh Lal
2021-08-25 10:02 ` [patch V3 0/8] extensible prctl task isolation interface and vmstat sync Marcelo Tosatti

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210830113846.GA17720@lothringen \
    --to=frederic@kernel.org \
    --cc=abelits@belits.com \
    --cc=cl@linux.com \
    --cc=juri.lelli@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mtosatti@redhat.com \
    --cc=nilal@redhat.com \
    --cc=nsaenzju@redhat.com \
    --cc=peterx@redhat.com \
    --cc=peterz@infradead.org \
    --subject='Re: [patch V3 2/8] add prctl task isolation prctl docs and samples' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).