From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1753872AbdK3RgR (ORCPT <rfc822;w@1wt.eu>);
        Thu, 30 Nov 2017 12:36:17 -0500
Received: from mga07.intel.com ([134.134.136.100]:47884 "EHLO mga07.intel.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1752442AbdK3RgP (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Thu, 30 Nov 2017 12:36:15 -0500
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.45,341,1508828400"; 
   d="scan'208";a="8078563"
Date: Thu, 30 Nov 2017 09:32:01 -0800
From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
        platform-driver-x86@vger.kernel.org, x86@kernel.org
Cc: linux-kernel@vger.kernel.org, Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>,
        Darren Hart <dvhart@infradead.org>,
        Andy Shevchenko <andy@infradead.org>
Subject: Re: [PATCH v6 06/11] intel_sgx: driver for Intel Software Guard
 Extensions
Message-ID: <20171130173200.GA11190@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20171125193132.24321-7-jarkko.sakkinen@linux.intel.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Sat, Nov 25, 2017 at 09:29:24PM +0200, Jarkko Sakkinen wrote:
> +static void *sgx_try_alloc_page(void)
> +{
> +       struct sgx_epc_bank *bank;
> +       void *page = NULL;
> +       int i;
> +
> +       for (i = 0; i < sgx_nr_epc_banks; i++) {
> +               bank = &sgx_epc_banks[i];
> +
> +               down_write(&bank->lock);

Is a R/W semaphore actually preferable to a spinlock?  Concurrent
free calls don't seem that interesting/beneficial because freeing
an enclave's pages isn't multiplexed across multiple CPUs, unlike
the allocation of EPC pages.

As a whole, I'm not a fan of packing the EPC page pointers into an
array rather than encapsulating them in a struct+list.  The primary
benefit I see for the array approach is that it saves ~8 bytes per
free EPC page, but at a cost of increased memory usage for in-use
pages and severely restricting the ability to enhance/modify how
EPC pages are tracked, reclaimed, etc...

The main issue is that the array approach relies on the caller to
handle reclaim.  This effectively makes it impossible to reclaim
pages from multiple processes, requires other consumers e.g. KVM
to implement their own reclaim logic and kthread, and prevents
cgroup accounting because the cgroup can't initiate reclaim.

> +
> +               if (atomic_read(&bank->free_cnt))
> +                       page = bank->pages[atomic_dec_return(&bank->free_cnt)];
> +
> +               up_write(&bank->lock);
> +
> +               if (page)
> +                       break;
> +       }
> +
> +       if (page)
> +               atomic_dec(&sgx_nr_free_pages);
> +
> +       return page;
> +}