All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
To: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: linux-sgx@vger.kernel.org
Subject: Re: [PATCH for_v23 v3 12/12] x86/sgx: Reinstate per EPC section free page counts
Date: Wed, 23 Oct 2019 15:02:02 +0300	[thread overview]
Message-ID: <20191023120202.GD23733@linux.intel.com> (raw)
In-Reply-To: <20191022193530.GL2343@linux.intel.com>

On Tue, Oct 22, 2019 at 12:35:30PM -0700, Sean Christopherson wrote:
> On Mon, Oct 21, 2019 at 02:19:08PM +0300, Jarkko Sakkinen wrote:
> > On Fri, Oct 18, 2019 at 07:30:57AM -0700, Sean Christopherson wrote:
> > > On Fri, Oct 18, 2019 at 03:49:42PM +0300, Jarkko Sakkinen wrote:
> > > > On Wed, Oct 16, 2019 at 11:37:45AM -0700, Sean Christopherson wrote:
> > > > > Track the free page count on a per EPC section basis so that the value
> > > > > is properly protected by the section's spinlock.
> > > > > 
> > > > > As was pointed out when the change was proposed[*], using a global
> > > > > non-atomic counter to track the number of free EPC pages is not safe.
> > > > > The order of non-atomic reads and writes are not guaranteed, i.e.
> > > > > concurrent RMW operats can write stale data.  This causes a variety
> > > > > of bad behavior, e.g. livelocks because the free page count wraps and
> > > > > causes the swap thread to stop reclaiming.
> > > > > 
> > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > > 
> > > > What is the reason not change it just to atomic?
> > > 
> > > The purpose of separate sections is to avoid bouncing locks and whatnot
> > > across packages.  Adding a global atomic to the hotpath defeats that
> > > purpose.
> > 
> > I do get that but it does not actually cause incorrect behaviour,
> > right? Not being atomic obivously does because READ part of the
> > READ+STORE can get re-ordered.
> 
> Haven't tested yet, but it should be functionally correct.  I just don't
> understand the motivation for the change to a global free count.  I get
> that we don't have any NUMA awareness whatsoever, but if that's the
> argument, why bother with the complexity of per-section tracking in the
> first place?

You are right what you are saying. We can revert to the aggregation
code. I'm just checking that I exactly get the point when it comes
to concurrency issues.

I can take care of reverting it as I broke it.

/Jarkko

  reply	other threads:[~2019-10-23 12:02 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-16 18:37 [PATCH for_v23 v3 00/12] x86/sgx: Bug fixes for v23 Sean Christopherson
2019-10-16 18:37 ` [PATCH for_v23 v3 01/12] x86/sgx: Pass EADD the kernel's virtual address for the source page Sean Christopherson
2019-10-18  9:57   ` Jarkko Sakkinen
2019-10-22  3:22     ` Sean Christopherson
2019-10-23 11:57       ` Jarkko Sakkinen
2019-10-16 18:37 ` [PATCH for_v23 v3 02/12] x86/sgx: Check the validity of the source page address for EADD Sean Christopherson
2019-10-16 18:37 ` [PATCH for_v23 v3 03/12] x86/sgx: Fix EEXTEND error handling Sean Christopherson
2019-10-18 10:42   ` Jarkko Sakkinen
2019-10-16 18:37 ` [PATCH for_v23 v3 04/12] x86/sgx: Drop mmap_sem before EEXTENDing an enclave page Sean Christopherson
2019-10-18 10:04   ` Jarkko Sakkinen
2019-10-16 18:37 ` [PATCH for_v23 v3 05/12] x86/sgx: Remove redundant message from WARN on non-emtpy mm_list Sean Christopherson
2019-10-18 12:08   ` Jarkko Sakkinen
2019-10-16 18:37 ` [PATCH for_v23 v3 06/12] x86/sgx: Fix a memory leak in sgx_encl_destroy() Sean Christopherson
2019-10-18 12:17   ` Jarkko Sakkinen
2019-10-16 18:37 ` [PATCH for_v23 v3 07/12] x86/sgx: WARN on any non-zero return from __eremove() Sean Christopherson
2019-10-16 18:37 ` [PATCH for_v23 v3 08/12] x86/sgx: WARN only once if EREMOVE fails Sean Christopherson
2019-10-16 18:37 ` [PATCH for_v23 v3 09/12] x86/sgx: Split second half of sgx_free_page() to a separate helper Sean Christopherson
2019-10-18 10:06   ` Jarkko Sakkinen
2019-10-22  3:36     ` Sean Christopherson
2019-10-23 11:59       ` Jarkko Sakkinen
2019-10-16 18:37 ` [PATCH for_v23 v3 10/12] x86/sgx: Use the post-reclaim variant of __sgx_free_page() Sean Christopherson
2019-10-16 18:37 ` [PATCH for_v23 v3 11/12] x86/sgx: Don't update free page count if EPC section allocation fails Sean Christopherson
2019-10-16 18:37 ` [PATCH for_v23 v3 12/12] x86/sgx: Reinstate per EPC section free page counts Sean Christopherson
2019-10-18 12:49   ` Jarkko Sakkinen
2019-10-18 12:55     ` Jarkko Sakkinen
2019-10-18 14:30     ` Sean Christopherson
2019-10-21 11:19       ` Jarkko Sakkinen
2019-10-22 19:35         ` Sean Christopherson
2019-10-23 12:02           ` Jarkko Sakkinen [this message]
2019-10-17 18:10 ` [PATCH for_v23 v3 00/12] x86/sgx: Bug fixes for v23 Jarkko Sakkinen
2019-10-17 18:12   ` Jarkko Sakkinen
2019-10-18 13:13   ` Jarkko Sakkinen

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=20191023120202.GD23733@linux.intel.com \
    --to=jarkko.sakkinen@linux.intel.com \
    --cc=linux-sgx@vger.kernel.org \
    --cc=sean.j.christopherson@intel.com \
    /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 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.