xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Dario Faggioli <raistlin@linux.it>
To: Jan Beulich <JBeulich@suse.com>,
	Igor Druzhinin <igor.druzhinin@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Julien Grall <julien.grall@arm.com>,
	KevinTian <kevin.tian@intel.com>,
	Jun Nakajima <jun.nakajima@intel.com>
Subject: Re: Ping: [PATCH] VMX: sync CPU state upon vCPU destruction
Date: Tue, 21 Nov 2017 17:42:44 +0100	[thread overview]
Message-ID: <1511282564.2661.27.camel@linux.it> (raw)
In-Reply-To: <5A14544F02000078001909F8@prv-mh.provo.novell.com>


[-- Attachment #1.1: Type: text/plain, Size: 2394 bytes --]

On Tue, 2017-11-21 at 08:29 -0700, Jan Beulich wrote:
> > > > On 21.11.17 at 15:07, <igor.druzhinin@citrix.com> wrote:
> > 
> The question here is: In what other cases do we expect an RCU
> callback to possibly touch guest state? I think the common use is
> to merely free some memory in a delayed fashion.
> 
> > Those choices that you outlined appear to be different in terms
> > whether
> > we solve the general problem and probably have some minor
> > performance
> > impact or we solve the ad-hoc problem but make the system more
> > entangled. Here I'm more inclined to the first choice because this
> > particular scenario the performance impact should be negligible.
> 
> For the problem at hand there's no question about a
> performance effect. The question is whether doing this for _other_
> RCU callbacks would introduce a performance drop in certain cases.
> 
Well, I personally favour the approach of making the piece of code that
plays with the context responsible of not messing up when doing so.

And (replying to Igor comment above), I don't think that syncing
context before RCU handlers solves the general problem --as you're
calling it-- of "VMX code asynchronously messing up with the context". 
In fzct, it solves the specific problem of "VMX code called via RCU,
asynchronously messing up with the context".
There may be other places where (VMX?) code messes with context, *not*
from within an RCU handler, and that would still be an issue.

All that being said, given the nature of RCUs themselves, and given the
"precedent" we have for tasklets, I don't think it's a problem to sync
the state in rcu_do_batch().

Looking at users of call_rcu() (and trying to follow the call chains),
I think the only occasion where there may be an impact on perf, would
be when it's used in del_msixtbl_entry() (e.g., when that is called by
msixtbl_pt_unregister())... but I'm not familiar with that area of
code, so I may very well be wrong.

So, to summarize, if it were me doing this, I'd sync either in
vmx_vcpu_destroy() or in complete_domain_destroy(). But (for what it's
worth) I'm fine with it happening in rcu_do_batch().

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

  parent reply	other threads:[~2017-11-21 16:43 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-09 14:49 [PATCH] VMX: sync CPU state upon vCPU destruction Jan Beulich
2017-11-09 15:02 ` Dario Faggioli
2017-11-10  8:41 ` Sergey Dyasli
2017-11-10  9:50   ` Dario Faggioli
2017-11-10 10:30   ` Jan Beulich
2017-11-10 14:46     ` Igor Druzhinin
2017-11-13  9:51       ` Jan Beulich
2017-11-21 13:22 ` Ping: " Jan Beulich
2017-11-21 14:07   ` Igor Druzhinin
2017-11-21 15:29     ` Jan Beulich
2017-11-21 16:00       ` Igor Druzhinin
2017-11-21 16:42       ` Dario Faggioli [this message]
2017-11-21 16:58         ` George Dunlap
2017-11-21 17:00       ` Sergey Dyasli
2017-11-21 17:26         ` Jan Beulich
2017-11-21 16:08   ` George Dunlap
2017-11-21 16:26 ` Igor Druzhinin

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=1511282564.2661.27.camel@linux.it \
    --to=raistlin@linux.it \
    --cc=George.Dunlap@eu.citrix.com \
    --cc=JBeulich@suse.com \
    --cc=igor.druzhinin@citrix.com \
    --cc=julien.grall@arm.com \
    --cc=jun.nakajima@intel.com \
    --cc=kevin.tian@intel.com \
    --cc=xen-devel@lists.xenproject.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).