[intel-sgx-kernel-dev] intel_sgx: clean up ksgxswapd
diff mbox

Message ID 20170201200248.15137-1-jarkko.sakkinen@linux.intel.com
State New
Headers show

Commit Message

Jarkko Sakkinen Feb. 1, 2017, 8:02 p.m. UTC
Use wait_event() and with its help clean up all the unnecessary cruft
from the implementation of ksgxswapd.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/platform/x86/intel_sgx_page_cache.c | 27 ++++-----------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

Comments

Sean Christopherson Feb. 22, 2017, 9:59 p.m. UTC | #1
On Wed, 2017-02-01 at 22:02 +0200, Jarkko Sakkinen wrote:
> Use wait_event() and with its help clean up all the unnecessary cruft
> from the implementation of ksgxswapd.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
Tested-by:
Sean Christopherson <sean.j.christopherson@intel.com>


> ---
>  drivers/platform/x86/intel_sgx_page_cache.c | 27 ++++-----------------------
>  1 file changed, 4 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_sgx_page_cache.c
> b/drivers/platform/x86/intel_sgx_page_cache.c
> index d073057..95949de 100644
> --- a/drivers/platform/x86/intel_sgx_page_cache.c
> +++ b/drivers/platform/x86/intel_sgx_page_cache.c
> @@ -388,31 +388,12 @@ static void sgx_swap_pages(unsigned long nr_to_scan)
>  
>  int ksgxswapd(void *p)
>  {
> -	DEFINE_WAIT(wait);
> -	unsigned int nr_free;
> -	unsigned int nr_high;
> +	while (!kthread_should_stop()) {
> +		wait_event(ksgxswapd_waitq, kthread_should_stop() ||
> +			   sgx_nr_free_pages < sgx_nr_high_pages);
>  
> -	for ( ; ; ) {
> -		if (kthread_should_stop())
> -			break;
> -
> -		spin_lock(&sgx_free_list_lock);
> -		nr_free = sgx_nr_free_pages;
> -		nr_high = sgx_nr_high_pages;
> -		spin_unlock(&sgx_free_list_lock);
> -
> -		if (nr_free < nr_high) {
> +		if (sgx_nr_free_pages < sgx_nr_high_pages)
>  			sgx_swap_pages(SGX_NR_SWAP_CLUSTER_MAX);
> -			schedule();
> -		} else {
> -			prepare_to_wait(&ksgxswapd_waitq,
> -					&wait, TASK_INTERRUPTIBLE);
> -
> -			if (!kthread_should_stop())
> -				schedule();
> -
> -			finish_wait(&ksgxswapd_waitq, &wait);
> -		}
>  	}
>  
>  	pr_info("%s: done\n", __func__);
Jarkko Sakkinen Feb. 24, 2017, 3:14 p.m. UTC | #2
On Wed, Feb 22, 2017 at 01:59:00PM -0800, Sean Christopherson wrote:
> On Wed, 2017-02-01 at 22:02 +0200, Jarkko Sakkinen wrote:
> > Use wait_event() and with its help clean up all the unnecessary cruft
> > from the implementation of ksgxswapd.
> > 
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Tested-by:
> Sean Christopherson <sean.j.christopherson@intel.com>

Thanks. There were some merge conflicts in the patch that you sent
(probably because I've squashed my stuff). I'll fix those and apply
it.

/Jarkko

> 
> 
> > ---
> >  drivers/platform/x86/intel_sgx_page_cache.c | 27 ++++-----------------------
> >  1 file changed, 4 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel_sgx_page_cache.c
> > b/drivers/platform/x86/intel_sgx_page_cache.c
> > index d073057..95949de 100644
> > --- a/drivers/platform/x86/intel_sgx_page_cache.c
> > +++ b/drivers/platform/x86/intel_sgx_page_cache.c
> > @@ -388,31 +388,12 @@ static void sgx_swap_pages(unsigned long nr_to_scan)
> >  
> >  int ksgxswapd(void *p)
> >  {
> > -	DEFINE_WAIT(wait);
> > -	unsigned int nr_free;
> > -	unsigned int nr_high;
> > +	while (!kthread_should_stop()) {
> > +		wait_event(ksgxswapd_waitq, kthread_should_stop() ||
> > +			   sgx_nr_free_pages < sgx_nr_high_pages);
> >  
> > -	for ( ; ; ) {
> > -		if (kthread_should_stop())
> > -			break;
> > -
> > -		spin_lock(&sgx_free_list_lock);
> > -		nr_free = sgx_nr_free_pages;
> > -		nr_high = sgx_nr_high_pages;
> > -		spin_unlock(&sgx_free_list_lock);
> > -
> > -		if (nr_free < nr_high) {
> > +		if (sgx_nr_free_pages < sgx_nr_high_pages)
> >  			sgx_swap_pages(SGX_NR_SWAP_CLUSTER_MAX);
> > -			schedule();
> > -		} else {
> > -			prepare_to_wait(&ksgxswapd_waitq,
> > -					&wait, TASK_INTERRUPTIBLE);
> > -
> > -			if (!kthread_should_stop())
> > -				schedule();
> > -
> > -			finish_wait(&ksgxswapd_waitq, &wait);
> > -		}
> >  	}
> >  
> >  	pr_info("%s: done\n", __func__);
>
Sean Christopherson Feb. 24, 2017, 4:55 p.m. UTC | #3
> On Wed, Feb 22, 2017 at 01:59:00PM -0800, Sean Christopherson wrote:
> > On Wed, 2017-02-01 at 22:02 +0200, Jarkko Sakkinen wrote:
> > > Use wait_event() and with its help clean up all the unnecessary cruft
> > > from the implementation of ksgxswapd.
> > > 
> > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > 
> > Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > Tested-by:
> > Sean Christopherson <sean.j.christopherson@intel.com>
> 
> Thanks. There were some merge conflicts in the patch that you sent
> (probably because I've squashed my stuff). I'll fix those and apply
> it.
> 
> /Jarkko

Found an issue with this patch after letting my machine sit idle for a while.
The wait_event macro uses TASK_UNINTERRUPTIBLE, which triggers the hung task
checks when the thread is idle.  The code should instead use wait_event_interruptible.

[ 3599.969498] INFO: task ksgxswapd:10754 blocked for more than 120 seconds.
[ 3599.976882]       Tainted: P           OE   4.4.0-64-generic #85-Ubuntu
[ 3599.984238] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this
message. [ 3599.998900] ksgxswapd       D ffff8808418a3e50     0 10754      2
0x00000000 [ 3599.998904]  ffff8808418a3e50 ffffffffc0bee34b ffff880841d23c00
ffff880827533c00 [ 3599.998906]  ffff8808418a4000 0000000000000000
ffffffffc0bef0d5 0000000000000000 [ 3599.998907]  0000000000000000
ffff8808418a3e68 ffffffff818384d5 ffff8808276ba300 [ 3599.998909] Call Trace:
[ 3599.998915]  [<ffffffffc0bee34b>] ? kref_put+0x25/0x27 [isgx]
[ 3599.998917]  [<ffffffffc0bef0d5>] ? sgx_swap_pages+0xff/0xff [isgx]
[ 3599.998920]  [<ffffffff818384d5>] schedule+0x35/0x80
[ 3599.998922]  [<ffffffffc0bef171>] ksgxswapd+0x9c/0x109 [isgx]
[ 3599.998924]  [<ffffffff810c41e0>] ? wake_atomic_t_function+0x60/0x60
[ 3599.998926]  [<ffffffff810a0ba8>] kthread+0xd8/0xf0
[ 3599.998928]  [<ffffffff810a0ad0>] ? kthread_create_on_node+0x1e0/0x1e0
[ 3599.998929]  [<ffffffff8183c98f>] ret_from_fork+0x3f/0x70
[ 3599.998931]  [<ffffffff810a0ad0>] ? kthread_create_on_node+0x1e0/0x1e0 


> > 
> > 
> > > ---
> > >  drivers/platform/x86/intel_sgx_page_cache.c | 27
> > > ++++----------------------- 1 file changed, 4 insertions(+), 23
> > > deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/intel_sgx_page_cache.c
> > > b/drivers/platform/x86/intel_sgx_page_cache.c
> > > index d073057..95949de 100644
> > > --- a/drivers/platform/x86/intel_sgx_page_cache.c
> > > +++ b/drivers/platform/x86/intel_sgx_page_cache.c
> > > @@ -388,31 +388,12 @@ static void sgx_swap_pages(unsigned long nr_to_scan)
> > >  
> > >  int ksgxswapd(void *p)
> > >  {
> > > -	DEFINE_WAIT(wait);
> > > -	unsigned int nr_free;
> > > -	unsigned int nr_high;
> > > +	while (!kthread_should_stop()) {
> > > +		wait_event(ksgxswapd_waitq, kthread_should_stop() ||
> > > +			   sgx_nr_free_pages < sgx_nr_high_pages);
> > >  
> > > -	for ( ; ; ) {
> > > -		if (kthread_should_stop())
> > > -			break;
> > > -
> > > -		spin_lock(&sgx_free_list_lock);
> > > -		nr_free = sgx_nr_free_pages;
> > > -		nr_high = sgx_nr_high_pages;
> > > -		spin_unlock(&sgx_free_list_lock);
> > > -
> > > -		if (nr_free < nr_high) {
> > > +		if (sgx_nr_free_pages < sgx_nr_high_pages)
> > >  			sgx_swap_pages(SGX_NR_SWAP_CLUSTER_MAX);
> > > -			schedule();
> > > -		} else {
> > > -			prepare_to_wait(&ksgxswapd_waitq,
> > > -					&wait, TASK_INTERRUPTIBLE);
> > > -
> > > -			if (!kthread_should_stop())
> > > -				schedule();
> > > -
> > > -			finish_wait(&ksgxswapd_waitq, &wait);
> > > -		}
> > >  	}
> > >  
> > >  	pr_info("%s: done\n", __func__);
> >
Jarkko Sakkinen Feb. 28, 2017, 7:48 a.m. UTC | #4
On Fri, Feb 24, 2017 at 04:55:19PM +0000, Christopherson, Sean J wrote:
> > On Wed, Feb 22, 2017 at 01:59:00PM -0800, Sean Christopherson wrote:
> > > On Wed, 2017-02-01 at 22:02 +0200, Jarkko Sakkinen wrote:
> > > > Use wait_event() and with its help clean up all the unnecessary cruft
> > > > from the implementation of ksgxswapd.
> > > > 
> > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > 
> > > Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > Tested-by:
> > > Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > Thanks. There were some merge conflicts in the patch that you sent
> > (probably because I've squashed my stuff). I'll fix those and apply
> > it.
> > 
> > /Jarkko
> 
> Found an issue with this patch after letting my machine sit idle for a while.
> The wait_event macro uses TASK_UNINTERRUPTIBLE, which triggers the hung task
> checks when the thread is idle.  The code should instead use wait_event_interruptible.

Good catch! I was able to reproduce this too. Thank you

/Jarkko

> 
> [ 3599.969498] INFO: task ksgxswapd:10754 blocked for more than 120 seconds.
> [ 3599.976882]       Tainted: P           OE   4.4.0-64-generic #85-Ubuntu
> [ 3599.984238] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this
> message. [ 3599.998900] ksgxswapd       D ffff8808418a3e50     0 10754      2
> 0x00000000 [ 3599.998904]  ffff8808418a3e50 ffffffffc0bee34b ffff880841d23c00
> ffff880827533c00 [ 3599.998906]  ffff8808418a4000 0000000000000000
> ffffffffc0bef0d5 0000000000000000 [ 3599.998907]  0000000000000000
> ffff8808418a3e68 ffffffff818384d5 ffff8808276ba300 [ 3599.998909] Call Trace:
> [ 3599.998915]  [<ffffffffc0bee34b>] ? kref_put+0x25/0x27 [isgx]
> [ 3599.998917]  [<ffffffffc0bef0d5>] ? sgx_swap_pages+0xff/0xff [isgx]
> [ 3599.998920]  [<ffffffff818384d5>] schedule+0x35/0x80
> [ 3599.998922]  [<ffffffffc0bef171>] ksgxswapd+0x9c/0x109 [isgx]
> [ 3599.998924]  [<ffffffff810c41e0>] ? wake_atomic_t_function+0x60/0x60
> [ 3599.998926]  [<ffffffff810a0ba8>] kthread+0xd8/0xf0
> [ 3599.998928]  [<ffffffff810a0ad0>] ? kthread_create_on_node+0x1e0/0x1e0
> [ 3599.998929]  [<ffffffff8183c98f>] ret_from_fork+0x3f/0x70
> [ 3599.998931]  [<ffffffff810a0ad0>] ? kthread_create_on_node+0x1e0/0x1e0 
> 
> 
> > > 
> > > 
> > > > ---
> > > >  drivers/platform/x86/intel_sgx_page_cache.c | 27
> > > > ++++----------------------- 1 file changed, 4 insertions(+), 23
> > > > deletions(-)
> > > > 
> > > > diff --git a/drivers/platform/x86/intel_sgx_page_cache.c
> > > > b/drivers/platform/x86/intel_sgx_page_cache.c
> > > > index d073057..95949de 100644
> > > > --- a/drivers/platform/x86/intel_sgx_page_cache.c
> > > > +++ b/drivers/platform/x86/intel_sgx_page_cache.c
> > > > @@ -388,31 +388,12 @@ static void sgx_swap_pages(unsigned long nr_to_scan)
> > > >  
> > > >  int ksgxswapd(void *p)
> > > >  {
> > > > -	DEFINE_WAIT(wait);
> > > > -	unsigned int nr_free;
> > > > -	unsigned int nr_high;
> > > > +	while (!kthread_should_stop()) {
> > > > +		wait_event(ksgxswapd_waitq, kthread_should_stop() ||
> > > > +			   sgx_nr_free_pages < sgx_nr_high_pages);
> > > >  
> > > > -	for ( ; ; ) {
> > > > -		if (kthread_should_stop())
> > > > -			break;
> > > > -
> > > > -		spin_lock(&sgx_free_list_lock);
> > > > -		nr_free = sgx_nr_free_pages;
> > > > -		nr_high = sgx_nr_high_pages;
> > > > -		spin_unlock(&sgx_free_list_lock);
> > > > -
> > > > -		if (nr_free < nr_high) {
> > > > +		if (sgx_nr_free_pages < sgx_nr_high_pages)
> > > >  			sgx_swap_pages(SGX_NR_SWAP_CLUSTER_MAX);
> > > > -			schedule();
> > > > -		} else {
> > > > -			prepare_to_wait(&ksgxswapd_waitq,
> > > > -					&wait, TASK_INTERRUPTIBLE);
> > > > -
> > > > -			if (!kthread_should_stop())
> > > > -				schedule();
> > > > -
> > > > -			finish_wait(&ksgxswapd_waitq, &wait);
> > > > -		}
> > > >  	}
> > > >  
> > > >  	pr_info("%s: done\n", __func__);
> > > 
> 
> 
>

Patch
diff mbox

diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c
index d073057..95949de 100644
--- a/drivers/platform/x86/intel_sgx_page_cache.c
+++ b/drivers/platform/x86/intel_sgx_page_cache.c
@@ -388,31 +388,12 @@  static void sgx_swap_pages(unsigned long nr_to_scan)
 
 int ksgxswapd(void *p)
 {
-	DEFINE_WAIT(wait);
-	unsigned int nr_free;
-	unsigned int nr_high;
+	while (!kthread_should_stop()) {
+		wait_event(ksgxswapd_waitq, kthread_should_stop() ||
+			   sgx_nr_free_pages < sgx_nr_high_pages);
 
-	for ( ; ; ) {
-		if (kthread_should_stop())
-			break;
-
-		spin_lock(&sgx_free_list_lock);
-		nr_free = sgx_nr_free_pages;
-		nr_high = sgx_nr_high_pages;
-		spin_unlock(&sgx_free_list_lock);
-
-		if (nr_free < nr_high) {
+		if (sgx_nr_free_pages < sgx_nr_high_pages)
 			sgx_swap_pages(SGX_NR_SWAP_CLUSTER_MAX);
-			schedule();
-		} else {
-			prepare_to_wait(&ksgxswapd_waitq,
-					&wait, TASK_INTERRUPTIBLE);
-
-			if (!kthread_should_stop())
-				schedule();
-
-			finish_wait(&ksgxswapd_waitq, &wait);
-		}
 	}
 
 	pr_info("%s: done\n", __func__);