xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Jan Beulich <jbeulich@suse.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>, Wei Liu <wl@xen.org>
Subject: Re: [PATCH v4 3/3] x86/time: avoid reading the platform timer in rendezvous functions
Date: Wed, 21 Apr 2021 12:06:34 +0200	[thread overview]
Message-ID: <88819ae1-d021-9192-4be7-a70064f23feb@suse.com> (raw)
In-Reply-To: <YH79ZxNRhW24jmUS@Air-de-Roger>

On 20.04.2021 18:12, Roger Pau Monné wrote:
> On Thu, Apr 01, 2021 at 11:55:10AM +0200, Jan Beulich wrote:
>> Reading the platform timer isn't cheap, so we'd better avoid it when the
>> resulting value is of no interest to anyone.
>>
>> The consumer of master_stime, obtained by
>> time_calibration_{std,tsc}_rendezvous() and propagated through
>> this_cpu(cpu_calibration), is local_time_calibration(). With
>> CONSTANT_TSC the latter function uses an early exit path, which doesn't
>> explicitly use the field. While this_cpu(cpu_calibration) (including the
>> master_stime field) gets propagated to this_cpu(cpu_time).stamp on that
>> path, both structures' fields get consumed only by the !CONSTANT_TSC
>> logic of the function.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> v4: New.
>> ---
>> I realize there's some risk associated with potential new uses of the
>> field down the road. What would people think about compiling time.c a
>> 2nd time into a dummy object file, with a conditional enabled to force
>> assuming CONSTANT_TSC, and with that conditional used to suppress
>> presence of the field as well as all audited used of it (i.e. in
>> particular that large part of local_time_calibration())? Unexpected new
>> users of the field would then cause build time errors.
> 
> Wouldn't that add quite a lot of churn to the file itself in the form
> of pre-processor conditionals?

Possibly - I didn't try yet, simply because of fearing this might
not be liked even without presenting it in patch form.

> Could we instead set master_stime to an invalid value that would make
> the consumers explode somehow?

No idea whether there is any such "reliable" value.

> I know there might be new consumers, but those should be able to
> figure whether the value is sane by looking at the existing ones.

This could be the hope, yes. But the effort of auditing the code to
confirm the potential of optimizing this (after vaguely getting the
impression there might be room) was non-negligible (in fact I did
three runs just to be really certain). This in particular means
that I'm in no way certain that looking at existing consumers would
point out the possible pitfall.

> Also, since this is only done on the BSP on the last iteration I
> wonder if it really makes such a difference performance-wise to
> warrant all this trouble.

By "all this trouble", do you mean the outlined further steps or
the patch itself? In the latter case, while it's only the BSP to
read the value, all other CPUs are waiting for the BSP to get its
part done. So the extra time it takes to read the platform clock
affects the overall duration of the rendezvous, and hence the time
not "usefully" spent by _all_ of the CPUs.

Jan


  reply	other threads:[~2021-04-21 10:06 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-01  9:53 [PATCH v4 0/3] x86/time: calibration rendezvous adjustments Jan Beulich
2021-04-01  9:54 ` [PATCH v4 1/3] x86/time: latch to-be-written TSC value early in rendezvous loop Jan Beulich
2021-04-20 15:44   ` Roger Pau Monné
2021-04-01  9:54 ` [PATCH v4 2/3] x86/time: yield to hyperthreads after updating TSC during rendezvous Jan Beulich
2021-04-20 15:59   ` Roger Pau Monné
2021-04-21  9:57     ` Jan Beulich
2021-04-01  9:55 ` [PATCH v4 3/3] x86/time: avoid reading the platform timer in rendezvous functions Jan Beulich
2021-04-20 16:12   ` Roger Pau Monné
2021-04-21 10:06     ` Jan Beulich [this message]
2021-04-29  9:32       ` Jan Beulich
2021-04-29 12:48       ` Roger Pau Monné
2021-04-29 12:53   ` Roger Pau Monné
2021-04-29 13:51     ` Jan Beulich
2021-04-15  9:54 ` Ping: [PATCH v4 0/3] x86/time: calibration rendezvous adjustments Jan Beulich

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=88819ae1-d021-9192-4be7-a70064f23feb@suse.com \
    --to=jbeulich@suse.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=roger.pau@citrix.com \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    --subject='Re: [PATCH v4 3/3] x86/time: avoid reading the platform timer in rendezvous functions' \
    /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).