Re: [PATCH v3 2/5] x86/sgx: Use sgx_free_epc_page() in sgx_reclaim_pages()

From: Jarkko Sakkinen <jarkko@kernel.org>
To: Dave Hansen <dave.hansen@intel.com>
Cc: linux-sgx@vger.kernel.org,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 2/5] x86/sgx: Use sgx_free_epc_page() in sgx_reclaim_pages()
Date: Wed, 10 Mar 2021 23:56:31 +0200	[thread overview]
Message-ID: <YElAj2zgismExjjM@kernel.org> (raw)
In-Reply-To: <b1b11501-805d-fcfd-4978-18d4b252510a@intel.com>

On Wed, Mar 10, 2021 at 07:55:35AM -0800, Dave Hansen wrote:
> On 3/10/21 7:11 AM, Jarkko Sakkinen wrote:
> >>> -		section = &sgx_epc_sections[epc_page->section];
> >>> -		spin_lock(&section->lock);
> >>> -		list_add_tail(&epc_page->list, &section->page_list);
> >>> -		section->free_cnt++;
> >>> -		spin_unlock(&section->lock);
> >>> +		sgx_free_epc_page(epc_page);
> >>>  	}
> >>>  }
> >> In current upstream (3fb6d0e00e), sgx_free_epc_page() calls __eremove().
> >>  This code does not call __eremove().  That seems to be changing
> >> behavior where none was intended.
> > EREMOVE does not matter here, as it doesn't in almost all most of the sites
> > where sgx_free_epc_page() is used in the driver. It does nothing to an
> > uninitialized pages.
> > 
> > The two patches that I posted originally for Kai's series took EREMOVE out
> > of sgx_free_epc_page() and put an explicit EREMOVE where it is actually
> > needed, but for reasons unknown to me, that change is gone.
> > 
> > Replacing the ad-hoc code with sgx_free_epc_page() is absolutely the right
> > action to take because it follows the pattern how sgx_free_epc_page() is
> > used in the driver.
> 
> That sounds generally fine.  But, this is a functional change.  Where
> there are functional changes, I always hope to see some mention of the
> change in the changelog.
> 
> Could you add some of this to the next changelog, please?

This appears for the first time in this patch set version, which means that
there is no patch changelog for this.

Maybe a better idea would be to explain the functional change in the commit
message (which of course implies also entry to the patch change log)?

/Jarkko