All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tamas K Lengyel <tamas@tklengyel.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Tamas K Lengyel" <tamas.lengyel@intel.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH] x86/mem_sharing: don't lock parent during fork reset
Date: Fri, 17 Sep 2021 10:21:41 -0400	[thread overview]
Message-ID: <CABfawh=B3zwfroiu=pPY5BTCMBkapYEGXixkRg66aXLQWxwU4A@mail.gmail.com> (raw)
In-Reply-To: <52b28c4d-1cf1-6c98-e1dc-1e0f7b2f768c@suse.com>

On Fri, Sep 17, 2021 at 3:26 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 16.09.2021 17:04, Tamas K Lengyel wrote:
> > During fork reset operation the parent domain doesn't need to be gathered using
> > rcu_lock_live_remote_domain_by_id as the fork reset doesn't modify anything on
> > the parent. The parent is also guaranteed to be paused while forks are active.
> > This patch reduces lock contention when performing resets in parallel.
>
> I'm a little in trouble following you here: RCU locks aren't really
> locks in that sense, so "lock contention" seems misleading to me. I
> can see that rcu_lock_domain_by_id()'s loop is extra overhead.
>
> Furthermore - does the parent being paused really mean the parent
> can't go away behind the back of the fork reset? In fork() I see
>
>     if ( rc && rc != -ERESTART )
>     {
>         domain_unpause(d);
>         put_domain(d);
>         cd->parent = NULL;
>     }
>
> i.e. the ref gets dropped before the parent pointer gets cleared. If
> the parent having a reference kept was indeed properly guaranteed, I
> agree the code change itself is fine.
>
> (The sequence looks correct at the other put_domain() site [dealing
> with the success case of fork(), when the reference gets retained]
> in domain_relinquish_resources().)

This code above you copied is when the fork() fails. Calling
fork_reset() before fork() successfully returns is not a sane sequence
and it is not "supported" by any means. If someone would try to do
that it would be racy as-is already with or without this patch.
Clearing the cd->parent pointer first here on the error path wouldn't
guarantee that sequence to be safe or sane either. Adding an extra
field to struct domain that signifies that "fork is complete" would be
a way to make that safe. But since the toolstack using this interface
is already sane (ie. never calls fork_reset before a successful fork)
I really don't think that's necessary. It would just grow struct
domain for very little benefit.

Tamas


  reply	other threads:[~2021-09-17 14:22 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-16 15:04 Tamas K Lengyel
2021-09-17  7:26 ` Jan Beulich
2021-09-17 14:21   ` Tamas K Lengyel [this message]
2021-09-20  8:14     ` 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='CABfawh=B3zwfroiu=pPY5BTCMBkapYEGXixkRg66aXLQWxwU4A@mail.gmail.com' \
    --to=tamas@tklengyel.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=george.dunlap@citrix.com \
    --cc=jbeulich@suse.com \
    --cc=roger.pau@citrix.com \
    --cc=tamas.lengyel@intel.com \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    --subject='Re: [PATCH] x86/mem_sharing: don'\''t lock parent during fork reset' \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.